Análise do Apache Dubbo RPC Framework pelo analisador de código estático PVS-Studio

O Apache Dubbo é um dos projetos Java mais populares no GitHub. Isso não é surpreendente. Foi criado há 8 anos e é amplamente aplicado como um ambiente RPC de alto desempenho. Obviamente, muitos dos erros em seu código foram corrigidos há muito tempo e a qualidade do código é mantida em um nível alto. No entanto, não há razão para optar por não verificar um projeto tão interessante usando o analisador de código estático PVS-Studio. Vamos ver como ficou.

Sobre o PVS-Studio

O analisador de código estático PVS-Studio existe há mais de 10 anos no mercado de TI e é uma solução de software multifuncional e fácil de introduzir. No momento, o analisador suporta C, C ++, C #, Java e funciona no Windows, Linux e macOS.

O PVS-Studio é uma solução B2B e é usado por um grande número de equipes em várias empresas. Se você deseja estimar os recursos do analisador, faça o download do distributivo e solicite uma chave de avaliação aqui .

Também existem opções de como você pode usar o PVS-Studio gratuitamente em projetos de código aberto ou se você é um estudante.

Apache Dubbo: O que é e os principais recursos

Atualmente, quase todos os grandes sistemas de software são distribuídos . Se em um sistema distribuído houver uma conexão interativa entre componentes remotos com baixa latência e relativamente poucos dados a serem transmitidos, é um forte motivo para usar um ambiente RPC (chamada de procedimento remoto).

O Apache Dubbo é um ambiente RPC (chamada de procedimento remoto) de alto desempenho com código-fonte aberto baseado em Java. Assim como muitos outros sistemas RPC, o dubbo é baseado na idéia de criar um serviço que define alguns métodos que podem ser chamados remotamente com seus parâmetros e tipos de valores de retorno. No lado do servidor, uma interface é implementada e o servidor do dubbo é iniciado para lidar com consultas de clientes. No lado do cliente, há um stub que fornece os mesmos métodos que o servidor. O Dubbo sugere três funções principais, que incluem chamadas remotas por interface, tolerância a falhas e balanceamento de carga, além de registro automático e descoberta de serviços.

Sobre a análise

A sequência de ações da análise é bastante simples e não exigiu muito tempo no meu caso:

• Apache Dubbo baixado do GitHub ;
• Utilizou instruções de inicialização do analisador Java e executou a análise;
• Obteve o relatório do analisador, reviu-o e destacou casos interessantes.

Os resultados da análise: 73 avisos de níveis alto e médio de certeza (46 e 27, respectivamente) foram emitidos para mais de 4000 arquivos, o que é um bom indicador da qualidade do código.

Nem todos os avisos são erros. É um resultado usual, como antes, usando diretamente o analisador, é preciso configurá-lo. Depois disso, você pode esperar uma porcentagem bastante baixa de falsos positivos ( exemplo ).

Não considerei 9 avisos (7 altos e 2 médios), relacionados a arquivos com testes.

Como resultado, recebi um pequeno número de avisos, que também incluíam falsos positivos, porque não configurei o analisador. Examinar todos os 73 avisos com mais descrição no artigo é longo, ridículo e tedioso, então eu escolhi os mais frívolos.

Byte de sinal em Java

A expressão V6007 'endKey [i] <0xff' sempre é verdadeira. OptionUtil.java (32)

