# Checking WildFly, a JavaEE application server 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\. ![0763_WildFly_Check/image1.png](https://import.viva64.com/docx/blog/0763_WildFly_Check/image1.png) 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\. :\) ## Analyzing the project The check was done on the source code of the WildFly project available at [GitHub](https://github.com/wildfly/wildfly/tree/303dcde5ff8e5f3787320e376fb6c5ff3c4d039f)\. [Cloc](https://github.com/AlDanial/cloc) counted 600 thousand lines of Java code, blank lines and comment lines excluded\. The project was scanned for defects with [PVS\-Studio](https://pvs-studio.com/en/pvs-studio/download/)\. 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: * 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 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\! ## Warning 1 – useless conditional statement [V6004](https://pvs-studio.com/en/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. WeldPortableExtensionProcessor\.java\(61\), WeldPortableExtensionProcessor\.java\(65\)\. ```cpp @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\. ## Warning 2 – duplicate conditions [V6007](https://pvs-studio.com/en/docs/warnings/v6007/) Expression 'poolStatsSize \> 0' is always true\. PooledConnectionFactoryStatisticsService\.java\(85\) ```cpp @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: * V6007 Expression 'referralMode \=\= null' is always false\. [DirContext\.java](https://github.com/wildfly/wildfly/blob/bf1c7695de3f9bd16426f83623cdc6341af36480/testsuite/shared/src/main/java/org/wildfly/test/security/common/elytron/DirContext.java)\(93\) * V6007 Expression 'mBeanServer \=\= null' is always true\. [WildFlyServerPlatform\.java](https://github.com/wildfly/wildfly/blob/1bf993be72b570a0c780a58ba05cabafe1ce7935/jpa/eclipselink/src/main/java/org/jipijapa/eclipselink/WildFlyServerPlatform.java)\(82\) * V6007 Expression 'result \!\= null' is always true\. New returns not\-null reference\. [JarCheck\.java](https://github.com/wildfly/wildfly/blob/1bf993be72b570a0c780a58ba05cabafe1ce7935/jdr/jboss-as-jdr/src/main/java/org/jboss/as/jdr/commands/JarCheck.java)\(84\) * V6007 Expression 'result' is always true\. [MultipleAdminObject2Impl\.java](https://github.com/wildfly/wildfly/blob/1bf993be72b570a0c780a58ba05cabafe1ce7935/testsuite/integration/basic/src/test/java/org/jboss/as/test/integration/jca/rar/MultipleAdminObject2Impl.java)\(147\) ## Warning 3 – null dereference [V6008](https://pvs-studio.com/en/docs/warnings/v6008/) Null dereference of 'tc'\. ExternalPooledConnectionFactoryService\.java\(382\) ```cpp 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\(\.\.\.\.\)\._ ## Warning 4 – extremely strange code [V6019](https://pvs-studio.com/en/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. EJB3Subsystem12Parser\.java\(79\) [V6037](https://pvs-studio.com/en/docs/warnings/v6037/) An unconditional 'throw' within a loop\. EJB3Subsystem12Parser\.java\(81\) ```cpp 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](https://pvs-studio.com/en/docs/warnings/v6019/) and [V6037](https://pvs-studio.com/en/docs/warnings/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: ```cpp if(reader.getAttributeCount() > 0) { throw ParseUtils.unexpectedAttribute(reader, 0); } ``` But let's dig a bit deeper and take a look at the _requireNoNamespaceAttribute\(\.\.\.\.\)_ method: ```cpp 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](https://github.com/wildfly/wildfly/blob/2598dfcff01751afc7069949f322e8ae08688c38/ejb3/src/main/java/org/jboss/as/ejb3/subsystem/EJB3Subsystem12Parser.java) at the same time\. ## Warning 5 – passing parameters to a constructor [V6022](https://pvs-studio.com/en/docs/warnings/v6022/) Parameter 'mechanismName' is not used inside constructor body\. DigestAuthenticationMechanism\.java\(144\) ```cpp 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: ```cpp public DigestAuthenticationMechanism (final List supportedAlgorithms, final List 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\. ## Warning 6 – duplicate lines [V6033](https://pvs-studio.com/en/docs/warnings/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\) ```cpp private static final Map 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\. ## Warning 7 – missing variables [V6046](https://pvs-studio.com/en/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Missing arguments: 2\. TxTestUtil\.java\(80\) ```cpp 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\. ## Warning 8 – comparing a string with an object [V6058](https://pvs-studio.com/en/docs/warnings/v6058/) The 'equals' function compares objects of incompatible types: String, ModelNode\. JaxrsIntegrationProcessor\.java\(563\) ```cpp // 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: ```cpp return !value.equals(attribute.getDefaultValue().asString()); ``` There is another [V6058](https://pvs-studio.com/en/docs/warnings/v6058/) warning in WildFly: * V6058 The 'equals' function compares objects of incompatible types: String, ObjectTypeAttributeDefinition\. [DataSourceDefinition\.java](https://github.com/wildfly/wildfly/blob/12da512624970c0e6e28aab85bdbef9fdac31108/connector/src/main/java/org/jboss/as/connector/subsystems/datasources/DataSourceDefinition.java)\(141\) ## Warning 9 – belated check [V6060](https://pvs-studio.com/en/docs/warnings/v6060/) The 'dataSourceController' reference was utilized before it was verified against null\. AbstractDataSourceAdd\.java\(399\), AbstractDataSourceAdd\.java\(297\) ```cpp 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\. ## Warning 10 \- double\-checked locking [V6082](https://pvs-studio.com/en/docs/warnings/v6082/) Unsafe double\-checked locking\. A previously assigned object may be replaced by another object\. JspApplicationContextWrapper\.java\(74\), JspApplicationContextWrapper\.java\(72\) ```cpp 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\. ## Conclusions 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](https://pvs-studio.com/en/pvs-studio/download/) or using one of the [free\-use](https://pvs-studio.com/en/blog/posts/0614/) options\.