O PVS-Studio para Java é enviado para o caminho. A próxima parada é a Elasticsearch

Longe do primeiro ano, a equipe do PVS-Studio tem blogado sobre verificações de projetos de código aberto pelo analisador de código estático de mesmo nome. Até o momento, mais de 300 projetos foram verificados e mais de 12.000 casos foram gravados no banco de dados de erros encontrados. Inicialmente, o analisador foi implementado para testar o código C e C ++ e, em seguida, apareceu o suporte à linguagem C #. Portanto, entre os projetos testados, a maioria (> 80%) recai sobre C e C ++. Mais recentemente, o Java foi adicionado às linguagens suportadas, o que significa que o PVS-Studio abre as portas para um novo mundo, e é hora de suplementar o banco de dados com erros de projetos Java.

O mundo Java é enorme e diversificado, então meus olhos se arregalam ao escolher um projeto para testar um novo analisador. Por fim, a escolha recaiu sobre o mecanismo de pesquisa de texto completo e o Elasticsearch de análise. Esse é um projeto bem-sucedido e, em projetos bem-sucedidos, encontrar erros é duplamente, ou até triplo, mais agradável. Então, quais defeitos o PVS-Studio detectou para Java? O resultado da verificação será discutido no artigo.

Conhecimento superficial do Elasticsearch

O Elasticsearch é um mecanismo de pesquisa e análise de texto completo escalável de código aberto. Ele permite que você armazene grandes quantidades de dados, conduza entre eles uma rápida pesquisa e análise (quase em tempo real). Normalmente, é usado como o mecanismo / tecnologia subjacente que fornece aos aplicativos funções complexas e requisitos de pesquisa.

Entre os principais sites que usam Elasticsearch, Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox estão mencionados.

Eu acho que com o conhecimento é suficiente.

Como foi

Não houve problemas com a verificação. A sequência de ações é bastante simples e não demorou muito tempo:

• Elasticsearch baixado do GitHub ;
• Usei as instruções para iniciar o analisador Java e iniciei a análise;
• Recebi um relatório do analisador, analisei e destaquei casos interessantes.

Agora vamos ao que interessa.

Cuidado Possível NullPointerException

V6008 Desreferência nula de 'linha'. GoogleCloudStorageFixture.java (451)

