Verificando WildFly - servidor de aplicativos JavaEE

image1.png


WildFly (anteriormente JBoss Application Server) é um servidor de aplicativos JavaEE de código aberto criado pela JBoss em fevereiro de 2008. O principal objetivo do projeto WildFly é fornecer um conjunto de ferramentas que são normalmente necessárias para aplicativos Java corporativos. E como o servidor é usado para desenvolver aplicativos corporativos, é especialmente importante minimizar o número de erros e possíveis vulnerabilidades no código. WildFly agora está sendo desenvolvido por uma grande empresa Red Hat, e a qualidade do código do projeto é mantida em um nível bastante alto, mas o analisador ainda conseguiu encontrar uma série de erros no projeto.



Meu nome é Dmitry e recentemente entrei para a equipe do PVS-Studio como programador Java. Como você sabe, a melhor maneira de se familiarizar com o analisador de código é experimentando-o na prática, então decidiu-se escolher algum projeto interessante, verificá-lo e escrever um artigo sobre ele baseado nos resultados. Isso é o que você está lendo agora. :)



Análise de projeto



Para a análise, usei o código-fonte do projeto WildFly publicado no GitHub . Cloc contabilizou 600 mil linhas de código Java no projeto, excluindo espaços em branco e comentários. A busca de erros no código foi realizada pelo PVS-Studio . PVS-Studio é uma ferramenta de busca de erros e vulnerabilidades potenciais no código-fonte de programas escritos em C, C ++, C # e Java. O plugin do analisador para IntelliJ IDEA versão 7.09 foi usado.



Como resultado da verificação do projeto, apenas 491 acionadores de analisador foram recebidos, o que indica um bom nível de qualidade do código WildFly. Destes, 113 são altos e 146 são médios. Ao mesmo tempo, uma parte decente das detecções recai sobre os diagnósticos:



  • V6002. A instrução switch não cobre todos os valores do enum.
  • V6008. Possível desreferência nula.
  • V6021. O valor é atribuído à variável 'x', mas não é usado.


Não considerei o desencadeamento desses diagnósticos no artigo, pois é difícil entender se são erros de fato. Esses avisos são melhor compreendidos pelos autores do código.



A seguir, veremos 10 acionadores de analisador que achei mais interessantes. Pergunte - por que 10? Só porque o número é parecido. :)



Então, vamos lá.



Aviso N1 é uma instrução condicional inútil



V6004 A instrução 'then' é equivalente à instrução '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;
        }
    }
}


O código nas ramificações if e else é o mesmo, e o operador condicional não faz sentido em sua forma atual. É difícil pensar em uma razão pela qual o desenvolvedor escreveu esse método dessa maneira. Provavelmente, o erro ocorreu como resultado de copiar e colar ou refatoração.



Aviso N2 - Condições duplicadas



A expressão V6007 'poolStatsSize> 0' é sempre verdadeira. PooledConnectionFactoryStatisticsService.java (85)



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


Neste caso, houve duplicação de condições. Isso não afetará os resultados do programa, mas piora a legibilidade do código. No entanto, é possível que a segunda verificação contenha alguma outra condição mais forte.



Outros exemplos desse diagnóstico sendo acionado no WildFly:





Aviso N3 - referência por referência nula



V6008 Dereferência 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());
     }
   }
   ....
}


Eles obviamente bagunçaram tudo aqui. Primeiro, certificamo-nos de que a referência é nula e, em seguida, chamamos o método getName nesta referência nula. Isso resultará em uma NullPointerException em vez da exceção esperada de connectorNotDefined (....).



Aviso N4 - código muito estranho



Código V6019 inacessível detectado. É possível que haja um erro. EJB3Subsystem12Parser.java (79)



V6037 Um 'lançamento' incondicional dentro de um 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);
    }
}


Um design extremamente estranho, ao qual dois diagnósticos reagiram ao mesmo tempo: V6019 e V6037 . Aqui, apenas uma iteração do loop é executada, após a qual ele sai por lançamento incondicional . Como tal, o método readAttributes lança uma exceção se o leitor contiver pelo menos um atributo. Este loop pode ser substituído por uma condição equivalente:



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


No entanto, você pode cavar um pouco mais fundo e olhar para o método requireNoNamespaceAttribute (....) :



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


Verifica-se que esse método gera internamente a mesma exceção. Provavelmente, o método readAttributes deve verificar se nenhum dos atributos especificados pertence a qualquer namespace e não se os atributos estão ausentes. Eu gostaria de dizer que tal construção surgiu como resultado da refatoração do código e lançando uma exceção no método requireNoNamespaceAttribute . Apenas olhar para o histórico de commits mostra que todo esse código foi adicionado ao mesmo tempo.



Aviso N5 - passando parâmetros para o construtor



O parâmetro V6022 ' engineName ' não é usado dentro do corpo do construtor. 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);
}


