Geekly articles weekly

Huawei Cloud: hoje está nublado no PVS-Studio

Quadro 2

Neste século, todo mundo já ouviu falar em serviços em nuvem. Muitas empresas dominaram esse segmento de mercado e criaram seus serviços em nuvem em várias direções. Nossa equipe também se interessou recentemente por esses serviços em termos de integração do analisador de código PVS-Studio com eles. Acreditamos que nossos leitores regulares já adivinhem que tipo de projeto verificaremos neste momento. A escolha recaiu sobre o código de serviços em nuvem da Huawei.

1. Introdução


Se você está acompanhando a equipe do PVS-Studio, provavelmente percebeu que recentemente estivemos muito interessados ​​em tecnologias em nuvem. Já publicamos vários artigos relacionados a este tópico:


E assim, quando eu estava escolhendo outro projeto interessante para o próximo artigo, recebi uma oferta de emprego da Huawei pelo correio. Ao coletar informações sobre essa empresa, eles têm seus próprios serviços em nuvem, mas o principal é que o código fonte desses serviços está disponível gratuitamente no GitHub. Esse foi o principal motivo para a escolha desta empresa para este artigo. Como um sábio chinês disse: "Acidentes não são acidentais".

Agora um pouco sobre o nosso analisador. O PVS-Studio é um analisador de código estático que identifica erros e vulnerabilidades no código fonte dos programas escritos em C, C ++, C # e Java. O analisador funciona nas plataformas 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 integrar-se ao SonarQube e Jenkins:


Análise do projeto


No processo de busca de informações para o artigo, descobri que a Huawei possui um centro de desenvolvedores onde você pode obter informações, guias e o código fonte dos serviços em nuvem. Uma variedade de linguagens de programação foi usada para criar esses serviços, mas linguagens como Go, Java e Python acabaram sendo as mais populares.

Desde que me especializei em Java, os projetos foram escolhidos de acordo. As fontes dos projetos analisados ​​no artigo podem ser obtidas no repositório GitHub huaweicloud .

Para analisar os projetos, eu precisava executar apenas algumas ações:

  • Obtenha projetos do repositório;
  • Use as instruções para iniciar o analisador Java e inicie a análise em cada um dos projetos.

Após analisar os projetos, conseguimos destacar apenas três aos quais gostaria de prestar atenção. Isso se deve ao fato de o tamanho dos demais projetos Java ser muito pequeno.

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


Existem poucos avisos, portanto, em geral, podemos dizer sobre a boa qualidade do código. Além disso, nem todas as operações são erros válidos. Isso se deve ao fato de o analisador às vezes não possuir informações suficientes para distinguir o código correto do incorreto. Portanto, os diagnósticos do analisador são aprimorados todos os dias com a ajuda das informações recebidas dos usuários. Veja também o artigo " Como e por que os analisadores estáticos combatem falsos positivos ".

No processo de análise de projetos, selecionei os avisos mais interessantes que serão discutidos neste artigo.

Ordem de inicialização do campo


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 ocorrer 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, o LOG no momento da inicialização do campo INSTANCE ainda não foi inicializado. Portanto, ao registrar informações de exceção no construtor, uma NullPointerException será lançada. Essa exceção é a causa de outra exceção ExceptionInInitializerError lançada se uma exceção ocorreu ao inicializar o campo estático. Tudo o que é necessário para resolver o problema descrito é colocar a inicialização do LOG antes da inicialização do INSTANCE .

Erro de digitação invisível


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 por um caractere. O programador nomeou os argumentos desses métodos de acordo com o nome do método. Como resultado, na linha apontada pelo analisador, 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 banal relacionado ao descuido, devido ao qual a atribuição do argumento suspende a si mesma. Como resultado, o valor do argumento chegado não será atribuído ao campo de suspensão , conforme implícito. Corretamente:

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

Condições predeterminadas


Como costuma acontecer, a regra do V6007 é antecipada em termos de número 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 com o método checkNotNull , você pode ter 100% de certeza de que o argumento passado para esse método não é nulo . Como os dois argumentos do método removeFirewallRuleFromPolicy são verificados pelo método checkNotNull , não faz sentido verificar os argumentos para valores nulos novamente. No entanto, uma expressão é passada para o método checkState como o primeiro argumento, onde os argumentos firewallPolicyId e firewallRuleId são verificados novamente quanto a nulo .