private static PathTrie<RequestHandler> defaultHandlers(....) { .... handlers.insert("POST /batch/storage/v1", (request) -> { .... // Reads the body line = reader.readLine(); byte[] batchedBody = new byte[0]; if ((line != null) || (line.startsWith("--" + boundary) == false)) // <= { batchedBody = line.getBytes(StandardCharsets.UTF_8); } .... }); .... } 

O erro nesse fragmento de código é que, se você não conseguir ler a linha do buffer, chamar o método beginWith na condição da instrução if lançará uma NullPointerException . Provavelmente, esse é um erro de digitação e, ao escrever a condição, o operador && foi criado em vez de || .

V6008 Desreferência nula potencial de 'followIndexMetadata'. TransportResumeFollowAction.java (171), TransportResumeFollowAction.java (170), TransportResumeFollowAction.java (194)

 void start( ResumeFollowAction.Request request, String clusterNameAlias, IndexMetaData leaderIndexMetadata, IndexMetaData followIndexMetadata, ....) throws IOException { MapperService mapperService = followIndexMetadata != null // <= ? .... : null; validate(request, leaderIndexMetadata, followIndexMetadata, // <= leaderIndexHistoryUUIDs, mapperService); .... } 

Outro aviso do diagnóstico V6008 . Agora, uma análise mais detalhada rebitou o objeto followIndexMetadata . O método start usa vários argumentos de entrada, incluindo o nosso suspeito. Então, com base na verificação de nulo em nosso objeto, um novo objeto é formado, que participa da lógica adicional do método. A verificação de null nos diz que followIndexMetadata ainda pode vir de fora com um objeto nulo. Ok, procure mais.

Em seguida, o método de validação é chamado com o envio de muitos argumentos (novamente, entre os quais o objeto em questão). E se você observar a implementação do método de validação, tudo se encaixará. Nosso potencial objeto nulo é passado pelo terceiro argumento para o método validate , onde é incondicionalmente desreferenciado. Como resultado, um potencial NullPointerException.

 static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex, // <= ....) { .... Map<String, String> ccrIndexMetadata = followIndex.getCustomData(....); // <= if (ccrIndexMetadata == null) { throw new IllegalArgumentException(....); } .... }} 

Não se sabe com quais argumentos o método start é realmente chamado. É possível que a verificação de todos os argumentos seja feita em algum lugar antes da chamada do método, e não enfrentamos nenhuma desreferência do objeto nulo. Mas, você deve admitir que essa implementação de código ainda parece pouco confiável e merece atenção.

V6060 A referência 'node' foi utilizada antes de ser verificada em relação a null. RestTasksAction.java (152), RestTasksAction.java (151)

 private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNodes discoveryNodes, TaskInfo taskInfo) { .... DiscoveryNode node = discoveryNodes.get(nodeId); .... // Node information. Note that the node may be null because it has // left the cluster between when we got this response and now. table.addCell(fullId ? nodeId : Strings.substring(nodeId, 0, 4)); table.addCell(node == null ? "-" : node.getHostAddress()); table.addCell(node.getAddress().address().getPort()); table.addCell(node == null ? "-" : node.getName()); table.addCell(node == null ? "-" : node.getVersion().toString()); .... } 

Outra regra de diagnóstico funcionou aqui, mas o problema é o mesmo: NullPointerException . A regra diz: “Gente, o que você está fazendo? Como assim? Oh problema! Por que você usa o objeto primeiro e, em seguida, na próxima linha de código, verifique se ele está nulo ?! ” Então, aqui desreferenciando um objeto nulo. Infelizmente, mesmo o comentário de um dos desenvolvedores não ajudou.

V6060 A referência 'causa' foi utilizada antes de ser verificada com relação a nulo. StartupException.java (76), StartupException.java (73)

 private void printStackTrace(Consumer<String> consumer) { Throwable originalCause = getCause(); Throwable cause = originalCause; if (cause instanceof CreationException) { cause = getFirstGuiceCause((CreationException)cause); } String message = cause.toString(); // <= consumer.accept(message); if (cause != null) { // <= // walk to the root cause while (cause.getCause() != null) { cause = cause.getCause(); } .... } .... } 

Deve-se notar aqui que o método getCause da classe Throwable pode retornar nulo . Além disso, o problema considerado acima é repetido e não faz sentido explicar algo em detalhes.

Condições sem sentido

A expressão V6007 's.charAt (i)! =' \ T '' é sempre verdadeira. Cron.java (1223)

 private static int findNextWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) { // intentionally empty } return i; } 

A função considerada retorna o índice do primeiro espaço, iniciando no índice i . O que está errado? Temos um aviso do analisador de que s.charAt (i)! = '\ T' é sempre verdadeiro, o que significa que sempre haverá verdade e a expressão (s.charAt (i)! = '' || s.charAt (i)! = '\ t') . É isso mesmo? Eu acho que você mesmo pode verificar isso facilmente, substituindo qualquer caractere.

Como resultado, esse método sempre retornará um índice igual a s.length () , o que não é verdadeiro. Atrevo-me a assumir que este método localizado um pouco mais alto é o culpado:

 private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) { // intentionally empty } return i; } 

Implementamos esse método, depois o copiamos e fizemos algumas pequenas correções para obter nosso método findNextWhiteSpace incorreto. O método foi ajustado, ajustado, mas não ajustado. Para remediar a situação, use o operador && em vez de || .

V6007 A expressão 'restante == 0' é sempre falsa. PemUtils.java (439)

 private static byte[] generateOpenSslKey(char[] password, byte[] salt, int keyLength) { .... int copied = 0; int remaining; while (copied < keyLength) { remaining = keyLength - copied; .... copied += bytesToCopy; if (remaining == 0) { // <= break; } .... } .... } 

A partir da condição do loop copiado <keyLength, podemos observar que as cópias sempre serão menores que keyLength . Portanto, uma comparação na igualdade da variável restante com 0 é inútil e sempre dará um resultado falso e, portanto, a condição não será encerrada do loop. Vale a pena excluir esse código ou ainda é necessário reconsiderar a lógica do comportamento? Eu acho que apenas desenvolvedores serão capazes de pontuar tudo o que eu.

A expressão V6007 'healthCheckDn.indexOf (' = ')> 0' é sempre falsa. ActiveDirectorySessionFactory.java (73)

 ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException { super(...., () -> { if (....) { final String healthCheckDn = ....; if (healthCheckDn.isEmpty() && healthCheckDn.indexOf('=') > 0) { return healthCheckDn; } } return ....; }, ....); .... } 

Mais uma vez uma expressão sem sentido. De acordo com a condição, para que a expressão lambda retorne a variável de sequência healthCheckDn , a sequência healthCheckDn deve estar vazia e a sequência que contém o caractere '=' não está na primeira posição. Fuh, meio que resolvido. E como você entendeu corretamente, isso é impossível. Não entenderemos a lógica do código, deixe para os desenvolvedores.

Dei apenas alguns exemplos errôneos, mas, além disso, houve muitos casos em que os diagnósticos do V6007 foram acionados , que devem ser considerados separadamente e conclusões tiradas.

O método é pequeno, mas udalenky

 private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; } 

Portanto, temos um método minúsculo de várias linhas. Mas insetos não dormem! Uma análise desse método deu o seguinte resultado:

1. A expressão V6007 '(int) x <0' é sempre falsa. BCrypt.java (429)
2. V6025 Possivelmente o índice '(int) x' está fora dos limites. BCrypt.java (431)

Problema N1. A expressão (int) x <0 é sempre falsa (Sim, sim, novamente V6007 ). A variável x não pode ser negativa, pois é do tipo char . O tipo de caractere é um número inteiro não assinado. Isso não pode ser chamado de um erro real, mas, no entanto, a verificação é redundante e pode ser removida.

Problema N2. Possível estouro da matriz que leva a uma ArrayIndexOutOfBoundsException . Então a pergunta começa, que está na superfície: "Espere, que tal verificar o índice?"

Portanto, temos uma matriz de tamanho fixo de 128 elementos:

 private static final byte index_64[] = { -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, -1, -1, -1, -1, -1, -1, -1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, -1, -1, -1, -1, -1, -1, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, -1, -1, -1, -1, -1 }; 

Quando a variável x entra na entrada do método char64 , ela verifica a validade do índice. Onde está a lacuna? Por que o caso de sair da matriz ainda é possível?

Verificar (int) x> index_64.length não está totalmente correto. Se x com um valor de 128 chegar na entrada do método char64 , a verificação não protegerá contra ArrayIndexOutOfBoundsException . Talvez isso nunca aconteça na realidade. No entanto, a verificação está incorreta e você precisa substituir o operador "maior que" (>) por "maior que ou igual a" (> =).

Comparações que tentaram

V6013 Os números 'displaySize' e 'that.displaySize' são comparados por referência. Possivelmente uma comparação de igualdade foi planejada. ColumnInfo.java (122)

 .... private final String table; private final String name; private final String esType; private final Integer displaySize; .... @Override public boolean equals(Object o) { if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } ColumnInfo that = (ColumnInfo) o; return displaySize == that.displaySize && // <= Objects.equals(table, that.table) && Objects.equals(name, that.name) && Objects.equals(esType, that.esType); } 

O erro aqui é que os objetos displaySize do tipo Inteiro são comparados pelo operador == , ou seja, são comparados por referência. É bem possível que os objetos ColumnInfo sejam comparados, para os quais os campos displaySize têm links diferentes, mas o mesmo conteúdo. E, neste caso, a comparação nos dará um resultado negativo, enquanto esperávamos a verdade.

Atrevo-me a sugerir que essa comparação poderia ser o resultado de refatoração com falha e, inicialmente, o campo displaySize era do tipo int .

V6058 A função 'equals' compara objetos de tipos incompatíveis: Inteiro, TimeValue. DatafeedUpdate.java (375)

 .... private final TimeValue queryDelay; private final TimeValue frequency; .... private final Integer scrollSize; .... boolean isNoop(DatafeedConfig datafeed) { return (frequency == null || Objects.equals(frequency, datafeed.getFrequency())) && (queryDelay == null || Objects.equals(queryDelay, datafeed.getQueryDelay())) && (scrollSize == null || Objects.equals(scrollSize, datafeed.getQueryDelay())) // <= && ....) } 

E novamente comparação incorreta de objetos. Agora compare objetos cujos tipos são incompatíveis ( Integer e TimeValue ). O resultado dessa comparação é óbvio e sempre falso. Pode-se observar que os campos da classe são comparados da mesma maneira, sendo necessário alterar apenas os nomes dos campos. Então, o desenvolvedor decidiu acelerar o processo de escrever código com copiar e colar, mas se premiou com o mesmo bug. A classe implementa um getter para o campo scrollSize ; portanto, para corrigir o erro, a solução correta seria usar o método apropriado: datafeed .getScrollSize () .

Vejamos mais alguns exemplos de erros sem nenhuma explicação. O problema já é óbvio.

V6001 Existem subexpressões idênticas 'takenInMillis' à esquerda e à direita do operador '=='. TermVectorsResponse.java (152)

 @Override public boolean equals(Object obj) { .... return index.equals(other.index) && type.equals(other.type) && Objects.equals(id, other.id) && docVersion == other.docVersion && found == other.found && tookInMillis == tookInMillis // <= && Objects.equals(termVectorList, other.termVectorList); } 

V6009 A função 'igual' recebe um argumento ímpar. Um objeto 'shardId.getIndexName ()' é usado como argumento para seu próprio método. SnapshotShardFailure.java (208)

 @Override public boolean equals(Object o) { .... return shardId.id() == that.shardId.id() && shardId.getIndexName().equals(shardId.getIndexName()) && // <= Objects.equals(reason, that.reason) && Objects.equals(nodeId, that.nodeId) && status.getStatus() == that.status.getStatus(); } 

Diversos

V6006 O objeto foi criado, mas não está sendo usado. A palavra-chave 'throw' pode estar ausente. JdbcConnection.java (88)

 @Override public void setAutoCommit(boolean autoCommit) throws SQLException { checkOpen(); if (!autoCommit) { new SQLFeatureNotSupportedException(....); } } 

O bug é óbvio e não requer explicações. O desenvolvedor lançou uma exceção, mas de maneira alguma a lança mais. Essa exceção anônima será criada com êxito e também com êxito e, mais importante, destruída sem deixar rastro. O motivo é a falta de uma declaração de lançamento .

V6003 O uso do padrão 'if (A) {....} else if (A) {....}' foi detectado. Há uma probabilidade de presença de erro lógico. MockScriptEngine.java (94), MockScriptEngine.java (105)

 @Override public <T> T compile(....) { .... if (context.instanceClazz.equals(FieldScript.class)) { .... } else if (context.instanceClazz.equals(FieldScript.class)) { .... } else if(context.instanceClazz.equals(TermsSetQueryScript.class)) { .... } else if (context.instanceClazz.equals(NumberSortScript.class)) .... } 

Em uma construção múltipla if-else, uma das condições é repetida duas vezes, portanto a situação requer uma revisão competente do código.

V6039 Existem duas instruções 'if' com expressões condicionais idênticas. A primeira instrução 'if' contém retorno de método. Isso significa que a segunda declaração 'if' não faz sentido. SearchAfterBuilder.java (94), SearchAfterBuilder.java (93)

 public SearchAfterBuilder setSortValues(Object[] values) { .... for (int i = 0; i < values.length; i++) { if (values[i] == null) continue; if (values[i] instanceof String) continue; if (values[i] instanceof Text) continue; if (values[i] instanceof Long) continue; if (values[i] instanceof Integer) continue; if (values[i] instanceof Short) continue; if (values[i] instanceof Byte) continue; if (values[i] instanceof Double) continue; if (values[i] instanceof Float) continue; if (values[i] instanceof Boolean) continue; // <= if (values[i] instanceof Boolean) continue; // <= throw new IllegalArgumentException(....); } .... } 

A mesma condição é usada duas vezes seguidas. A segunda condição é supérflua ou é necessário usar um tipo diferente em vez de booleano ?

V6009 A função 'substring' recebe argumentos ímpares. O argumento 'queryStringIndex + 1' não deve ser maior que 'queryStringLength'. LoggingAuditTrail.java (660)

 LogEntryBuilder withRestUriAndMethod(RestRequest request) { final int queryStringIndex = request.uri().indexOf('?'); int queryStringLength = request.uri().indexOf('#'); if (queryStringLength < 0) { queryStringLength = request.uri().length(); } if (queryStringIndex < 0) { logEntry.with(....); } else { logEntry.with(....); } if (queryStringIndex > -1) { logEntry.with(...., request.uri().substring(queryStringIndex + 1,// <= queryStringLength)); // <= } .... } 

Considere imediatamente um cenário de erro que pode gerar uma StringIndexOutOfBoundsException . Uma exceção ocorrerá quando request.uri () retornar uma sequência que contenha o caractere '#' anterior a '?'. Nesse caso, não há verificações no método e, se isso ainda ocorrer, o desastre não será evitado. Talvez isso nunca aconteça devido a várias verificações do objeto de solicitação fora do método, mas, na minha opinião, esperar que essa não seja a melhor idéia.

Conclusão

Ao longo dos anos, o PVS-Studio ajudou a encontrar falhas no código de projetos comerciais e de código aberto gratuitos. Mais recentemente, o Java foi adicionado ao suporte para linguagens analisadas. E um dos primeiros testes para o nosso recém-chegado foi o Elasticsearch, em desenvolvimento ativo. Esperamos que essa verificação seja útil para o projeto e interessante para os leitores.

Para que o PVS-Studio for Java se adapte rapidamente a um novo mundo, precisamos de novos testes, novos usuários, feedback ativo e clientes :). Então, sugiro, sem demora, fazer o download e testar nosso analisador em seu rascunho de trabalho!

Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Maxim Stefanov. O PVS-Studio para Java entra na estrada. A próxima parada é a Elasticsearch