WildFly (formerly JBoss Application Server) is an open source JavaEE application server created by JBoss in February 2008. The main goal of the WildFly project is to provide a set of tools that are typically needed by enterprise Java applications. And since the server is used for developing enterprise applications, it is especially important to minimize the number of errors and possible vulnerabilities in the code. Now WildFly is being developed by a large company Red Hat, and the quality of the project code is maintained at a fairly high level, but the analyzer still managed to find a number of errors in the project.
My name is Dmitry, and recently I joined the PVS-Studio team as a Java programmer. As you know, the best way to get acquainted with the code analyzer is to try it in practice, so it was decided to choose some interesting project, check it and write an article about it based on the results. This is what you are reading now. :)
Project analysis
For analysis, I used the source code of the WildFly project published on GitHub . Cloc counted 600 thousand lines of Java code in the project, excluding blanks and comments. The search for errors in the code was carried out by PVS-Studio . PVS-Studio is a tool for finding bugs and potential vulnerabilities in the source code of programs written in C, C ++, C # and Java. The analyzer plugin for IntelliJ IDEA version 7.09 was used.
As a result of checking the project, only 491 analyzer triggers were received, which indicates a good level of WildFly code quality. Of these, 113 are high and 146 are medium. At the same time, a decent part of the detections falls on diagnostics:
- V6002. The switch statement does not cover all values of the enum.
- V6008. Potential null dereference.
- V6021. The value is assigned to the 'x' variable but is not used.
I have not considered the triggering of these diagnostics in the article, since it is difficult to understand whether they are in fact errors. Such warnings are better understood by the authors of the code.
Next, we will look at 10 analyzer triggers that I found the most interesting. Ask - why 10? Just because the number is like it. :)
So, let's go.
Warning N1 is a useless conditional statement
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 code in the if and else branches is the same, and the conditional operator does not make sense in its current form. It's hard to think of a reason why the developer wrote this method this way. Most likely, the error occurred as a result of copy-paste or refactoring.
N2 Warning - Duplicate Conditions
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) {
....
}
}
}
}In this case, there was a duplication of conditions. This will not affect the results of the program's work, but it worsens the readability of the code. However, it is possible that the second check should have contained some other, stronger condition.
Other examples of this diagnostic being triggered in WildFly:
- V6007 Expression 'referralMode == null' is always false. DirContext.java (93)
- V6007 Expression 'mBeanServer == null' is always true. WildFlyServerPlatform.java (82)
- V6007 Expression 'result! = Null' is always true. New returns not-null reference. JarCheck.java (84)
- V6007 Expression 'result' is always true. MultipleAdminObject2Impl.java (147)
Warning N3 - reference by null reference
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());
}
}
....
}They've obviously messed up here. First, we make sure that the reference is null, and then we call the getName method on this very null reference. This will result in a NullPointerException instead of the expected exception from connectorNotDefined (....).
Warning N4 - very strange code
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);
}
}An extremely strange design, to which two diagnostics reacted at once: V6019 and V6037 . Here only one iteration of the loop is performed, after which it exits by unconditional throw . As such, the readAttributes method throws 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);
}However, you can dig a little deeper and look at the requireNoNamespaceAttribute (....) method :
public static void requireNoNamespaceAttribute
(XMLExtendedStreamReader reader, int index)
throws XMLStreamException {
if (!isNoNamespaceAttribute(reader, index)) {
throw unexpectedAttribute(reader, index);
}
}It is found that this method internally throws the same exception. Most likely, the readAttributes method should check that none of the specified attributes belong to any namespace, and not that the attributes are missing. I would like to say that such a construction arose as a result of code refactoring and throwing an exception into the requireNoNamespaceAttribute method . Just looking at the commit history shows that all this code was added at the same time.
Warning N5 - passing parameters to constructor
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);
}Usually unused variables and function parameters are not something critical: in general, they remain after refactoring or are added to implement new functionality in the future. However, this operation seemed quite suspicious to me:
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 for the class, you can see that the mechanizmName string is assumed as the sixth parameter . The first constructor takes a string with the same name as the third parameter and calls the second constructor. However, this string is not used, and a constant is passed to the second constructor instead. Perhaps the author of the code here planned to pass the mechanismName to the constructor instead of the DEFAULT_NAME constant .
Warning N6 - duplicate lines
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 reports that two values with the same key have been added to the dictionary. In this case, the added key-value matches are completely duplicated. The recorded values are constants in the TransportConstants class , and there can be one of two options: either the author accidentally duplicated the code, or forgot to change the values during copy-paste. During a cursory inspection, I could not find missing keys and values, so I assume that the first scenario is more likely.
Warning N7 - Variables Lost
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 are lost (wanted). It was supposed that two other lines would be substituted into the formatted string, but the author of the code, apparently, forgot to add them. Formatting a string without appropriate arguments will throw an IllegalFormatException instead of the RuntimeException that the developers intended. In principle, IllegalFormatException inherits from RuntimeException , but the message passed to the exception will be lost in the output, and it will be more difficult to understand what exactly went wrong when debugging.
Warning N8 - string to object comparison
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()); // <=
}In this case, the string is compared to an object, and such a comparison is always false. That is, if a modelNode value equal to attribute.getDefaultValue () is passed to the method , then the behavior of the method will turn out to be incorrect, and the value will be recognized as valid for sending despite the comment.
Most likely, they forgot to call the asString () method to represent attribute.getDefaultValue () as a string. The corrected version might look like this:
return !value.equals(attribute.getDefaultValue().asString());WildFly has one more similar triggering of the V6058 diagnostic :
- V6058 The 'equals' function compares objects of incompatible types: String, ObjectTypeAttributeDefinition. DataSourceDefinition.java (141)
Warning N9 - Late Check
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 found that the object was used in the code long before it was checked for null , and the distance between them is as much as 102 lines of code! This is extremely difficult to notice when manually analyzing the code.
Warning N10 - double-checked locking
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;
}This uses the "double-checked locking" pattern, and a situation may occur when the method will return an incompletely initialized variable.
Thread A notices that the value has not been initialized, so it acquires the lock and starts initializing the value. In this case, the thread manages to write the object into the field even before it has been initialized to the end. Thread B detects that the object has been created and returns it, although thread A has not yet had time to do all the work with the factory .
As a result, an object may be returned from the method, on which not all planned actions have been performed.
conclusions
Despite the fact that the project is being developed by a large company Red Hat and the quality of the code in the project is at a high level, the static analysis carried out using PVS-Studio was able to reveal a certain number of errors that in one way or another may affect the operation of the server. And since WildFly is designed for creating enterprise applications, these errors can lead to extremely sad consequences.
I invite everyone to download PVS-Studio and check their project with it. To do this, you can request a trial license or use one of the free use cases.
If you want to share this article with an English-speaking audience, please use the translation link: Dmitry Scherbakov. Checking WildFly, a JavaEE Application Server .