Geekly articles weekly

Huawei Cloud: c'est nuageux dans PVS-Studio aujourd'hui

Image 2

De nos jours, tout le monde connaît les services cloud. De nombreuses entreprises ont craqué ce segment de marché et créé leurs propres services cloud à diverses fins. Récemment, notre équipe s'est également intéressée à ces services en termes d'intégration de l'analyseur de code PVS-Studio. Il y a de fortes chances que nos lecteurs réguliers aient déjà deviné quel type de projet nous allons vérifier cette fois. Le choix s'est porté sur le code des services cloud de Huawei.

Présentation


Si vous suivez les articles de l'équipe PVS-Studio, vous avez probablement remarqué que nous avions creusé profondément dans les technologies cloud ces derniers temps. Nous avons déjà publié plusieurs articles sur ce sujet:


Au moment où je cherchais un projet inhabituel pour le prochain article, j'ai reçu un e-mail avec une offre d'emploi de Huawei . Après avoir collecté des informations sur cette entreprise, il s'est avéré qu'ils avaient leurs propres services cloud, mais l'essentiel est que le code source de ces services est disponible sur GitHub. C'était la principale raison du choix de cette entreprise pour cet article. Comme l'a dit un sage chinois: "Les accidents ne sont pas accidentels".

Permettez-moi de vous donner quelques détails sur notre analyseur. PVS-Studio est un analyseur statique pour la détection de bogues dans le code source des programmes, écrit en C, C ++, C # et Java. L'analyseur fonctionne sous Windows, Linux et macOS. En plus des plugins pour les environnements de développement classiques, tels que Visual Studio ou IntelliJ IDEA, l'analyseur a la capacité de s'intégrer dans SonarQube et Jenkins:


Analyse de projet


Lorsque je faisais des recherches pour l'article, j'ai découvert que Huawei avait un centre de développement avec les informations disponibles, les manuels et les sources de leurs services cloud. Une grande variété de langages de programmation ont été utilisés pour créer ces services, mais les langages tels que Go, Java et Python étaient les plus répandus.

Depuis que je suis spécialisé en Java, les projets ont été sélectionnés en fonction de mes connaissances et compétences. Vous pouvez obtenir des sources de projet analysées dans l'article dans un référentiel GitHub huaweicloud .

Pour analyser des projets, je n'avais besoin que de quelques tâches:

  • Obtenez des projets à partir du référentiel;
  • Utilisez les instructions de démarrage de l'analyseur Java et exécutez l'analyse sur chaque projet.

Après avoir analysé les projets, nous n'en avons sélectionné que trois, auxquels nous voudrions prêter attention. C'est parce que la taille des autres projets Java s'est avérée trop petite.

Résultats de l'analyse du projet (nombre d'avertissements et nombre de fichiers):


Il y avait peu d'avertissements, ce qui nous indique une haute qualité de code, d'autant plus que tous les avertissements ne signalent pas de vraies erreurs. Cela est dû au fait que l'analyseur manque parfois d'informations pour distinguer le code correct de celui erroné. Pour cette raison, nous modifions quotidiennement les diagnostics de l'analyseur en recourant aux informations des utilisateurs. Vous pouvez consulter l'article " La façon dont les analyseurs statiques luttent contre les faux positifs, et pourquoi ils le font ."

En analysant le projet, je n'ai sélectionné que les avertissements les plus marquants, dont je parlerai dans cet article.

Ordre d'initialisation des champs


Le cycle d'initialisation de la classe V6050 est présent. L'initialisation de 'INSTANCE' apparaît avant l'initialisation 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); // <= } } }

S'il existe une exception dans le constructeur de classe UntrustedSSL , les informations sur cette exception sont enregistrées dans le bloc catch à l'aide de l'enregistreur LOG . Cependant, en raison de l'ordre d'initialisation des champs statiques, au moment de l'initialisation du champ INSTANCE , LOG n'est pas encore initialisé. Par conséquent, si vous enregistrez des informations sur l'exception dans le constructeur, cela entraînera NullPointerException . Cette exception est la raison d'une autre exception ExceptionInInitializerError , qui est levée s'il y avait eu une exception lors de l'initialisation du champ statique. Ce dont vous avez besoin pour résoudre ce problème est de placer l'initialisation de LOG avant l'initialisation d' INSTANCE .

Faute de frappe discrète


V6005 La variable 'this.metricSchema' est assignée à elle-même. 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; } .... }

Les deux méthodes définissent le champ metricSchema , mais les noms des méthodes diffèrent par le symbole "un". Le programmeur a nommé les arguments de ces méthodes en fonction du nom de la méthode. Par conséquent, dans la ligne vers laquelle pointe l'analyseur, le champ metricSchema est affecté à lui-même et l' argument de la méthode metricsSchema n'est pas utilisé.