Normalmente, as variáveis ​​não utilizadas e os parâmetros de função não são algo crítico: em geral, eles permanecem após a refatoração ou são adicionados para implementar uma nova funcionalidade no futuro. No entanto, esta operação parecia bastante suspeita para mim:



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


Se você observar o segundo construtor da classe, verá que a string mechanizmName é assumida como o sexto parâmetro . O primeiro construtor pega uma string com o mesmo nome do terceiro parâmetro e chama o segundo construtor. No entanto, essa string não é usada e uma constante é passada para o segundo construtor. Talvez o autor do código aqui tenha planejado passar o mechanName para o construtor em vez da constante DEFAULT_NAME .



Aviso N6 - linhas duplicadas



V6033 Um item com a mesma chave 'org.apache.activemq.artemis.core.remoting.impl.netty.

TransportConstants.NIO_REMOTING_THREADS_PROPNAME 'já foi adicionado. 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);
    ....
}


O analisador relata que dois valores com a mesma chave foram adicionados ao dicionário. Nesse caso, as correspondências de valor-chave adicionado são completamente duplicadas. Os valores registrados são constantes na classe TransportConstants e pode haver uma de duas opções: o autor acidentalmente duplicou o código ou esqueceu de alterar os valores durante o copiar e colar. Durante uma inspeção rápida, não consegui encontrar chaves e valores ausentes, então presumo que o primeiro cenário seja mais provável.



Aviso N7 - Variáveis ​​perdidas



V6046 Formato incorreto. Um número diferente de itens de formato é esperado. Argumentos ausentes: 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);
  }
}


As variáveis ​​são perdidas (desejadas). Supunha-se que duas outras linhas seriam substituídas na string formatada, mas o autor do código, aparentemente, esqueceu de adicioná-las. A formatação de uma string sem argumentos apropriados lançará uma IllegalFormatException em vez da RuntimeException pretendida pelos desenvolvedores. Em princípio, IllegalFormatException herda de RuntimeException , mas a mensagem passada para a exceção será perdida na saída e será mais difícil entender o que exatamente deu errado durante a depuração.



Aviso N8 - comparação de string para objeto



V6058 A função 'equals' compara objetos de tipos incompatíveis: 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());        // <=
}


Nesse caso, a string é comparada a um objeto e tal comparação é sempre falsa. Ou seja, se um valor modelNode igual a attribute.getDefaultValue () for passado para o método , o comportamento do método ficará incorreto e o valor será reconhecido como válido para envio, apesar do comentário.



Provavelmente, eles se esqueceram de chamar o método asString () para representar attribute.getDefaultValue () como uma string. A versão corrigida pode ser assim:



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


WildFly tem mais um acionamento semelhante do diagnóstico V6058 :



  • V6058 A função 'equals' compara objetos de tipos incompatíveis: String, ObjectTypeAttributeDefinition. DataSourceDefinition.java (141)


Aviso N9 - Check Tardio



V6060 A referência 'dataSourceController' foi utilizada antes de ser verificada em relação ao 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) {....}
  ....
}


O analisador descobriu que o objeto foi usado no código muito antes de ser verificado como nulo , e a distância entre eles é de até 102 linhas de código! Isso é extremamente difícil de perceber ao analisar manualmente o código.



Aviso N10 - bloqueio verificado duas vezes



V6082 Travamento inseguro com verificação dupla. Um objeto atribuído anteriormente pode ser substituído por outro 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;
}


Isso usa o padrão de "bloqueio de verificação dupla" e pode ocorrer uma situação em que o método retorne uma variável inicializada de forma incompleta.



O thread A percebe que o valor não foi inicializado, então adquire o bloqueio e começa a inicializar o valor. Nesse caso, a thread consegue gravar o objeto no campo antes mesmo de ter sido inicializado até o final. O thread B detecta que o objeto foi criado e o retorna, embora o thread A ainda não tenha tido tempo de fazer todo o trabalho com a fábrica .



Como resultado, um objeto pode ser retornado do método, no qual nem todas as ações planejadas foram executadas.



conclusões



Apesar de o projeto estar sendo desenvolvido por uma grande empresa Red Hat e a qualidade do código do projeto estar em um alto nível, a análise estática realizada utilizando o PVS-Studio foi capaz de revelar um certo número de erros que de uma forma ou de outra podem afetar o funcionamento do servidor. E como o WildFly foi projetado para criar aplicativos corporativos, esses erros podem levar a consequências extremamente tristes.



Convido a todos a baixar PVS-Studio e conferir seus projetos com ele. Para fazer isso, você pode solicitar uma licença de teste ou usar um dos casos de uso gratuitos .





Se você quiser compartilhar este artigo com um público que fala inglês, use o link de tradução: Dmitry Scherbakov. Verificando WildFly, um servidor de aplicativos JavaEE .



All Articles