O PVS-Studio para Java entra na estrada. A próxima parada é a Elasticsearch

A equipe do PVS-Studio mantém o blog sobre as verificações de projetos de código aberto pelo analisador de código estático de mesmo nome há muitos anos. Até o momento, mais de 300 projetos foram verificados, a base de erros contém mais de 12.000 casos. Inicialmente, o analisador foi implementado para verificar o código C e C ++, e o suporte ao C # foi adicionado posteriormente. Portanto, em todos os projetos verificados, a maioria (> 80%) responde por C e C ++. Recentemente, o Java foi adicionado à lista de linguagens suportadas, o que significa que agora existe um novo mundo aberto para o PVS-Studio, então é hora de complementar a base com erros de projetos Java.

O mundo Java é vasto e variado, portanto, nem se sabe onde procurar primeiro ao escolher um projeto para testar o novo analisador. Por fim, a escolha recaiu no mecanismo de busca e análise de texto completo Elasticsearch. É um projeto bem-sucedido e é especialmente agradável encontrar erros em projetos significativos. Então, quais defeitos o PVS-Studio for Java conseguiu detectar? Conversas adicionais serão corretas sobre os resultados da verificação.

Brevemente sobre elasticsearch

O Elasticsearch é um mecanismo de pesquisa e análise RESTful distribuído com código-fonte aberto, capaz de resolver um número crescente de casos de uso. Permite armazenar grandes quantidades de dados, realizar uma rápida pesquisa e análise (quase em tempo real). Normalmente, é usado como mecanismo / tecnologia subjacente, que fornece aos aplicativos funções complexas e requisitos de pesquisa.

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

Tudo bem, chega de introdução.

Toda a história de como as coisas eram

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

• Elasticsearch baixado do GitHub ;
• Siga as instruções de como executar o analisador Java e execute a análise;
• Recebeu o relatório do analisador, aprofundou-o e apontou casos interessantes.

Agora vamos ao ponto principal.

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 a sequência do buffer não foi lida, a chamada do método beginWith na condição da instrução if resultará no lançamento da exceção NullPointerException . Provavelmente, esse é um erro de digitação e, ao escrever uma condição, os desenvolvedores significaram o operador && 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 . O objeto followIndexMetadata despertou meu interesse. O método start aceita vários argumentos como entrada, nosso suspeito está certo entre eles. Depois disso, com base na verificação de nulo em nosso objeto , um novo objeto é criado, envolvido em uma lógica de método adicional. A verificação de nulo mostra que followIndexMetadata ainda pode vir de fora como um objeto nulo. Bem, vamos olhar mais longe.

Em seguida, vários argumentos são enviados para o método validate (novamente, existe um objeto considerado entre eles). Se olharmos para a implementação do método de validação, tudo se encaixa. Nosso objeto nulo em potencial é passado para o método validate como um terceiro argumento, onde é incondicionalmente desreferenciado. NullPointerException potencial como resultado.

 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 sabemos ao certo com quais argumentos o método start é chamado. É bem possível que todos os argumentos sejam verificados em algum lugar antes de chamar o método e nenhuma desreferência de objeto nulo ocorrerá. De qualquer forma, devemos admitir que essa implementação de código ainda não parece 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 com o mesmo problema foi acionada aqui. NullPointerException . A regra clama aos desenvolvedores: "Gente, o que você está fazendo? Como você pôde fazer isso? Oh, é horrível! Por que você primeiro usa o objeto e verifica se há nulo na próxima linha? Aqui está como a desreferência de objeto nulo acontece. Infelizmente, mesmo o comentário de um desenvolvedor 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(); } .... } .... } 

Nesse caso, devemos levar em consideração que o método getCause da classe Throwable pode retornar nulo . O problema acima se repete ainda mais, portanto sua explicação é desnecessária.

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 caractere de espaço, iniciando no índice i . O que há de errado? Temos o aviso do analisador de que s.charAt (i)! = '\ T' sempre é verdadeiro, o que significa a expressão (s.charAt (i)! = '' || s.charAt (i)! = '\ T ') sempre será verdade também. Isso é verdade? Eu acho que você pode ter certeza disso facilmente, substituindo qualquer caractere.

Como resultado, esse método sempre retornará o índice, igual a s.length () , que está errado. Atrevo-me a sugerir que o método acima é o culpado aqui:

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