V6005 La variable 'suspend' est assignée à elle-même. SuspendTransferTaskRequest.java (77)

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

Voici une erreur triviale liée à la négligence, à cause de laquelle l'argument de suspension est attribué à lui-même. Par conséquent, le champ suspend ne se verra pas attribuer la valeur de l'argument obtenu comme implicite. La bonne version:

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

Prédétermination des conditions


Comme cela arrive souvent, la règle V6007 prend de l' avance en termes de quantité d'avertissements.

V6007 L' expression «firewallPolicyId == null» est toujours fausse. 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"); .... }

Dans cette méthode, les arguments sont vérifiés pour null par la méthode checkNotNull :

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

Après avoir vérifié l'argument par la méthode checkNotNull , vous pouvez être sûr à 100% que l'argument passé à cette méthode n'est pas égal à null . Étant donné que les deux arguments de la méthode removeFirewallRuleFromPolicy sont vérifiés par la méthode checkNotNull , leur nouvelle vérification de null n'a aucun sens. Cependant, l'expression, où les arguments firewallPolicyId et firewallRuleId sont revérifiés pour null , est passée comme premier argument à la méthode checkState .

Un avertissement similaire est également émis pour firewallRuleId :

  • V6007 L'expression 'firewallRuleId == null' est toujours fausse. FirewallPolicyServiceImpl.java (125)

V6007 L' expression 'filteringParams! = Null' est toujours vraie. NetworkPolicyServiceImpl.java (60)

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

Dans cette méthode, si l'argument filteringParams est null , la méthode renvoie une valeur. C'est pourquoi la vérification vers laquelle l'analyseur pointe sera toujours vraie, ce qui, à son tour, signifie que cette vérification n'a pas de sens.

13 autres classes sont similaires:

  • V6007 L'expression 'filteringParams! = Null' est toujours vraie. PolicyRuleServiceImpl.java (58)
  • V6007 L'expression 'filteringParams! = Null' est toujours vraie. GroupServiceImpl.java (58)
  • V6007 L'expression 'filteringParams! = Null' est toujours vraie. ExternalSegmentServiceImpl.java (57)
  • V6007 L'expression 'filteringParams! = Null' est toujours vraie. L3policyServiceImpl.java (57)
  • V6007 L'expression 'filteringParams! = Null' est toujours vraie. PolicyRuleSetServiceImpl.java (58)
  • et ainsi de suite ...

Référence nulle


V6008 Déréférence nulle potentielle 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; }

Dans cette méthode, l'initialisation du champ de référence m.blockDeviceMapping ne se produira pas si l'argument blockDevice est nul . Ce champ est initialisé uniquement dans cette méthode, donc lors de l'appel de la méthode add à partir du champ m.blockDeviceMapping , une exception NullPointerException se produira.

V6008 Déréférence nulle potentielle de 'FileId.get (chemin)' dans la fonction '<init>'. TrackedFile.java (140), TrackedFile.java (115)

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

Le constructeur de la classe TrackedFile reçoit le résultat de la méthode FileId.get (chemin) statique comme troisième argument. Mais cette méthode peut retourner null :

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

Dans le constructeur, appelé via ceci , l'argument id ne change pas jusqu'à sa première utilisation:

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

Comme nous pouvons le voir, si null est passé comme troisième argument à la méthode, une exception se produira.

Voici un autre cas similaire:

  • V6008 Déréférence nulle potentielle de «tampon». PublishingQueue.java (518)

V6008 Déréférence nulle potentielle 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 à nouveau. Un certain nombre de vérifications dans l'opérateur conditionnel permet à l'objet zéro dataTmpFile de déréférencer davantage. Je pense qu'il y a deux fautes de frappe ici et le chèque devrait en fait ressembler à ceci:

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

Sous-chaînes et nombres négatifs


V6009 La fonction 'substring' pourrait recevoir la valeur '-1' alors qu'une valeur non négative est attendue. Inspectez l'argument: 2. RemoveVersionProjectIdFromURL.java (37)

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

L'implication est que cette méthode obtient une URL sous forme de chaîne, qui n'est en aucun cas validée. Plus tard, cette chaîne est coupée plusieurs fois à l'aide de la méthode lastIndexOf . Si la méthode lastIndexOf ne trouve pas de correspondance dans la chaîne, elle renverra -1. Cela conduira à StringIndexOutOfBoundsException , car les arguments de la méthode de sous - chaîne doivent être des nombres non négatifs. Pour un fonctionnement correct de la méthode, il faut ajouter une validation d'argument d'entrée ou vérifier que les résultats de la méthode lastIndexOf sont des nombres non négatifs.