Um aviso semelhante é emitido para firewallRuleId :

  • 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 sai e retorna um valor. Portanto, o teste apontado pelo analisador sempre será verdadeiro, o que significa que esse teste não faz sentido.

Semelhante ocorre em 13 classes:

  • 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, quando o método add é chamado, o campo m.blockDeviceMapping lança uma NullPointerException .

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 resultado do método estático FileId.get (path) é passado para o construtor da classe TrackedFile como o 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é que seja usado pela primeira vez:

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

Portanto, se nulo for passado para o método como o terceiro argumento, ocorrerá uma exceção.

Uma situação semelhante ocorre em outro caso:

  • 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 ; } } .... }

E novamente NPE. Várias verificações na instrução condicional permitem que o objeto dataTmpFile seja nulo para desreferenciamento adicional. Acho que existem dois erros de digitação aqui, e a verificação deve ser 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("/")); }

Entende-se que a URL é passada para esse método como uma sequência, que não é validada de forma alguma. Depois que essa string é aparada várias vezes usando o método lastIndexOf . Se lastIndexOf não encontrar uma correspondência na sequência, ele retornará -1. Isso lançará uma StringIndexOutOfBoundsException , pois os argumentos para o método de substring devem ser números não negativos. Para que o método funcione corretamente, você deve adicionar a validação do argumento de entrada ou verificar se os resultados dos métodos lastIndexOf não são números negativos.

Aqui estão alguns outros lugares onde isso acontece:

  • 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, não foi levado em consideração que a chamada do método concat não alterará a sequência do host devido à imutabilidade dos objetos do tipo String . Para que o método funcione corretamente, você deve atribuir o resultado do método concat à variável do host no bloco if . Corretamente:

 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 a inicialização. Provavelmente, a variável url deve ser passada para o método uri como o segundo argumento em vez de functionUrn , pois a variável functionUrn está envolvida na 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, eles esqueceram de usar o argumento returnType e atribuíram seu valor ao campo returnType . Portanto, ao chamar o método getReturnType , o objeto criado por esse construtor retornará o valor padrão null , embora provavelmente tenha a intenção de obter o objeto que foi 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); } .... }

A presença de 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 executam as ações opostas. De fato, ambos os métodos fazem a mesma coisa - eles criam e retornam um objeto ServiceAction . Provavelmente, o método de desativação foi criado copiando o código do método de habilitação , mas esqueceu de alterar o corpo do método.

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, decidimos verificar o conteúdo de uma estrutura do tipo Mapa quanto à desigualdade nula . Para fazer isso, o argumento params chama o método get duas vezes, cujo resultado é passado para o método checkNotNull . Tudo parece lógico, mas não importa como! Se , o argumento params é verificado como nulo . Depois disso, pode-se supor que o argumento de entrada possa ser nulo , mas antes dessa verificação, o método get já era chamado nos parâmetros duas vezes. Se null for passado como argumento para esse método, na primeira vez que o método get for chamado, uma exceção será lançada.

Uma situação semelhante ocorre em mais três 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


Hoje, as grandes empresas modernas simplesmente não podem prescindir do uso de serviços em nuvem. Um grande número de pessoas usa esses serviços; portanto, mesmo o menor erro no serviço pode levar a problemas para muitas pessoas, bem como a custos adicionais para a empresa corrigir as conseqüências desse erro. No desenvolvimento, é sempre necessário levar em consideração o fator humano, pois qualquer pessoa, mais cedo ou mais tarde, comete erros, e este artigo é um exemplo disso. É por isso que você deve usar todas as ferramentas possíveis para melhorar a qualidade do código.

O PVS-Studio certamente informará a Huawei sobre os resultados da verificação de seus serviços em nuvem para que os desenvolvedores desta empresa possam estudá-los com mais detalhes.

O uso único da análise de código estático demonstrada no artigo não pode mostrar todas as suas vantagens. Informações mais detalhadas sobre como usar a análise estática corretamente podem ser encontradas aqui e aqui . Você pode baixar o analisador PVS-Studio aqui .



Se você deseja compartilhar este artigo com um público que fala inglês, use o link para a tradução: Valery Komarov. Huawei Cloud: Hoje está nublado no PVS-Studio .

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

More articles:


All Articles