Clever Geek Handbook

Comprobación de WildFly - Servidor de aplicaciones JavaEE

image1.png


WildFly (anteriormente JBoss Application Server) es un servidor de aplicaciones JavaEE de código abierto creado por JBoss en febrero de 2008. El objetivo principal del proyecto WildFly es proporcionar un conjunto de herramientas que normalmente necesitan las aplicaciones empresariales Java. Y dado que el servidor se utiliza para desarrollar aplicaciones empresariales, es especialmente importante minimizar la cantidad de errores y posibles vulnerabilidades en el código. WildFly ahora está siendo desarrollado por una gran empresa, Red Hat, y la calidad del código del proyecto se mantiene en un nivel bastante alto, pero el analizador aún logró encontrar una serie de errores en el proyecto.



Mi nombre es Dmitry y recientemente me uní al equipo de PVS-Studio como programador de Java. Como sabes, la mejor manera de familiarizarte con el analizador de código es probarlo en la práctica, por lo que se decidió elegir algún proyecto interesante, revisarlo y escribir un artículo al respecto en base a los resultados. Esto es lo que estás leyendo ahora. :)



Análisis de proyectos



Para el análisis, utilicé el código fuente del proyecto WildFly publicado en GitHub . Cloc contó 600 mil líneas de código Java en el proyecto, sin incluir espacios en blanco ni comentarios. La búsqueda de errores en el código fue realizada por PVS-Studio . PVS-Studio es una herramienta para buscar errores y vulnerabilidades potenciales en el código fuente de programas escritos en C, C ++, C # y Java. Se utilizó el complemento analizador para IntelliJ IDEA versión 7.09.



Como resultado de verificar el proyecto, solo se recibieron 491 activadores del analizador, lo que indica un buen nivel de calidad del código WildFly. De estos, 113 son altos y 146 medianos. Al mismo tiempo, una parte decente de las detecciones recae en los diagnósticos:



  • V6002. La declaración de cambio no cubre todos los valores de la enumeración.
  • V6008. Potencial desreferencia nula.
  • V6021. El valor se asigna a la variable 'x' pero no se utiliza.


No he considerado la activación de estos diagnósticos en el artículo, ya que es difícil comprender si en realidad son errores. Los autores del código comprenden mejor estas advertencias.



A continuación, veremos los 10 disparadores del analizador que encontré más interesantes. Pregunte: ¿por qué 10? Solo porque el número es así. :)



Entonces, vamos.



Advertencia N1 es una declaración condicional inútil



V6004 La instrucción 'then' es equivalente a la instrucción 'else'. 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;
        }
    }
}


El código en las ramas if y else es el mismo, y el operador condicional no tiene sentido en su forma actual. Es difícil pensar en una razón por la que un desarrollador escribió este método de esta manera. Lo más probable es que el error se haya producido como resultado de copiar y pegar o refactorizar.



Advertencia de N2: condiciones duplicadas



La expresión V6007 'poolStatsSize> 0' siempre es verdadera. PooledConnectionFactoryStatisticsService.java (85) 



@Override
public void start(StartContext context) throws StartException {
  ....
  if (poolStatsSize > 0) {
    if (registration != null) {
      if (poolStatsSize > 0) {
        ....
      }
    }
  }
}


En este caso, hubo una duplicación de condiciones. Esto no afectará los resultados del programa, pero empeora la legibilidad del código. Sin embargo, es posible que la segunda comprobación debiera contener alguna otra condición más fuerte.



Otros ejemplos de este diagnóstico que se activa en WildFly:





Advertencia N3: referencia por referencia nula



V6008 Desreferencia nula de '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());
     }
   }
   ....
}


Obviamente se han equivocado aquí. Primero, nos aseguramos de que la referencia sea nula y luego llamamos al método getName en esta referencia tan nula. Esto dará como resultado una NullPointerException en lugar de la excepción esperada de connectorNotDefined (....).



Advertencia N4: código muy extraño



V6019 Se detectó un código inalcanzable. Es posible que exista un error. EJB3Subsystem12Parser.java (79)



V6037 Un 'lanzamiento' incondicional dentro de un bucle. 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);
    }
}


Un diseño extremadamente extraño, al que reaccionaron dos diagnósticos a la vez: V6019 y V6037 . Aquí solo se realiza una iteración del bucle, después de lo cual sale mediante un lanzamiento incondicional . Como tal, el método readAttributes lanza una excepción si el lector contiene al menos un atributo. Este bucle se puede reemplazar con una condición equivalente: 



if(reader.getAttributeCount() > 0) {
  throw ParseUtils.unexpectedAttribute(reader, 0);
}


Sin embargo, puede profundizar un poco más y mirar el método requireNoNamespaceAttribute (....) : 



public static void requireNoNamespaceAttribute
 (XMLExtendedStreamReader reader, int index) 
  throws XMLStreamException {
   if (!isNoNamespaceAttribute(reader, index)) {
        throw unexpectedAttribute(reader, index);
   }
}


Se encuentra que este método genera internamente la misma excepción. Lo más probable es que el método readAttributes compruebe que ninguno de los atributos especificados pertenezca a ningún espacio de nombres y que no falten los atributos. Me gustaría decir que tal construcción surgió como resultado de la refactorización del código y arrojar una excepción en el método requireNoNamespaceAttribute . Con solo mirar el historial de confirmaciones, se muestra que todo este código se agregó al mismo tiempo.