Os desenvolvedores implementaram o método, copiaram e obtiveram o método errado findNextWhiteSpace, depois de fazer algumas edições. Eles continuaram consertando e consertando o método, mas não o consertaram. Para acertar, deve-se usar 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, não faz sentido comparar a variável restante com 0 e sempre será falsa; nesse ponto, o loop não sairá por uma condição. Remover o código ou reconsiderar a lógica de comportamento? Eu acho que apenas desenvolvedores poderão colocar todos os pontos sobre o i.

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 ....; }, ....); .... } 

Expressão sem sentido novamente. De acordo com a condição, a sequência healthCheckDn deve estar vazia e conter o caractere '=' não na primeira posição, para que a expressão lambda retorne a variável de sequência healthCheckDn . Ufa, isso é tudo! Como você provavelmente entendeu, é impossível. Não vamos nos aprofundar no código, vamos deixar a critério dos desenvolvedores.

Eu citei apenas alguns exemplos errôneos, mas além disso havia muitos disparos de diagnóstico do V6007 , que deveriam ser considerados um por um, tirando conclusões relevantes.

Pouco método pode percorrer um longo caminho

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

Então, aqui temos um método minúsculo de várias linhas. Mas os erros estão no relógio! A 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)

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

Edição N2. Possível índice de matriz fora dos limites, resultando na exceção ArrayIndexOutOfBoundsException . Em seguida, é apresentada a pergunta, que está na superfície: "Espere, e a verificação do í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 o método char64 recebe a variável x , a validade do índice é verificada. Onde está a falha? Por que o índice de matriz fora dos limites ainda é possível?

A verificação (int) x> index_64.length não está correta. Se o método char64 receber x com o valor 128, a verificação não protegerá contra ArrayIndexOutOfBoundsException. Talvez isso nunca aconteça na realidade. No entanto, a verificação está escrita incorretamente e é necessário alterar o operador "maior que" (>) por "maior que ou igual a (> =).

Comparações, que fizeram o seu melhor

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 que está incorreto aqui é que os objetos displaySize do tipo Inteiro são comparados usando o operador == , ou seja, por referência. É bem possível que objetos ColumnInfo , cujos campos displaySize tenham referências diferentes, mas com o mesmo conteúdo, sejam comparados. Nesse caso, a comparação nos dará o resultado negativo, quando esperávamos ser verdade.

Atrevo-me a supor que essa comparação possa ser o resultado de uma 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())) // <= && ....) } 

Comparação incorreta de objetos novamente. Desta vez, objetos com tipos incompatíveis são comparados ( Integer e TimeValue ). O resultado dessa comparação é óbvio e sempre falso. Você pode ver que os campos de classe são comparados entre si, apenas os nomes dos campos precisam ser alterados. Aqui está o ponto - um desenvolvedor decidiu acelerar o processo usando o copiar-colar e conseguiu um bug na barganha. A classe implementa um getter para o campo scrollSize ; portanto, para corrigir o erro, use o método datafeed .getScrollSize ().

Vejamos alguns exemplos errôneos sem nenhuma explicação. Os problemas são bastante óbvios.

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. Um desenvolvedor criou uma exceção, mas não a lançou em nenhum outro lugar. Essa exceção anônima é criada com sucesso e, o mais importante, será removida sem problemas. O motivo é a falta do operador de arremesso .

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 vários casos , uma das condições é repetida duas vezes, portanto a situação exige uma revisão de código competente.

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 outro tipo deve ser usado 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)); // <= } .... } 

Vamos considerar aqui o cenário incorreto que pode causar a exceção StringIndexOutOfBoundsException . A exceção ocorrerá quando request.uri () retornar uma string que contenha o caractere '#' before '?'. Não há verificações no método; caso isso ocorra, o problema está se formando. Talvez isso nunca aconteça devido a várias verificações do objeto fora do método, mas definir esperanças sobre isso não é a melhor idéia.

Conclusão

Por muitos anos, o PVS-Studio ajuda a encontrar defeitos no código de projetos comerciais e de código aberto gratuitos. Recentemente, o Java se juntou à lista de idiomas suportados para análise. O Elasticsearch se tornou um dos primeiros testes para o recém-chegado. Esperamos que essa verificação seja útil para o projeto e interessante para os leitores.

O PVS-Studio para Java precisa de novos desafios, novos usuários, feedback ativo e clientes para se adaptar rapidamente ao novo mundo :). Por isso, convido você a baixar e experimentar nosso analisador em seu projeto de trabalho imediatamente!