{"id":393671,"date":"2024-06-29T11:09:06","date_gmt":"2024-06-29T11:09:06","guid":{"rendered":"http:\/\/savepearlharbor.com\/?p=393671"},"modified":"-0001-11-30T00:00:00","modified_gmt":"-0001-11-29T21:00:00","slug":"","status":"publish","type":"post","link":"https:\/\/savepearlharbor.com\/?p=393671","title":{"rendered":"<span>Checking WildFly, a JavaEE Application Server<\/span>"},"content":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-1\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<div style=\"text-align:center;\"><img decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/getpro\/habr\/post_images\/535\/9eb\/5eb\/5359eb5eb321f4cb53f1eb442ea8b437.png\" alt=\"image1.png\" data-src=\"https:\/\/habrastorage.org\/getpro\/habr\/post_images\/535\/9eb\/5eb\/5359eb5eb321f4cb53f1eb442ea8b437.png\"\/><\/div>\n<p>  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.<br \/>  <a name=\"habracut\"><\/a><br \/>  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 \u2013 and here it is for you to read. \ud83d\ude42<\/p>\n<h2>Analyzing the project<\/h2>\n<p>  The check was done on the source code of the WildFly project available at <a href=\"https:\/\/github.com\/wildfly\/wildfly\/tree\/303dcde5ff8e5f3787320e376fb6c5ff3c4d039f\">GitHub<\/a>. <a href=\"https:\/\/github.com\/AlDanial\/cloc\">Cloc<\/a> counted 600 thousand lines of Java code, blank lines and comment lines excluded. The project was scanned for defects with <a href=\"https:\/\/www.viva64.com\/en\/pvs-studio-download\/\">PVS-Studio<\/a>. 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. <\/p>\n<p>  The check revealed a total of 491 warnings, which indicates a high quality of WildFly&#8217;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:<\/p>\n<ul>\n<li>V6002. The switch statement does not cover all values of the enum. <\/li>\n<li>V6008. Potential null dereference.<\/li>\n<li>V6021. The value is assigned to the &#8216;x&#8217; variable but is not used.<\/li>\n<\/ul>\n<p>  I won&#8217;t look into those diagnostics in the article because it&#8217;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.<\/p>\n<p>  Below, I&#8217;ll discuss 10 warnings that looked most interesting to me. Why 10? Well, I just like the number. \ud83d\ude42<\/p>\n<p>  Here we go!<\/p>\n<h2>Warning 1 \u2013 useless conditional statement<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6004\/\">V6004<\/a> The &#8216;then&#8217; statement is equivalent to the &#8216;else&#8217; statement. WeldPortableExtensionProcessor.java(61), WeldPortableExtensionProcessor.java(65).<\/p>\n<pre><code class=\"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;         }     } }<\/code><\/pre>\n<p>  The <i>if<\/i> and <i>else<\/i> branches are identical, so the original conditional statement is useless. I&#8217;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.<\/p>\n<h2>Warning 2 \u2013 duplicate conditions<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6007\/\">V6007<\/a> Expression &#8216;poolStatsSize > 0&#8217; is always true. PooledConnectionFactoryStatisticsService.java(85)<\/p>\n<pre><code class=\"cpp\">@Override public void start(StartContext context) throws StartException {   ....   if (poolStatsSize > 0) {     if (registration != null) {       if (poolStatsSize > 0) {         ....       }     }   } }<\/code><\/pre>\n<p>  This snippet contains duplicate conditions. This won&#8217;t affect the program&#8217;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.<\/p>\n<p>  Examples of other warnings by this diagnostic in WildFly:<\/p>\n<ul>\n<li>V6007 Expression &#8216;referralMode == null&#8217; is always false. <a href=\"https:\/\/github.com\/wildfly\/wildfly\/blob\/bf1c7695de3f9bd16426f83623cdc6341af36480\/testsuite\/shared\/src\/main\/java\/org\/wildfly\/test\/security\/common\/elytron\/DirContext.java\">DirContext.java<\/a>(93)<\/li>\n<li>V6007 Expression &#8216;mBeanServer == null&#8217; is always true. <a href=\"https:\/\/github.com\/wildfly\/wildfly\/blob\/1bf993be72b570a0c780a58ba05cabafe1ce7935\/jpa\/eclipselink\/src\/main\/java\/org\/jipijapa\/eclipselink\/WildFlyServerPlatform.java\">WildFlyServerPlatform.java<\/a>(82)<\/li>\n<li>V6007 Expression &#8216;result != null&#8217; is always true. New returns not-null reference. <a href=\"https:\/\/github.com\/wildfly\/wildfly\/blob\/1bf993be72b570a0c780a58ba05cabafe1ce7935\/jdr\/jboss-as-jdr\/src\/main\/java\/org\/jboss\/as\/jdr\/commands\/JarCheck.java\">JarCheck.java<\/a>(84)<\/li>\n<li>V6007 Expression &#8216;result&#8217; is always true. <a href=\"https:\/\/github.com\/wildfly\/wildfly\/blob\/1bf993be72b570a0c780a58ba05cabafe1ce7935\/testsuite\/integration\/basic\/src\/test\/java\/org\/jboss\/as\/test\/integration\/jca\/rar\/MultipleAdminObject2Impl.java\">MultipleAdminObject2Impl.java<\/a>(147)<\/li>\n<\/ul>\n<p>  <\/p>\n<h2>Warning 3 \u2013 null dereference<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6008\/\">V6008<\/a> Null dereference of &#8216;tc&#8217;. ExternalPooledConnectionFactoryService.java(382)<\/p>\n<pre><code class=\"cpp\">private void createService(ServiceTarget serviceTarget,          ServiceContainer container) throws Exception {    ....    for (TransportConfiguration tc : connectors) {      if (tc == null) {         throw MessagingLogger.ROOT_LOGGER.connectorNotDefined(tc.getName());      }    }    .... }<\/code><\/pre>\n<p>  This code is an outright mess. The programmer first makes sure that the reference is null and then calls the <i>getName<\/i> method on this very null reference. As a result, a <i>NullPointerException<\/i> is thrown instead of the expected exception from <i>connectorNotDefined(&#8230;.).<\/i><\/p>\n<h2>Warning 4 \u2013 extremely strange code<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6019\/\">V6019<\/a> Unreachable code detected. It is possible that an error is present. EJB3Subsystem12Parser.java(79)<\/p>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6037\/\">V6037<\/a> An unconditional &#8216;throw&#8217; within a loop. EJB3Subsystem12Parser.java(81)<\/p>\n<pre><code class=\"cpp\">protected void readAttributes(final XMLExtendedStreamReader reader)    throws XMLStreamException {     for (int i = 0; i &lt; reader.getAttributeCount(); i++) {       ParseUtils.requireNoNamespaceAttribute(reader, i);       throw ParseUtils.unexpectedAttribute(reader, i);     } }<\/code><\/pre>\n<p>  This is an utterly strange construct, and it triggered two diagnostics at once: <a href=\"https:\/\/www.viva64.com\/en\/w\/v6019\/\">V6019<\/a> and <a href=\"https:\/\/www.viva64.com\/en\/w\/v6037\/\">V6037<\/a>. The loop iterates only once and then terminates on reaching the unconditional <i>throw<\/i>. If left as it is, the <i>readAttributes<\/i> method will throw an exception if <i>reader<\/i> contains at least one attribute. This loop can be replaced with an equivalent condition:<\/p>\n<pre><code class=\"cpp\">if(reader.getAttributeCount() > 0) {   throw ParseUtils.unexpectedAttribute(reader, 0); }<\/code><\/pre>\n<p>  But let&#8217;s dig a bit deeper and take a look at the <i>requireNoNamespaceAttribute(&#8230;.)<\/i> method:<\/p>\n<pre><code class=\"cpp\">public static void requireNoNamespaceAttribute  (XMLExtendedStreamReader reader, int index)    throws XMLStreamException {    if (!isNoNamespaceAttribute(reader, index)) {         throw unexpectedAttribute(reader, index);    } }<\/code><\/pre>\n<p>  As you can see, this method throws the same exception too. With the <i>readAttributes<\/i> 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&#8217;d say this construct appeared as a result of refactoring and the moving of the exception out into a separate method, <i>requireNoNamespaceAttribute<\/i>. But the commit history shows that all this code was <a href=\"https:\/\/github.com\/wildfly\/wildfly\/blob\/2598dfcff01751afc7069949f322e8ae08688c38\/ejb3\/src\/main\/java\/org\/jboss\/as\/ejb3\/subsystem\/EJB3Subsystem12Parser.java\">added<\/a> at the same time.<\/p>\n<h2>Warning 5 \u2013 passing parameters to a constructor<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6022\/\">V6022<\/a> Parameter &#8216;mechanismName&#8217; is not used inside constructor body. DigestAuthenticationMechanism.java(144)<\/p>\n<pre><code class=\"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); }<\/code><\/pre>\n<p>  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: <\/p>\n<pre><code class=\"cpp\">public DigestAuthenticationMechanism   (final List&lt;DigestAlgorithm> supportedAlgorithms,     final List&lt;DigestQop> supportedQops,    final String realmName,     final String domain,     final NonceManager nonceManager,     final String mechanismName,     final IdentityManager identityManager,    boolean validateUri) {....}<\/code><\/pre>\n<p>  If you look at the second constructor, you&#8217;ll see that the string <i>mechanizmName<\/i> 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&#8217;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 <i>mechanismName<\/i> instead of the <i>DEFAULT_NAME<\/i> constant. <\/p>\n<h2>Warning 6 \u2013 duplicate lines<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6033\/\">V6033<\/a> An item with the same key &#8216;org.apache.activemq.artemis.core.remoting.impl.netty.<br \/>  TransportConstants.NIO_REMOTING_THREADS_PROPNAME&#8217; has already been added. LegacyConnectionFactoryService.java(145), LegacyConnectionFactoryService.java(139)<\/p>\n<pre><code class=\"cpp\">private static final Map&lt;String, String>  PARAM_KEY_MAPPING = new HashMap&lt;>(); .... 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);     .... }<\/code><\/pre>\n<p>  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 <i>TransportConstants<\/i> class, so there&#8217;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&#8217;t reveal any missing keys and values, so I&#8217;d go for the first explanation.<\/p>\n<h2>Warning 7 \u2013 missing variables<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6046\/\">V6046<\/a> Incorrect format. A different number of format items is expected. Missing arguments: 2. TxTestUtil.java(80)<\/p>\n<pre><code class=\"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);   } }<\/code><\/pre>\n<p>  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 <i>IllegalFormatException<\/i> instead of the expected <i>RuntimeException<\/i>. In fact, <i>IllegalFormatException<\/i> is inherited from <i>RuntimeException<\/i>, but since the error message passed to the exception will be missing in the output, it won&#8217;t be easy to tell what exactly went wrong when you try to debug this.<\/p>\n<h2>Warning 8 \u2013 comparing a string with an object<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6058\/\">V6058<\/a> The &#8216;equals&#8217; function compares objects of incompatible types: String, ModelNode. JaxrsIntegrationProcessor.java(563)<\/p>\n<pre><code class=\"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());        \/\/ &lt;= }<\/code><\/pre>\n<p>  A string is compared with an object \u2013 and such comparison always returns false. That is, even if the value <i>modelNode<\/i> is equal to <i>attribute.getDefaultValue()<\/i>, the method will return <i>false<\/i> anyway and the value will be permitted for sending \u2013 contrary to what the comment says.<\/p>\n<p>  It seems the programmer forgot to call the <i>asString()<\/i> method to have <i>attribute.getDefaultValue()<\/i> represented as a string. This is what the fixed version could look like:<\/p>\n<pre><code class=\"cpp\">return !value.equals(attribute.getDefaultValue().asString());<\/code><\/pre>\n<p>  There is another <a href=\"https:\/\/www.viva64.com\/en\/w\/v6058\/\">V6058<\/a> warning in WildFly:<\/p>\n<ul>\n<li>V6058 The &#8216;equals&#8217; function compares objects of incompatible types: String, ObjectTypeAttributeDefinition. <a href=\"https:\/\/github.com\/wildfly\/wildfly\/blob\/12da512624970c0e6e28aab85bdbef9fdac31108\/connector\/src\/main\/java\/org\/jboss\/as\/connector\/subsystems\/datasources\/DataSourceDefinition.java\">DataSourceDefinition.java<\/a>(141)<\/li>\n<\/ul>\n<p>  <\/p>\n<h2>Warning 9 \u2013 belated check<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6060\/\">V6060<\/a> The &#8216;dataSourceController&#8217; reference was utilized before it was verified against null. AbstractDataSourceAdd.java(399), AbstractDataSourceAdd.java(297)<\/p>\n<pre><code class=\"cpp\">static void secondRuntimeStep(OperationContext context, ModelNode operation,  ManagementResourceRegistration datasourceRegistration,  ModelNode model, boolean isXa) throws OperationFailedException {   final ServiceController&lt;?> dataSourceController =             registry.getService(dataSourceServiceName);   ....   dataSourceController.getService()     ....   if (dataSourceController != null) {....}   .... }<\/code><\/pre>\n<p>  The analyzer has detected that an object is being used long before it gets checked for <i>null<\/i>, which doesn&#8217;t happen until 102 lines later! You don&#8217;t easily notice defects like that with manual code review. <\/p>\n<h2>Warning 10 \u2014 double-checked locking<\/h2>\n<p>  <a href=\"https:\/\/www.viva64.com\/en\/w\/v6082\/\">V6082<\/a> Unsafe double-checked locking. A previously assigned object may be replaced by another object. JspApplicationContextWrapper.java(74), JspApplicationContextWrapper.java(72)<\/p>\n<pre><code class=\"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; }<\/code><\/pre>\n<p>  The pattern used here is called &#171;double-checked locking&#187;, and it might cause the method to return a partially initialized variable.<\/p>\n<p>  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 <i>factory<\/i> yet.<\/p>\n<p>  As a result, the method might return an object before all intended operations have been performed on it.<\/p>\n<h2>Conclusions<\/h2>\n<p>  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&#8217;s work one way or another. And since WildFly is intended for creating enterprise applications, those defects may have grave implications.<\/p>\n<p>  Please don&#8217;t hesitate to download PVS-Studio and try it on your projects. You can do this by submitting a form for a <a href=\"https:\/\/www.viva64.com\/en\/pvs-studio-download\/\">trial license<\/a> or using one of the <a href=\"https:\/\/www.viva64.com\/en\/b\/0614\/\">free-use<\/a> options.<\/div>\n<\/div>\n<\/div>\n<p><!----><!----><\/div>\n<p><!----><!----><br \/> \u0441\u0441\u044b\u043b\u043a\u0430 \u043d\u0430 \u043e\u0440\u0438\u0433\u0438\u043d\u0430\u043b \u0441\u0442\u0430\u0442\u044c\u0438 <a href=\"https:\/\/habr.com\/ru\/articles\/521756\/\"> https:\/\/habr.com\/ru\/articles\/521756\/<\/a><\/p>\n","protected":false},"excerpt":{"rendered":"<div><!--[--><!--]--><\/div>\n<div id=\"post-content-body\">\n<div>\n<div class=\"article-formatted-body article-formatted-body article-formatted-body_version-1\">\n<div xmlns=\"http:\/\/www.w3.org\/1999\/xhtml\">\n<div style=\"text-align:center;\"><img decoding=\"async\" src=\"https:\/\/habrastorage.org\/r\/w1560\/getpro\/habr\/post_images\/535\/9eb\/5eb\/5359eb5eb321f4cb53f1eb442ea8b437.png\" alt=\"image1.png\" data-src=\"https:\/\/habrastorage.org\/getpro\/habr\/post_images\/535\/9eb\/5eb\/5359eb5eb321f4cb53f1eb442ea8b437.png\"\/><\/div>\n<p>  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.  <\/p>\n","protected":false},"author":1,"featured_media":0,"comment_status":"open","ping_status":"open","sticky":false,"template":"","format":"standard","meta":{"footnotes":""},"categories":[],"tags":[],"class_list":["post-393671","post","type-post","status-publish","format-standard","hentry"],"_links":{"self":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/393671","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/users\/1"}],"replies":[{"embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcomments&post=393671"}],"version-history":[{"count":0,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=\/wp\/v2\/posts\/393671\/revisions"}],"wp:attachment":[{"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fmedia&parent=393671"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Fcategories&post=393671"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/savepearlharbor.com\/index.php?rest_route=%2Fwp%2Fv2%2Ftags&post=393671"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}