WildFly (formerly known as JBoss Application Server) is an open-source JavaEE application server developed and first released by JBoss in February, 2008. The primary goal of the project is to provide a set of tools usually required for enterprise Java applications. And since the server is used for developing enterprise applications, it is especially important to minimize the number of bugs and potential vulnerabilities in its code. Today, WildFly is being developed by the large company Red Hat, and they keep the code quality at a pretty high level. That said, our analyzer was still able to find a number of programming mistakes in the project.
My name is Dmitry, and I have just recently joined the PVS-Studio team as a Java programmer. As is well known, the best way to get started with a code analyzer is to try it on real code, so I decided to pick some interesting project, check it, and write an article based on the results of the check – and here it is for you to read. :)
The check was done on the source code of the WildFly project available at GitHub. Cloc counted 600 thousand lines of Java code, blank lines and comment lines excluded. The project was scanned for defects with PVS-Studio. PVS-Studio is a tool for detecting bugs and potential vulnerabilities in the source code of programs written in C, C++, C#, and Java. The analyzer was used as a plugin for IntelliJ IDEA 7.09.
The check revealed a total of 491 warnings, which indicates a high quality of WildFly's code. Among those, 113 are high-level warnings and 146 are medium-level warnings. A big portion of those, however, is associated with the following three diagnostic rules:
I won't look into those diagnostics in the article because it's usually hard to tell if the warnings point at genuine bugs or not. Warnings of these types are better to be left for the project authors to investigate.
Below, I'll discuss 10 warnings that looked most interesting to me. Why 10? Well, I just like the number. :)
Here we go!
V6004 The 'then' statement is equivalent to the 'else' statement. WeldPortableExtensionProcessor.java(61), WeldPortableExtensionProcessor.java(65).
@Override
public void deploy(DeploymentPhaseContext
phaseContext) throws DeploymentUnitProcessingException {
final DeploymentUnit deploymentUnit = phaseContext.getDeploymentUnit();
// for war modules we require a beans.xml to load portable extensions
if (PrivateSubDeploymentMarker.isPrivate(deploymentUnit)) {
if (!WeldDeploymentMarker.isPartOfWeldDeployment(deploymentUnit)) {
return;
}
} else {
// if any deployments have a beans.xml we need
// to load portable extensions
// even if this one does not.
if (!WeldDeploymentMarker.isPartOfWeldDeployment(deploymentUnit)) {
return;
}
}
}The if and else branches are identical, so the original conditional statement is useless. I'm not sure how this method came to be. It must have something to do with copy-paste or refactoring, if I was to guess.
V6007 Expression 'poolStatsSize > 0' is always true. PooledConnectionFactoryStatisticsService.java(85)
@Override
public void start(StartContext context) throws StartException {
....
if (poolStatsSize > 0) {
if (registration != null) {
if (poolStatsSize > 0) {
....
}
}
}
}This snippet contains duplicate conditions. This won't affect the program's logic, but it does make the code less comprehensible. An alternative explanation, though, is that the second condition might have been meant to check some other, stronger condition.
Examples of other warnings by this diagnostic in WildFly:
V6008 Null dereference of 'tc'. ExternalPooledConnectionFactoryService.java(382)
private void createService(ServiceTarget serviceTarget,
ServiceContainer container) throws Exception {
....
for (TransportConfiguration tc : connectors) {
if (tc == null) {
throw MessagingLogger.ROOT_LOGGER.connectorNotDefined(tc.getName());
}
}
....
}This code is an outright mess. The programmer first makes sure that the reference is null and then calls the getName method on this very null reference. As a result, a NullPointerException is thrown instead of the expected exception from connectorNotDefined(....).
V6019 Unreachable code detected. It is possible that an error is present. EJB3Subsystem12Parser.java(79)
V6037 An unconditional 'throw' within a loop. EJB3Subsystem12Parser.java(81)
protected void readAttributes(final XMLExtendedStreamReader reader)
throws XMLStreamException {
for (int i = 0; i < reader.getAttributeCount(); i++) {
ParseUtils.requireNoNamespaceAttribute(reader, i);
throw ParseUtils.unexpectedAttribute(reader, i);
}
}This is an utterly strange construct, and it triggered two diagnostics at once: V6019 and V6037. The loop iterates only once and then terminates on reaching the unconditional throw. If left as it is, the readAttributes method will throw an exception if reader contains at least one attribute. This loop can be replaced with an equivalent condition:
if(reader.getAttributeCount() > 0) {
throw ParseUtils.unexpectedAttribute(reader, 0);
}But let's dig a bit deeper and take a look at the requireNoNamespaceAttribute(....) method:
public static void requireNoNamespaceAttribute
(XMLExtendedStreamReader reader, int index)
throws XMLStreamException {
if (!isNoNamespaceAttribute(reader, index)) {
throw unexpectedAttribute(reader, index);
}
}As you can see, this method throws the same exception too. With the readAttributes method, then, the developer must have intended to check that none of the specified attributes belongs to any namespace rather than that no attributes are present at all. I'd say this construct appeared as a result of refactoring and the moving of the exception out into a separate method, requireNoNamespaceAttribute. But the commit history shows that all this code was added at the same time.
V6022 Parameter 'mechanismName' is not used inside constructor body. DigestAuthenticationMechanism.java(144)
public DigestAuthenticationMechanism(final String realmName,
final String domain,
final String mechanismName,
final IdentityManager identityManager,
boolean validateUri) {
this(Collections.singletonList(DigestAlgorithm.MD5),
Collections.singletonList(DigestQop.AUTH),
realmName, domain, new SimpleNonceManager(),
DEFAULT_NAME, identityManager, validateUri);
}Unused variables and parameters are usually nothing to worry about: for the most part, they linger as leftovers after refactoring or get added for later implementation of new features. But this particular warning seemed quite suspicious:
public DigestAuthenticationMechanism
(final List<DigestAlgorithm> supportedAlgorithms,
final List<DigestQop> supportedQops,
final String realmName,
final String domain,
final NonceManager nonceManager,
final String mechanismName,
final IdentityManager identityManager,
boolean validateUri) {....}If you look at the second constructor, you'll see that the string mechanizmName is supposed to be used as its sixth parameter. The first constructor gets a string of the same name as its third parameter and then calls the second constructor. But that string isn't used, and what gets passed to the second constructor instead is a constant. It means that the first constructor was probably meant to receive mechanismName instead of the DEFAULT_NAME constant.
V6033 An item with the same key 'org.apache.activemq.artemis.core.remoting.impl.netty.TransportConstants.NIO_REMOTING_THREADS_PROPNAME' has already been added. LegacyConnectionFactoryService.java(145), LegacyConnectionFactoryService.java(139)
private static final Map<String, String>
PARAM_KEY_MAPPING = new HashMap<>();
....
static {
PARAM_KEY_MAPPING.put(
org.apache.activemq.artemis.core.remoting.impl.netty
.TransportConstants.NIO_REMOTING_THREADS_PROPNAME,
TransportConstants.NIO_REMOTING_THREADS_PROPNAME);
....
PARAM_KEY_MAPPING.put(
org.apache.activemq.artemis.core.remoting.impl.netty
.TransportConstants.NIO_REMOTING_THREADS_PROPNAME,
TransportConstants.NIO_REMOTING_THREADS_PROPNAME);
....
}The analyzer is warning about two values being added to the dictionary for the same key. In this case, the key-value pairs being added are absolute duplicates. The values are constants from the TransportConstants class, so there's one of the two explanations: the programmer either accidentally cloned a code fragment or forgot to change the values in the copied-and-pasted fragment. A quick glance at the dictionary didn't reveal any missing keys and values, so I'd go for the first explanation.
V6046 Incorrect format. A different number of format items is expected. Missing arguments: 2. TxTestUtil.java(80)
public static void addSynchronization(TransactionManager tm,
TransactionCheckerSingletonRemote checker) {
try {
addSynchronization(tm.getTransaction(), checker);
} catch (SystemException se) {
throw new RuntimeException(String
.format("Can't obtain transaction for transaction manager '%s' "
+ "to enlist add test synchronization '%s'"), se);
}
}Variables missing! The programmer wanted two strings to be substituted into the format string but apparently forgot to add them. Using a format string without appropriate arguments will lead to throwing an IllegalFormatException instead of the expected RuntimeException. In fact, IllegalFormatException is inherited from RuntimeException, but since the error message passed to the exception will be missing in the output, it won't be easy to tell what exactly went wrong when you try to debug this.
V6058 The 'equals' function compares objects of incompatible types: String, ModelNode. JaxrsIntegrationProcessor.java(563)
// Send value to RESTEasy only if it's not null, empty string, or the
// default value.
private boolean isTransmittable(AttributeDefinition attribute,
ModelNode modelNode) {
if (modelNode == null || ModelType
.UNDEFINED.equals(modelNode.getType())) {
return false;
}
String value = modelNode.asString();
if ("".equals(value.trim())) {
return false;
}
return !value.equals(attribute.getDefaultValue()); // <=
}A string is compared with an object – and such comparison always returns false. That is, even if the value modelNode is equal to attribute.getDefaultValue(), the method will return false anyway and the value will be permitted for sending – contrary to what the comment says.
It seems the programmer forgot to call the asString() method to have attribute.getDefaultValue() represented as a string. This is what the fixed version could look like:
return !value.equals(attribute.getDefaultValue().asString());There is another V6058 warning in WildFly:
V6060 The 'dataSourceController' reference was utilized before it was verified against null. AbstractDataSourceAdd.java(399), AbstractDataSourceAdd.java(297)
static void secondRuntimeStep(OperationContext context, ModelNode operation,
ManagementResourceRegistration datasourceRegistration,
ModelNode model, boolean isXa) throws OperationFailedException {
final ServiceController<?> dataSourceController =
registry.getService(dataSourceServiceName);
....
dataSourceController.getService()
....
if (dataSourceController != null) {....}
....
}The analyzer has detected that an object is being used long before it gets checked for null, which doesn't happen until 102 lines later! You don't easily notice defects like that with manual code review.
V6082 Unsafe double-checked locking. A previously assigned object may be replaced by another object. JspApplicationContextWrapper.java(74), JspApplicationContextWrapper.java(72)
private volatile ExpressionFactory factory;
....
@Override
public ExpressionFactory getExpressionFactory() {
if (factory == null) {
synchronized (this) {
if (factory == null) {
factory = delegate.getExpressionFactory();
for (ExpressionFactoryWrapper wrapper : wrapperList) {
factory = wrapper.wrap(factory, servletContext);
}
}
}
}
return factory;
}The pattern used here is called "double-checked locking", and it might cause the method to return a partially initialized variable.
Thread A notices an uninitialized value, so it acquires a lock and starts initializing that value. But the thread will have already written the object to the field before the initialization is over. Now, thread B sees the newly created object and returns it even though thread A has not finished initializing factory yet.
As a result, the method might return an object before all intended operations have been performed on it.
Despite the project being developed by the large company Red Hat and the code quality is being kept at a high level, static analysis carried out by PVS-Studio has revealed a number of defects that could affect the server's work one way or another. And since WildFly is intended for creating enterprise applications, those defects may have grave implications.
Please don't hesitate to download PVS-Studio and try it on your projects. You can do this by submitting a form for a trial license or using one of the free-use options.