Au cours de ce siècle, tout le monde a entendu parler des services cloud. De nombreuses entreprises ont maîtrisé ce segment de marché et créé leurs services cloud dans différentes directions. Notre équipe s'est également récemment intéressée à ces services en termes d'intégration de l'analyseur de code PVS-Studio avec eux. Nous pensons que nos lecteurs réguliers devinent déjà 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 l'équipe PVS-Studio, vous avez probablement remarqué que récemment nous nous sommes beaucoup intéressés aux technologies cloud. Nous avons déjà publié plusieurs articles liés à ce sujet:
Et donc, quand je prenais un autre projet intéressant pour le prochain article, j'ai reçu une offre d'emploi de
Huawei par la poste. Lors de la collecte d'informations sur cette société, 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 gratuitement 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."
Maintenant, parlons un peu de notre analyseur. PVS-Studio est un analyseur de code statique qui identifie les erreurs et les vulnérabilités dans le code source des programmes écrits en C, C ++, C # et Java. L'analyseur fonctionne sur les plateformes Windows, Linux et macOS. En plus des plugins pour des environnements de développement classiques tels que Visual Studio ou IntelliJ IDEA, l'analyseur a la capacité de s'intégrer avec SonarQube et Jenkins:
Analyse de projet
Au cours de la recherche d'informations pour l'article, j'ai découvert que Huawei dispose d'un
centre de développement où vous pouvez obtenir des informations, des guides et le code source de leurs services cloud. Divers langages de programmation ont été utilisés pour créer ces services, mais des langages tels que Go, Java et Python se sont avérés être les plus populaires.
Comme je suis spécialisé en Java, les projets ont été choisis en conséquence. Les sources des projets analysés dans l'article peuvent être obtenues dans le référentiel GitHub
huaweicloud .
Pour analyser les projets, je n'avais besoin d'effectuer que quelques actions:
- Obtenez des projets à partir du référentiel;
- Utilisez les instructions pour démarrer l'analyseur Java et démarrer l'analyse sur chacun des projets.
Après avoir analysé les projets, nous avons réussi à n'en distinguer que trois auxquels je voudrais prêter attention. Cela est dû au fait que la taille des projets Java restants était trop petite.
Résultats de l'analyse du projet (nombre d'avertissements et nombre de fichiers):
Il y a peu d'avertissements, donc en général on peut dire de la bonne qualité du code. De plus, toutes les opérations ne sont pas des erreurs valides. Cela est dû au fait que l'analyseur n'a parfois pas suffisamment d'informations pour distinguer le code correct du code incorrect. Par conséquent, les diagnostics de l'analyseur sont améliorés chaque jour à l'aide des informations reçues des utilisateurs. Voir également l'article "
Comment et pourquoi les analyseurs statiques combattent les faux positifs ".
Dans le processus d'analyse des projets, j'ai sélectionné les avertissements les plus intéressants, 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);
Si une exception se produit dans le constructeur de la 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, le
LOG au moment de l'initialisation du champ
INSTANCE n'est pas encore initialisé. Par conséquent, lors de la journalisation des informations d'exception dans le constructeur, une exception
NullPointerException sera levée. Cette exception est la cause d'une autre
exception ExceptionInInitializerError qui est levée si une exception s'est produite lors de l'initialisation du champ statique. Tout ce qui est nécessaire pour résoudre le problème décrit est de placer l'initialisation
LOG avant l'initialisation
INSTANCE .
Faute de frappe invisible
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;
Les deux méthodes définissent le champ
metricSchema , mais les noms de méthode diffèrent par leur caractère. Le programmeur a nommé les arguments de ces méthodes en fonction du nom de la méthode. Par conséquent, dans la ligne pointée par l'analyseur, le champ
metricSchema est assigné à 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 banale liée à la négligence, à cause de laquelle l'affectation de l'argument
suspendre à lui-même se produit. Par conséquent, la valeur de l'argument arrivé ne sera pas affectée au champ de
suspension , comme implicite. Correctement:
public void setSuspend(boolean suspend) { this.suspend = suspend; }
Conditions prédéterminées
Comme cela arrive souvent, la règle
V6007 prend de l' avance en termes de nombre 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 avec la méthode
checkNotNull , vous pouvez être sûr à 100% que l'argument passé à cette méthode n'est pas
nul . Étant donné que les deux arguments de la méthode
removeFirewallRuleFromPolicy sont vérifiés par la méthode
checkNotNull , il est inutile de vérifier à nouveau les arguments pour
les valeurs nulles . Cependant, une expression est passée à la méthode
checkState comme premier argument, où les arguments
firewallPolicyId et
firewallRuleId sont revérifiés pour
null .
Un avertissement similaire est é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) {
Dans cette méthode, si l'argument
filteringParams est
nul , la méthode se ferme et renvoie une valeur. Par conséquent, le test pointé par l'analyseur sera toujours vrai, ce qui signifie que ce test n'a pas de sens.
La même chose se produit dans 13 classes:
- 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);
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 lorsque la méthode
add est appelée, le champ
m.blockDeviceMapping lèvera une
NullPointerException .
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 résultat de la méthode statique
FileId.get (chemin) est transmis au constructeur de la classe
TrackedFile 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é par
ce biais, l'argument
id ne change pas tant qu'il n'est pas utilisé pour la première fois:
public TrackedFile(...., ...., FileId id, ....) throws IOException { .... FileId newId = FileId.get(path); if (!id.equals(newId)) { .... } }
Par conséquent, si
null est passé à la méthode comme troisième argument, une exception se produit.
Une situation similaire se produit dans un autre cas:
- 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();
Et encore NPE. Un certain nombre de vérifications dans l'instruction conditionnelle permettent à l'objet
dataTmpFile d'être null pour un déréférencement ultérieur. Je pense qu'il y a deux fautes de frappe ici, et le chèque devrait en fait être comme 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("/")); }
Il est entendu que l'URL est transmise à cette méthode sous forme de chaîne, qui n'est en aucun cas validée. Après cette chaîne est découpée plusieurs fois à l'aide de la méthode
lastIndexOf . Si
lastIndexOf ne trouve pas de correspondance dans la chaîne, il renverra -1. Cela
lèvera une
StringIndexOutOfBoundsException , car les arguments de la méthode de
sous -
chaîne doivent être des nombres non négatifs. Pour que la méthode fonctionne correctement, vous devez ajouter la validation de l'argument d'entrée ou vérifier que les résultats des méthodes
lastIndexOf ne sont pas des nombres négatifs.
Voici d'autres endroits où cela se produit:
- 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, il n'a pas été pris en compte que l'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 que la méthode fonctionne correctement, vous devez affecter le résultat de la méthode
concat à la variable
hôte dans le bloc
if . Correctement:
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 l'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 est impliquée dans l'initialisation de la variable
url .
Argument non utilisé dans 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)
Dans ce constructeur, ils ont oublié d'utiliser l'argument
returnType et d'assigner sa valeur au champ
returnType . Par conséquent, lorsque la méthode
getReturnType est
appelée , l'objet créé par ce constructeur renverra la valeur par défaut
null , bien qu'il était très probablement destiné à obtenir l'objet qui a été 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) {
La présence de 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 effectuent les actions opposées. En fait, les deux méthodes font la même chose: elles créent et renvoient un objet
ServiceAction . Très probablement, la méthode
disable a été créée en copiant le code de la méthode
enable , mais a oublié de changer le corps de la méthode.
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) {
Dans cette méthode, nous avons décidé de vérifier le contenu d'une structure de type
Map pour l'inégalité
nulle . Pour ce faire, l'argument
params appelle deux fois la méthode
get , dont le résultat est passé à la méthode
checkNotNull . Tout semble logique, mais peu importe comment! Dans
if , l'argument
params est vérifié pour
null . Après quoi, on peut supposer que l'argument d'entrée peut être
nul , mais avant cette vérification, la méthode
get était déjà appelée deux fois sur les
paramètres . Si
null est passé comme argument à cette méthode, la première fois que la méthode
get est appelée, 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
De nos jours, les grandes entreprises modernes ne peuvent tout simplement pas se passer de l'utilisation des services cloud. Un grand nombre de personnes utilisent ces services, donc même la moindre erreur dans le service peut entraîner des problèmes pour de nombreuses personnes, ainsi que des coûts supplémentaires pour l'entreprise afin de corriger les conséquences de cette erreur. Lors du développement, il est toujours nécessaire de prendre en compte le facteur humain, car toute personne fait tôt ou tard des erreurs, et cet article en est un exemple. C'est pourquoi vous devez utiliser tous les outils possibles pour améliorer la qualité du code.
PVS-Studio informera certainement Huawei des résultats de la vérification de leurs services cloud afin que les développeurs de cette société puissent les étudier plus en détail.
L'utilisation ponctuelle de l'analyse de code statique démontrée dans l'article ne peut pas montrer tous ses avantages. Des informations plus détaillées sur la façon d'utiliser correctement l'analyse statique peuvent être trouvées
ici et
ici . Vous pouvez télécharger l'analyseur PVS-Studio
ici .
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Valery Komarov.
Huawei Cloud: c'est nuageux dans PVS-Studio aujourd'hui .