
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:
- A expressão V6007 'referralMode == null' é sempre falsa. DirContext.java (93)
- A expressão V6007 'mBeanServer == null' é sempre verdadeira. WildFlyServerPlatform.java (82)
- A expressão V6007 'resultado! = Nulo' é sempre verdadeira. New retorna uma referência não nula. JarCheck.java (84)
- O 'resultado' da expressão V6007 é sempre verdadeiro. MultipleAdminObject2Impl.java (147)
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 .