Geekly articles weekly

Huawei Cloud: Hoje está nublado no PVS-Studio

Quadro 2

Hoje em dia todo mundo sabe sobre serviços em nuvem. Muitas empresas quebraram esse segmento de mercado e criaram seus próprios serviços em nuvem de vários propósitos. Recentemente, nossa equipe também se interessou por esses serviços em termos de integração do analisador de código PVS-Studio. Provavelmente, nossos leitores regulares já adivinharam que tipo de projeto verificaremos neste momento. A escolha recaiu no código dos serviços em nuvem da Huawei.

1. Introdução


Se você está acompanhando as postagens da equipe do PVS-Studio, provavelmente já percebeu que estávamos pesquisando profundamente as tecnologias na nuvem ultimamente. Já publicamos vários artigos sobre este tópico:


Bem quando eu estava procurando por um projeto incomum para o próximo artigo, recebi um email com uma oferta de emprego da Huawei . Após coletar algumas informações sobre essa empresa, descobriu-se que eles tinham seus próprios serviços em nuvem, mas o principal é que o código fonte desses serviços está disponível no GitHub. Esse foi o principal motivo para a escolha desta empresa para este artigo. Como disse um sábio chinês: "Os acidentes não são acidentais".

Deixe-me dar alguns detalhes sobre o nosso analisador. O PVS-Studio é um analisador estático para detecção de erros no código fonte dos programas, escrito em C, C ++, C # e Java. O analisador funciona no Windows, Linux e macOS. Além dos plugins para ambientes clássicos de desenvolvimento, como o Visual Studio ou o IntelliJ IDEA, o analisador tem a capacidade de se integrar ao SonarQube e Jenkins:


Análise do projeto


Quando eu estava pesquisando o artigo, descobri que a Huawei tinha um centro de desenvolvedores com informações, manuais e fontes disponíveis de seus serviços em nuvem. Uma grande variedade de linguagens de programação foi usada para criar esses serviços, mas linguagens como Go, Java e Python foram as mais prevalecentes.

Desde que me especializei em Java, os projetos foram selecionados de acordo com meus conhecimentos e habilidades. Você pode analisar as fontes do projeto no artigo em um repositório do GitHub huaweicloud .

Para analisar projetos, eu precisava apenas de algumas coisas:

  • Obtenha projetos do repositório;
  • Use as instruções de inicialização do analisador Java e execute a análise em cada projeto.

Após analisar os projetos, selecionamos apenas três deles, sobre os quais gostaríamos de prestar atenção. É por causa do fato de que o tamanho dos demais projetos Java acabou sendo muito pequeno.

Resultados da análise do projeto (número de avisos e número de arquivos):


Havia poucos avisos, o que nos diz sobre a alta qualidade do código, ainda mais porque nem todos os avisos apontam para erros reais. Isso se deve ao fato de o analisador às vezes não ter informações para distinguir o código correto do incorreto. Por esse motivo, ajustamos os diagnósticos do analisador diariamente, recorrendo às informações dos usuários. Você pode ver o artigo " A maneira como os analisadores estáticos combatem os falsos positivos e por que eles fazem isso ".

Ao analisar o projeto, escolhi apenas os avisos mais importantes, sobre os quais falarei neste artigo.

Ordem de inicialização dos campos


O ciclo de inicialização da classe V6050 está presente. A inicialização de 'INSTANCE' aparece antes da inicialização de 'LOG'. UntrustedSSL.java (32), UntrustedSSL.java (59), UntrustedSSL.java (33)

public class UntrustedSSL { private static final UntrustedSSL INSTANCE = new UntrustedSSL(); private static final Logger LOG = LoggerFactory.getLogger(UntrustedSSL.class); .... private UntrustedSSL() { try { .... } catch (Throwable t) { LOG.error(t.getMessage(), t); // <= } } }

Se houver alguma exceção no construtor da classe UntrustedSSL , as informações sobre essa exceção serão registradas no bloco catch usando o log do LOG . No entanto, devido à ordem de inicialização dos campos estáticos, no momento da inicialização do campo INSTANCE , o LOG ainda não foi inicializado. Portanto, se você registrar informações sobre a exceção no construtor, isso resultará em NullPointerException . Essa exceção é o motivo de outra exceção ExceptionInInitializerError , lançada se houvesse uma exceção quando o campo estático foi inicializado. O que você precisa para resolver esse problema é colocar a inicialização do LOG antes da inicialização do INSTANCE .

Erro de digitação discreto


V6005 A variável 'this.metricSchema' é atribuída a si mesma. OpenTSDBSchema.java (72)