Voici quelques autres extraits avec des choses similaires:

  • V6009 La fonction 'substring' pourrait recevoir la valeur '-1' alors qu'une valeur non négative est attendue. Inspectez l'argument: 2. RemoveProjectIdFromURL.java (37)
  • V6009 La fonction 'substring' pourrait recevoir la valeur '-1' alors qu'une valeur non négative est attendue. Inspectez l'argument: 2. RemoveVersionProjectIdFromURL.java (38)

Résultat oublié


V6010 La valeur de retour de la fonction 'concat' doit être utilisée. 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; }

Lors de l'écriture de ce code, son auteur n'a pas tenu compte du fait qu'un appel de la méthode concat ne changera pas la chaîne hôte en raison de l'immuabilité des objets de type String . Pour un fonctionnement correct de la méthode, le résultat de la méthode concat doit être affecté à la variable hôte dans le bloc if . La bonne version:

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

Variables inutilisées


V6021 L'URL variable n'est pas utilisée. 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(); }

Dans cette méthode, la variable url n'est pas utilisée après son initialisation. Très probablement, la variable url doit être passée à la méthode uri comme deuxième argument au lieu de functionUrn , car la variable functionUrn participe à l'initialisation de la variable url .

L'argument n'a pas utilisé le constructeur


V6022 Le paramètre 'returnType' n'est pas utilisé dans le corps du constructeur. 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; } .... }

Dans ce constructeur, le programmeur a oublié d'utiliser l'argument returnType et d'affecter sa valeur au champ returnType . C'est pourquoi lors de l'appel de la méthode getReturnType à partir de l'objet, créé par ce constructeur, null sera retourné par défaut. Mais très probablement, le programmeur avait l'intention d'obtenir l'objet, précédemment transmis au constructeur.

Même fonctionnalité


V6032 Il est étrange que le corps de la méthode 'enable' soit entièrement équivalent au corps d'une autre méthode '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); } .... }

Avoir deux méthodes identiques n'est pas une erreur, mais le fait que deux méthodes effectuent la même action est au moins étrange. En regardant les noms des méthodes ci-dessus, nous pouvons supposer qu'elles doivent effectuer les actions opposées. En fait, les deux méthodes font la même chose: créer et renvoyer l'objet ServiceAction . Très probablement, la méthode disable a été créée en copiant le code de la méthode enable , mais le corps de la méthode est resté le même.

J'ai oublié de vérifier la chose principale


V6060 La référence "params" a été utilisée avant d'être vérifiée par rapport à null. 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)); }

Dans cette méthode, l'auteur a décidé de vérifier le contenu d'une structure de type Map pour null . Pour ce faire, la méthode get est appelée deux fois à partir de l'argument params . Le résultat de la méthode get est transmis à la méthode checkNotNull . Tout semble logique, mais ce n'est pas comme ça! L'argument params est vérifié pour null dans if . Après cela, il est prévu que l'argument d'entrée soit nul , mais avant cette vérification, la méthode get a déjà été appelée deux fois depuis params. Si null est passé comme argument à cette méthode, la première fois que vous appelez la méthode get , une exception sera levée.

Une situation similaire se produit à trois autres endroits:

  • V6060 La référence "params" a été utilisée avant d'être vérifiée par rapport à null. DomainService.java (389), DomainService.java (387)
  • V6060 La référence "params" a été utilisée avant d'être vérifiée par rapport à null. DomainService.java (372), DomainService.java (369)
  • V6060 La référence "params" a été utilisée avant d'être vérifiée par rapport à null. DomainService.java (353), DomainService.java (350)

Conclusion


Les grandes entreprises d'aujourd'hui ne peuvent se passer de l'utilisation des services cloud. Un grand nombre de personnes utilisent ces services. De ce point de vue, même une petite erreur dans un service peut entraîner des problèmes pour de nombreuses personnes ainsi que des pertes supplémentaires, accumulées par une entreprise pour remédier aux conséquences néfastes de cette erreur. Les défauts humains doivent toujours être pris en compte, d'autant plus que tôt ou tard tout le monde fait des erreurs, comme décrit dans cet article. Ce fait justifie l'utilisation de tous les outils possibles pour améliorer la qualité du code.

PVS-Studio informera certainement la société Huawei des résultats de la vérification de leurs services cloud afin que les développeurs Huawei puissent s'y attarder, car l'utilisation ponctuelle de l'analyse de code statique couverte par ces articles ( 1 , 2 ) ne peut pas pleinement démontrer tous ses avantages. Vous pouvez télécharger l'analyseur PVS-Studio ici .

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

More articles:


All Articles