public static final ByteSequence prefixEndOf(ByteSequence prefix) { byte[] endKey = prefix.getBytes().clone(); for (int i = endKey.length - 1; i >= 0; i--) { if (endKey[i] < 0xff) { // <= endKey[i] = (byte) (endKey[i] + 1); return ByteSequence.from(Arrays.copyOf(endKey, i + 1)); } } return ByteSequence.from(NO_PREFIX_END); } 

Um valor do tipo de byte (o valor de -128 a 127) é comparado com o valor 0xff (255). Nessa condição, não é levado em consideração que o tipo de byte em Java é assinado; portanto, a condição sempre será verdadeira, o que significa que não faz sentido.

Retorno dos mesmos valores

A expressão V6007 'isPreferIPV6Address ()' é sempre falsa. NetUtils.java (236)

 private static Optional<InetAddress> toValidAddress(InetAddress address) { if (address instanceof Inet6Address) { Inet6Address v6Address = (Inet6Address) address; if (isPreferIPV6Address()) { // <= return Optional.ofNullable(normalizeV6Address(v6Address)); } } if (isValidV4Address(address)) { return Optional.of(address); } return Optional.empty(); } 

O método éPreferIPV6Address .

 static boolean isPreferIPV6Address() { boolean preferIpv6 = Boolean.getBoolean("java.net.preferIPv6Addresses"); if (!preferIpv6) { return false; // <= } return false; // <= } 

O método isPreferIPV6Address retorna false nos dois casos. Muito provavelmente, um desenvolvedor queria que um caso retornasse verdadeiro, caso contrário , o método não faz nenhum sentido.

Verificações inúteis

A expressão V6007 '! Mask [i] .equals (ipAddress [i])' é sempre verdadeira. NetUtils.java (476)

 public static boolean matchIpRange(....) throws UnknownHostException { .... for (int i = 0; i < mask.length; i++) { if ("*".equals(mask[i]) || mask[i].equals(ipAddress[i])) { continue; } else if (mask[i].contains("-")) { .... } else if (....) { continue; } else if (!mask[i].equals(ipAddress[i])) { // <= return false; } } return true; } 

Na condição if-else-if, a verificação "*" é igual a (mask [i]) || A máscara [i] .equals (endereço_IP [i]) é executada. Se a condição não for atendida, o próximo check-in se-mais-se ocorrer, mostra que a máscara [i] e o endereço IP [i] não são iguais. Mas, em uma das seguintes verificações, verifique se if [else ] e mask [i] e ipAddress [i] não são iguais. Como mask [i] e ipAddress [i ] não recebem nenhum valor, a segunda verificação não faz sentido.

V6007 A expressão 'message.length> 0' sempre é verdadeira. DeprecatedTelnetCodec.java (302)

V6007 A expressão 'message! = Null' sempre é verdadeira. DeprecatedTelnetCodec.java (302)

 protected Object decode(.... , byte[] message) throws IOException { .... if (message == null || message.length == 0) { return NEED_MORE_INPUT; } .... // Here the variable <i>message </i> doesn't change! .... if (....) { String value = history.get(index); if (value != null) { byte[] b1 = value.getBytes(); if (message != null && message.length > 0) { // <= byte[] b2 = new byte[b1.length + message.length]; System.arraycopy(b1, 0, b2, 0, b1.length); System.arraycopy(message, 0, b2, b1.length, message.length); message = b2; } else { message = b1; } } } .... } 

A mensagem de verificação ! = Nulo && message.length> 0 na linha 302 é redundante. Antes da verificação na linha 302, a seguinte verificação é realizada:

 if (message == null || message.length == 0) { return NEED_MORE_INPUT; } 

Se a condição da verificação não atender, saberemos que a mensagem não é nula e seu comprimento não é igual a 0. Daqui resulta que seu comprimento é maior que 0 (como o comprimento de uma string não pode ser um número negativo). A mensagem da variável local não recebe nenhum valor antes da linha 302, o que significa que na linha 302 o valor da variável da mensagem é o mesmo que no código acima. Pode-se concluir que a expressão message! = Null && message.length> 0 sempre será verdadeira, mas o código no bloco else nunca será executado.

Definindo o valor de um campo de referência não inicializado

A expressão V6007 '! ShouldExport ()' é sempre falsa. ServiceConfig.java (371)

 public synchronized void export() { checkAndUpdateSubConfigs(); if (!shouldExport()) { // <= return; } if (shouldDelay()) { .... } else { doExport(); } 

O método shouldExport da classe ServiceConfig chama o método getExport , definido na mesma classe.

 private boolean shouldExport() { Boolean export = getExport(); // default value is true return export == null ? true : export; } .... @Override public Boolean getExport() { return (export == null && provider != null) ? provider.getExport() : export; } 

O método getExport chama o método getExport da classe AbstractServiceConfig abstrata , que retorna o valor do campo de exportação do tipo Booleano . Há também um método setExport para definir o valor do campo.

 protected Boolean export; .... public Boolean getExport() { return export; } .... public void setExport(Boolean export) { this.export = export; } 

O campo de exportação é definido no código apenas pelo método setExport . O método setExport é chamado apenas no método de construção da classe AbstractServiceBuilder abstrata (estendendo AbstractServiceConfig ) apenas no caso de o campo não ser nulo.

 @Override public void build(T instance) { .... if (export != null) { instance.setExport(export); } .... } 

Como todos os campos de referência, por padrão, são inicializados por nulo e o campo de exportação não recebeu um valor, o método setExport nunca será chamado.

Como resultado, o método getExport da classe ServiceConfig , expandindo a classe AbstractServiceConfig sempre retornará nulo . O valor de retorno é usado no método shouldExport da classe ServiceConfig , portanto, o método shouldExport sempre retornará true . Devido ao retorno true, o valor da expressão ! ShouldExport () sempre será falso. Acontece que o campo de exportação da classe ServiceConfig nunca será retornado até que o seguinte código seja executado:

 if (shouldDelay()) { DELAY_EXPORT_EXECUTOR.schedule(this::doExport, getDelay(), ....); } else { doExport(); } 

Valor do parâmetro não negativo

V6009 A função 'substring' pode receber o valor '-1' enquanto se espera um valor não negativo. Argumento de inspeção: 2. AbstractEtcdClient.java (169)

 protected void createParentIfAbsent(String fixedPath) { int i = fixedPath.lastIndexOf('/'); if (i > 0) { String parentPath = fixedPath.substring(0, i); if (categories.stream().anyMatch(c -> fixedPath.endsWith(c))) { if (!checkExists(parentPath)) { this.doCreatePersistent(parentPath); } } else if (categories.stream().anyMatch(c -> parentPath.endsWith(c))) { String grandfather = parentPath .substring(0, parentPath.lastIndexOf('/')); // <= if (!checkExists(grandfather)) { this.doCreatePersistent(grandfather); } } } } 

O resultado da função lastIndexOf é passado como o segundo parâmetro para a função de substring , cujo segundo parâmetro não deve ser um número negativo, embora lastIndexOf possa retornar -1 se não encontrar o valor procurado na string. Se o segundo parâmetro do método de substring for menor que o primeiro (-1 <0), a exceção StringIndexOutOfBoundsException será lançada. Para corrigir o erro, precisamos verificar o resultado, retornado pela função lastIndexOf . Se estiver correto (pelo menos, não negativo), passe-o para a função de substring .

Contador de loop não utilizado

V6016 Acesso suspeito ao elemento do objeto 'types' por um índice constante dentro de um loop. RpcUtils.java (153)

 public static Class<?>[] getParameterTypes(Invocation invocation) { if ($INVOKE.equals(invocation.getMethodName()) && invocation.getArguments() != null && invocation.getArguments().length > 1 && invocation.getArguments()[1] instanceof String[]) { String[] types = (String[]) invocation.getArguments()[1]; if (types == null) { return new Class<?>[0]; } Class<?>[] parameterTypes = new Class<?>[types.length]; for (int i = 0; i < types.length; i++) { parameterTypes[i] = ReflectUtils.forName(types[0]); // <= } return parameterTypes; } return invocation.getParameterTypes(); } 

No loop for , um índice constante 0 é usado para acessar o elemento dos tipos de matriz. Pode ter sido feito para usar a variável i como um índice para acessar os elementos da matriz. Mas os autores deixaram isso de fora.

Inutilização inútil

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

 @Override public NextAction handleRead(FilterChainContext context) throws IOException { .... do { savedReadIndex = frame.readerIndex(); try { msg = codec.decode(channel, frame); } catch (Exception e) { previousData = ChannelBuffers.EMPTY_BUFFER; throw new IOException(e.getMessage(), e); } if (msg == Codec2.DecodeResult.NEED_MORE_INPUT) { frame.readerIndex(savedReadIndex); return context.getStopAction(); } else { if (savedReadIndex == frame.readerIndex()) { previousData = ChannelBuffers.EMPTY_BUFFER; throw new IOException("Decode without read data."); } if (msg != null) { context.setMessage(msg); return context.getInvokeAction(); } else { return context.getInvokeAction(); } } } while (frame.readable()); // <= .... } 

A expressão na condição do loop do - while (frame.readable ()) é um código inacessível, pois o método sai durante a primeira iteração do loop. Várias verificações da variável msg são executadas no corpo do loop usando if-else. Ao fazer isso, os valores if e else são retornados do método ou as exceções são lançadas. Essa é a razão pela qual o corpo do loop é executado apenas uma vez; portanto, o uso desse loop não faz sentido.

Copiar e colar no comutador

V6067 Dois ou mais ramificações de caso executam as mesmas ações. JVMUtil.java (67), JVMUtil.java (71)

 private static String getThreadDumpString(ThreadInfo threadInfo) { .... if (i == 0 && threadInfo.getLockInfo() != null) { Thread.State ts = threadInfo.getThreadState(); switch (ts) { case BLOCKED: sb.append("\t- blocked on " + threadInfo.getLockInfo()); sb.append('\n'); break; case WAITING: // <= sb.append("\t- waiting on " + threadInfo.getLockInfo()); // <= sb.append('\n'); // <= break; // <= case TIMED_WAITING: // <= sb.append("\t- waiting on " + threadInfo.getLockInfo()); // <= sb.append('\n'); // <= break; // <= default: } } .... } 

O código na opção para os casos WAITING e TIMED_WAITING contém código de copiar e colar. Se as mesmas ações tiverem que ser executadas, o código poderá ser simplificado removendo o conteúdo do bloco de maiúsculas e minúsculas WAITING . Como resultado, o mesmo código será executado para WAITING e TIMED_WAITING.

Conclusão

Qualquer pessoa interessada em usar RPC em Java, provavelmente já ouviu falar sobre o Apache Dubbo. É um projeto popular de código aberto com uma longa história e código, escrito por muitos desenvolvedores. O código deste projeto é de ótima qualidade, mas o analisador de código estático do PVS-Studio conseguiu encontrar um certo número de erros. Isso leva ao fato de que a análise estática é muito importante no desenvolvimento de projetos de médio e grande porte, não importa quão perfeito seja o seu código.

Nota Essas verificações únicas demonstram os recursos de um analisador de código estático, mas representam uma maneira completamente errada de seu uso. Mais detalhes dessa idéia são descritos aqui e aqui . Use a análise regularmente!

Obrigado aos desenvolvedores do Apache Dubbo por uma ferramenta tão maravilhosa. Espero que este artigo o ajude a melhorar o código. O artigo não descreve todos os trechos de código suspeitos; portanto, é melhor para os desenvolvedores verificarem o projeto e avaliarem os resultados.