 public class OpenTSDBSchema { @JsonProperty("metric") private List<SchemaField> metricSchema; .... public void setMetricsSchema(List<SchemaField> metricsSchema) { this.metricSchema = metricSchema; // <= } public void setMetricSchema(List<SchemaField> metricSchema) { this.metricSchema = metricSchema; } .... }

Ambos os métodos definem o campo metricSchema , mas os nomes dos métodos diferem pelo símbolo de um. O programador nomeou os argumentos desses métodos de acordo com o nome do método. Como resultado, na linha em que o analisador aponta, o campo metricSchema é atribuído a si mesmo e o argumento do método metricsSchema não é usado.

V6005 A variável 'suspensão' é atribuída a si mesma. SuspendTransferTaskRequest.java (77)

 public class SuspendTransferTaskRequest { .... private boolean suspend; .... public void setSuspend(boolean suspend) { suspend = suspend; } .... }

Aqui está um erro trivial relacionado ao descuido, pelo qual o argumento de suspensão é atribuído a si mesmo. Como resultado, o campo de suspensão não receberá o valor do argumento obtido, conforme implícito. A versão correta:

 public void setSuspend(boolean suspend) { this.suspend = suspend; }

Pré-determinação de condições


Como costuma acontecer, a regra do V6007 avança em termos de quantidade de avisos.

A expressão V6007 'firewallPolicyId == null' sempre é falsa. FirewallPolicyServiceImpl.java (125)

 public FirewallPolicy removeFirewallRuleFromPolicy(String firewallPolicyId, String firewallRuleId) { checkNotNull(firewallPolicyId); checkNotNull(firewallRuleId); checkState(!(firewallPolicyId == null && firewallRuleId == null), "Either a Firewall Policy or Firewall Rule identifier must be set"); .... }

Nesse método, os argumentos são verificados como nulos pelo método checkNotNull :

 @CanIgnoreReturnValue public static <T> T checkNotNull(T reference) { if (reference == null) { throw new NullPointerException(); } else { return reference; } }

Após verificar o argumento pelo método checkNotNull , você pode ter 100% de certeza de que o argumento passado para esse método não é igual a nulo . Como os dois argumentos do método removeFirewallRuleFromPolicy são verificados pelo método checkNotNull , a verificação adicional de nulo não faz sentido. No entanto, a expressão em que os argumentos firewallPolicyId e firewallRuleId são verificados novamente como nulos é passada como o primeiro argumento para o método checkState .

Um aviso semelhante é emitido para firewallRuleId também:

  • A expressão V6007 'firewallRuleId == null' sempre é falsa. FirewallPolicyServiceImpl.java (125)

A expressão V6007 'filteringParams! = Null' sempre é verdadeira. NetworkPolicyServiceImpl.java (60)