Advertencia N5: pasar parámetros al constructor



V6022 El parámetro ' MechanName ' no se usa dentro del cuerpo del constructor. 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);
}


Por lo general, las variables y los parámetros de función no utilizados no son algo crítico: en general, permanecen después de la refactorización o agregados para implementar nuevas funcionalidades en el futuro. Sin embargo, esta operación me pareció bastante sospechosa: 



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) {....}


Si observa el segundo constructor de la clase, puede ver que la cadena mechanizmName se asume como el sexto parámetro . El primer constructor toma una cadena con el mismo nombre que el tercer parámetro y llama al segundo constructor. Sin embargo, esta cadena no se usa y, en su lugar, se pasa una constante al segundo constructor. Quizás el autor del código aquí planeó pasar el MechanName al constructor en lugar de la constante DEFAULT_NAME .



Advertencia N6: líneas duplicadas



V6033 Un elemento con la misma clave 'org.apache.activemq.artemis.core.remoting.impl.netty.

TransportConstants.NIO_REMOTING_THREADS_PROPNAME 'ya se ha agregado. 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);
    ....
}


El analizador informa que se han agregado al diccionario dos valores con la misma clave. En este caso, las coincidencias de valor-clave agregadas están completamente duplicadas. Los valores registrados son constantes en la clase TransportConstants , y puede haber una de dos opciones: o el autor duplicó accidentalmente el código u olvidó cambiar los valores durante copiar y pegar. Durante una inspección superficial, no pude encontrar claves y valores faltantes, así que supongo que el primer escenario es más probable.



Advertencia N7 - Variables perdidas



V6046 Formato incorrecto. Se espera un número diferente de elementos de formato. Faltan argumentos: 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);
  }
}


Las variables se pierden (se buscan). Se suponía que otras dos líneas serían sustituidas en la cadena formateada, pero el autor del código, aparentemente, se olvidó de agregarlas. Dar formato a una cadena sin los argumentos adecuados arrojará una IllegalFormatException en lugar de la RuntimeException que pretendían los desarrolladores. En principio, IllegalFormatException hereda de RuntimeException , pero el mensaje pasado a la excepción se perderá en la salida y será más difícil entender qué salió mal exactamente al depurar.



Advertencia N8: comparación de cadena a objeto



V6058 La función 'igual' compara objetos de tipos incompatibles: 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());        // <=
}


En este caso, la cadena se compara con un objeto y esa comparación siempre es falsa. Es decir, si se pasa al método un valor modelNode igual a attribute.getDefaultValue () , el comportamiento del método será incorrecto y el valor se reconocerá como válido para enviar a pesar del comentario.



Lo más probable es que se hayan olvidado de llamar al método asString () para representar attribute.getDefaultValue () como una cadena. La versión corregida podría verse así: 



return !value.equals(attribute.getDefaultValue().asString());


WildFly tiene una activación más similar del diagnóstico V6058 :



  • V6058 La función 'igual' compara objetos de tipos incompatibles: String, ObjectTypeAttributeDefinition. DataSourceDefinition.java (141)


Advertencia N9 - Verificación tardía



V6060 Se utilizó la referencia 'dataSourceController' antes de que se verificara con un valor nulo. 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) {....}
  ....
}


El analizador descubrió que el objeto se usó en el código mucho antes de que se verificara si era nulo , ¡y la distancia entre ellos es de hasta 102 líneas de código! Esto es extremadamente difícil de notar al analizar manualmente el código.



Advertencia N10: bloqueo con doble verificación



V6082 Bloqueo inseguro verificado dos veces . Un objeto asignado previamente puede ser reemplazado por otro objeto. 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;
}


Esto usa el patrón de "bloqueo de doble verificación" y puede ocurrir una situación en la que el método devuelva una variable inicializada de forma incompleta.



El subproceso A advierte que el valor no se ha inicializado, por lo que adquiere el bloqueo y comienza a inicializar el valor. En este caso, el hilo se las arregla para escribir el objeto en el campo incluso antes de que se haya inicializado hasta el final. El hilo B detecta que el objeto ha sido creado y lo devuelve, aunque el hilo A todavía no ha tenido tiempo de hacer todo el trabajo con la fábrica .



Como resultado, se puede devolver un objeto del método en el que no se han realizado todas las acciones planificadas.



conclusiones



A pesar de que el proyecto está siendo desarrollado por una gran empresa Red Hat y la calidad del código en el proyecto es de un alto nivel, el análisis estático realizado mediante PVS-Studio pudo revelar una cierta cantidad de errores que de una forma u otra pueden afectar el funcionamiento del servidor. Y dado que WildFly está diseñado para crear aplicaciones empresariales, estos errores pueden tener consecuencias extremadamente tristes.



Los invito a todos a descargar PVS-Studio y comprobar su proyecto con él. Para hacer esto, puede solicitar una licencia de prueba o utilizar uno de los casos de uso gratuitos .





Si desea compartir este artículo con una audiencia de habla inglesa, utilice el enlace de traducción: Dmitry Scherbakov. Comprobando WildFly, un servidor de aplicaciones JavaEE .


More articles:


All Articles