 private Invocation<NetworkServicePolicies> buildInvocation(Map<String, String> filteringParams) { .... if (filteringParams == null) { return servicePoliciesInvocation; } if (filteringParams != null) { // <= .... } return servicePoliciesInvocation; }

Nesse método, se o argumento filteringParams for nulo , o método retornará um valor. É por isso que a verificação apontada pelo analisador sempre será verdadeira, o que, por sua vez, significa que essa verificação não faz sentido.

Mais 13 classes são semelhantes:

  • A expressão V6007 'filteringParams! = Null' sempre é verdadeira. PolicyRuleServiceImpl.java (58)
  • A expressão V6007 'filteringParams! = Null' sempre é verdadeira. GroupServiceImpl.java (58)
  • A expressão V6007 'filteringParams! = Null' sempre é verdadeira. ExternalSegmentServiceImpl.java (57)
  • A expressão V6007 'filteringParams! = Null' sempre é verdadeira. L3policyServiceImpl.java (57)
  • A expressão V6007 'filteringParams! = Null' sempre é verdadeira. PolicyRuleSetServiceImpl.java (58)
  • e assim por diante ...

Referência nula


V6008 Desreferência nula potencial de 'm.blockDeviceMapping'. NovaServerCreate.java (390)

 @Override public ServerCreateBuilder blockDevice(BlockDeviceMappingCreate blockDevice) { if (blockDevice != null && m.blockDeviceMapping == null) { m.blockDeviceMapping = Lists.newArrayList(); } m.blockDeviceMapping.add(blockDevice); // <= return this; }

Nesse método, a inicialização do campo de referência m.blockDeviceMapping não ocorrerá se o argumento blockDevice for nulo . Este campo é inicializado apenas neste método, portanto, ao chamar o método add do campo m.blockDeviceMapping , uma NullPointerException ocorrerá.

V6008 Desreferência nula potencial de 'FileId.get (caminho)' na função '<init>'. TrackedFile.java (140), TrackedFile.java (115)

 public TrackedFile(FileFlow<?> flow, Path path) throws IOException { this(flow, path, FileId.get(path), ....); }

O construtor da classe TrackedFile recebe o resultado do método estático FileId.get (path) como um terceiro argumento. Mas esse método pode retornar nulo :

 public static FileId get(Path file) throws IOException { if (!Files.exists(file)) { return null; } .... }

No construtor, chamado por meio disso , o argumento id não muda até seu primeiro uso:

 public TrackedFile(...., ...., FileId id, ....) throws IOException { .... FileId newId = FileId.get(path); if (!id.equals(newId)) { .... } }

Como podemos ver, se null for passado como o terceiro argumento para o método, ocorrerá uma exceção.

Aqui está outro caso semelhante:

  • V6008 Desreferência nula potencial de 'buffer'. PublishingQueue.java (518)

V6008 Dereferência nula potencial de 'dataTmpFile'. CacheManager.java (91)

 @Override public void putToCache(PutRecordsRequest putRecordsRequest) { .... if (dataTmpFile == null || !dataTmpFile.exists()) { try { dataTmpFile.createNewFile(); // <= } catch (IOException e) { LOGGER.error("Failed to create cache tmp file, return.", e); return ; } } .... }

NPE novamente. Um número de verificações no operador condicional permite que o objeto zero dataTmpFile faça uma desreferência adicional. Eu acho que existem dois erros de digitação aqui e a verificação deve ficar assim:

 if (dataTmpFile != null && !dataTmpFile.exists())

Substrings e números negativos


V6009 A função 'substring' pode receber o valor '-1' enquanto se espera um valor não negativo. Inspecione o argumento: 2. RemoveVersionProjectIdFromURL.java (37)

 @Override public String apply(String url) { String urlRmovePojectId = url.substring(0, url.lastIndexOf("/")); return urlRmovePojectId.substring(0, urlRmovePojectId.lastIndexOf("/")); }

A implicação é que esse método obtém uma URL como uma sequência, que não é validada de forma alguma. Posteriormente, essa cadeia é cortada várias vezes usando o método lastIndexOf . Se o método lastIndexOf não encontrar uma correspondência na sequência, ele retornará -1. Isso levará a StringIndexOutOfBoundsException , pois os argumentos do método de substring precisam ser números não negativos. Para a operação correta do método, é necessário adicionar uma validação de argumento de entrada ou verificar se os resultados do método lastIndexOf são números não negativos.

Aqui estão alguns outros trechos com uma maneira semelhante:

  • V6009 A função 'substring' pode receber o valor '-1' enquanto se espera um valor não negativo. Inspecione o argumento: 2. RemoveProjectIdFromURL.java (37)
  • V6009 A função 'substring' pode receber o valor '-1' enquanto se espera um valor não negativo. Inspecione o argumento: 2. RemoveVersionProjectIdFromURL.java (38)

Resultado esquecido


V6010 O valor de retorno da função 'concat' deve ser utilizado. AKSK.java (278)

 public static String buildCanonicalHost(URL url) { String host = url.getHost(); int port = url.getPort(); if (port > -1) { host.concat(":" + Integer.toString(port)); } return host; }

Ao escrever esse código, seu autor não levou em consideração que uma chamada do método concat não alteraria a string do host devido à imutabilidade dos objetos do tipo String . Para a operação correta do método, o resultado do método concat deve ser atribuído à variável host no bloco if . A versão correta:

 if (port > -1) { host = host.concat(":" + Integer.toString(port)); }

Variáveis ​​não utilizadas


V6021 A variável 'url' não é usada. TriggerV2Service.java (95)

 public ActionResponse deleteAllTriggersForFunction(String functionUrn) { checkArgument(!Strings.isNullOrEmpty(functionUrn), ....); String url = ClientConstants.FGS_TRIGGERS_V2 + ClientConstants.URI_SEP + functionUrn; return deleteWithResponse(uri(triggersUrlFmt, functionUrn)).execute(); }

Nesse método, a variável url não é usada após sua inicialização. Provavelmente, a variável url precisa ser passada para o método uri como um segundo argumento em vez de functionUrn , pois a variável functionUrn participa da inicialização da variável url .

Argumento não usado no construtor


V6022 O parâmetro 'returnType' não é usado dentro do corpo do construtor. HttpRequest.java (68)

 public class HttpReQuest<R> { .... Class<R> returnType; .... public HttpRequest(...., Class<R> returnType) // <= { this.endpoint = endpoint; this.path = path; this.method = method; this.entity = entity; } .... public Class<R> getReturnType() { return returnType; } .... }

Nesse construtor, o programador esqueceu de usar o argumento returnType e atribuiu seu valor ao campo returnType . É por isso que, ao chamar o método getReturnType a partir do objeto criado por esse construtor, null será retornado por padrão. Mas o mais provável é que o programador pretendesse obter o objeto, passado anteriormente ao construtor.

Mesma funcionalidade


V6032 É estranho que o corpo do método 'enable' seja totalmente equivalente ao corpo de outro método 'disable'. ServiceAction.java (32), ServiceAction.java (36)

 public class ServiceAction implements ModelEntity { private String binary; private String host; private ServiceAction(String binary, String host) { this.binary = binary; this.host = host; } public static ServiceAction enable(String binary, String host) { // <= return new ServiceAction(binary, host); } public static ServiceAction disable(String binary, String host) { // <= return new ServiceAction(binary, host); } .... }

Ter dois métodos idênticos não é um erro, mas o fato de dois métodos executarem a mesma ação é pelo menos estranho. Observando os nomes dos métodos acima, podemos assumir que eles devem executar as ações opostas. De fato, os dois métodos fazem a mesma coisa - crie e retorne o objeto ServiceAction . Provavelmente, o método desativar foi criado copiando o código do método enable , mas o corpo do método permaneceu o mesmo.

Esqueceu de verificar o principal


V6060 A referência 'params' foi utilizada antes de ser verificada com relação a nulo. DomainService.java (49), DomainService.java (46)

 public Domains list(Map<String, String> params) { Preconditions.checkNotNull(params.get("page_size"), ....); Preconditions.checkNotNull(params.get("page_number"), ....); Invocation<Domains> domainInvocation = get(Domains.class, uri("/domains")); if (params != null) { // <= .... } return domainInvocation.execute(this.buildExecutionOptions(Domains.class)); }

Nesse método, o autor decidiu verificar se o conteúdo de uma estrutura do tipo Mapa é nulo . Para fazer isso, o método get é chamado duas vezes a partir do argumento params . O resultado do método get é passado para o método checkNotNull . Tudo parece lógico, mas não é assim! O argumento params é verificado como nulo em if . Depois disso, espera-se que o argumento de entrada seja nulo , mas antes dessa verificação, o método get já foi chamado duas vezes a partir dos parâmetros. Se null for passado como argumento para esse método, na primeira vez que você chamar o método get , uma exceção será lançada.

Uma situação semelhante ocorre em três outros lugares:

  • V6060 A referência 'params' foi utilizada antes de ser verificada com relação a nulo. DomainService.java (389), DomainService.java (387)
  • V6060 A referência 'params' foi utilizada antes de ser verificada com relação a nulo. DomainService.java (372), DomainService.java (369)
  • V6060 A referência 'params' foi utilizada antes de ser verificada com relação a nulo. DomainService.java (353), DomainService.java (350)

Conclusão


As grandes empresas de hoje não podem prescindir do uso de serviços em nuvem. Um grande número de pessoas usa esses serviços. Nesta visão, mesmo um pequeno erro em um serviço pode levar a problemas para muitas pessoas, bem como a perdas adicionais acumuladas por uma empresa para remediar as conseqüências adversas desse erro. Falhas humanas sempre devem ser levadas em consideração, especialmente porque mais cedo ou mais tarde todos cometem erros, conforme descrito neste artigo. Esse fato substancia o uso de todas as ferramentas possíveis para melhorar a qualidade do código.

O PVS-Studio definitivamente informará a empresa Huawei sobre os resultados da verificação de seus serviços em nuvem, para que os desenvolvedores da Huawei possam se concentrar neles, porque o uso único da análise de código estático coberto por estes artigos ( 1 , 2 ) não pode demonstrar completamente todas as suas vantagens. Você pode baixar o analisador PVS-Studio aqui .

Source: https://habr.com/ru/post/pt477558/

More articles:


All Articles