Analyse du framework RPC Apache Dubbo par l'analyseur de code statique PVS-Studio

Apache Dubbo est l'un des projets Java les plus populaires sur GitHub. Ce n'est pas surprenant. Il a été créé il y a 8 ans et est largement utilisé comme environnement RPC haute performance. Bien sûr, la plupart des bogues de son code ont depuis longtemps été corrigés et la qualité du code est maintenue à un niveau élevé. Cependant, il n'y a aucune raison de refuser de vérifier un projet aussi intéressant en utilisant l'analyseur de code statique PVS-Studio. Voyons comment cela s'est avéré.

À propos de PVS-Studio

L'analyseur de code statique PVS-Studio existe depuis plus de 10 ans sur le marché informatique et est une solution logicielle multifonctionnelle et facile à introduire. À l'heure actuelle, l'analyseur prend en charge C, C ++, C #, Java et fonctionne sur Windows, Linux et macOS.

PVS-Studio est une solution B2B et est utilisée par un grand nombre d'équipes dans diverses entreprises. Si vous souhaitez estimer les capacités de l'analyseur, vous pouvez télécharger la distribution et demander une clé d'essai ici .

Il existe également des options sur la façon dont vous pouvez utiliser PVS-Studio gratuitement dans des projets open source ou si vous êtes étudiant.

Apache Dubbo: Qu'est-ce que c'est et principales fonctionnalités

De nos jours, presque tous les grands systèmes logiciels sont distribués . Si, dans un système distribué, il existe une connexion interactive entre des composants distants à faible latence et relativement peu de données à transmettre, c'est une bonne raison d'utiliser un environnement RPC (appel de procédure distante).

Apache Dubbo est un environnement RPC (appel de procédure distante) hautes performances avec un code open source basé sur Java. Comme beaucoup d'autres systèmes RPC, dubbo est basé sur l'idée de créer un service définissant certaines méthodes qui peuvent être appelées à distance avec leurs paramètres et types de valeurs de retour. Côté serveur, une interface est mise en place et le serveur dubbo est lancé pour gérer les demandes des clients. Côté client, il existe un stub qui fournit les mêmes méthodes que le serveur. Dubbo propose trois fonctions clés, qui comprennent les appels à distance d'interface, la tolérance aux pannes et l'équilibrage de charge, ainsi que l'enregistrement automatique et la découverte des services.

À propos de l'analyse

La séquence d'actions de l'analyse est assez simple et n'a pas nécessité beaucoup de temps dans mon cas:

• Apache Dubbo téléchargé depuis GitHub ;
• Utilisé les instructions de démarrage de l'analyseur Java et exécuté l'analyse;
• A obtenu le rapport de l'analyseur, l'a examiné et a mis en évidence des cas intéressants.

Les résultats de l'analyse: 73 avertissements de niveaux de certitude élevé et moyen (46 et 27, respectivement) ont été émis pour plus de 4000 fichiers, ce qui est un bon indicateur de la qualité du code.

Tous les avertissements ne sont pas des erreurs. C'est un résultat habituel, car avant d'utiliser directement l'analyseur, il faut le configurer. Après cela, vous pouvez vous attendre à un pourcentage assez faible de faux positifs ( exemple ).

Je n'ai pas considéré 9 avertissements (7 élevés et 2 moyens), liés à des fichiers avec des tests.

Par conséquent, j'ai reçu un petit nombre d'avertissements, qui comprenaient également des faux positifs, car je n'ai pas configuré l'analyseur. Explorer les 73 avertissements avec une description plus détaillée dans l'article est long, ridicule et fastidieux, j'ai donc choisi les plus sapides.

Signer l'octet en Java

V6007 L' expression 'endKey [i] <0xff' est toujours vraie. 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); } 

Une valeur de type octet (la valeur de -128 à 127) est comparée à la valeur 0xff (255). Dans cette condition, il n'est pas pris en compte que le type d' octet en Java est signé, donc la condition sera toujours vraie, ce qui signifie qu'elle n'a pas de sens.

Retour des mêmes valeurs

V6007 L' expression 'isPreferIPV6Address ()' est toujours fausse. 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(); } 

La méthode estPreferIPV6Address .

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

La méthode isPreferIPV6Address renvoie false dans les deux cas. Très probablement, un développeur voulait qu'un cas retourne vrai, sinon la méthode n'a aucun sens.

Contrôles inutiles

V6007 L' expression «! Mask [i] .equals (ipAddress [i])» est toujours vraie. 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; } 

Dans la condition if-else-if, la coche "*". Equals (mask [i]) || mask [i] .equals (ipAddress [i]) est exécuté. Si la condition n'est pas remplie, le prochain check-in if-else-if se produit qui nous montre que mask [i] et ipAddress [i] ne sont pas égaux. Mais dans l'une des vérifications suivantes dans if-else-if, il est vérifié que mask [i] et ipAddress [i] ne sont pas égaux. Puisque mask [i] et ipAddress [i ] ne reçoivent aucune valeur, la deuxième vérification est inutile.

V6007 L' expression ' message.length > 0' est toujours vraie. DeprecatedTelnetCodec.java (302)

V6007 L' expression 'message! = Null' est toujours vraie. 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; } } } .... } 

Le message de vérification ! = Null && message.length> 0 à la ligne 302 est redondant. Avant la vérification à la ligne 302, la vérification suivante est effectuée:

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

Si la condition de la vérification ne remplit pas, nous saurons que le message n'est pas nul et sa longueur n'est pas égale à 0. Il en résulte que sa longueur est supérieure à 0 (car la longueur d'une chaîne ne peut pas être un nombre négatif). Aucune valeur n'est attribuée au message de variable locale avant la ligne 302, ce qui signifie qu'à la ligne 302, la valeur de la variable de message est la même que dans le code ci-dessus. On peut conclure que l'expression message! = Null && message.length> 0 sera toujours vraie, mais le code du bloc else ne sera jamais exécuté.

Définition de la valeur d'un champ de référence non initialisé

V6007 L' expression '! ShouldExport ()' est toujours fausse. ServiceConfig.java (371)

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

La méthode shouldExport de la classe ServiceConfig appelle la méthode getExport , définie dans la même 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; } 

La méthode getExport appelle la méthode getExport de la classe abstraite AbstractServiceConfig , qui renvoie la valeur du champ d' exportation du type booléen . Il existe également une méthode setExport pour définir la valeur du champ.

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

Le champ d'exportation est défini dans le code uniquement par la méthode setExport . La méthode setExport est appelée uniquement dans la méthode de génération de la classe abstraite AbstractServiceBuilder (étendant AbstractServiceConfig ) uniquement si le champ n'est pas nul.

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

Étant donné que tous les champs de référence par défaut sont initialisés par null et qu'aucune valeur n'a été affectée au champ d' exportation , la méthode setExport ne sera jamais appelée.

Par conséquent, la méthode getExport de la classe ServiceConfig , en développant la classe AbstractServiceConfig , renverra toujours null . La valeur de retour est utilisée dans la méthode shouldExport de la classe ServiceConfig , par conséquent, la méthode shouldExport renvoie toujours true . En raison de la valeur true, la valeur de l' expression! ShouldExport () sera toujours false. Il s'avère que le champ d' exportation de la classe ServiceConfig ne sera jamais renvoyé tant que le code suivant n'est pas exécuté:

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

Valeur du paramètre non négatif

V6009 La fonction 'substring' pourrait recevoir la valeur '-1' alors qu'une valeur non négative est attendue. Inspectez l'argument: 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); } } } } 

Le résultat de la fonction lastIndexOf est transmis en tant que deuxième paramètre à la fonction de sous - chaîne , dont le deuxième paramètre ne doit pas être un nombre négatif, même si lastIndexOf peut renvoyer -1 s'il ne trouve pas la valeur recherchée dans la chaîne. Si le deuxième paramètre de la méthode de sous - chaîne est inférieur au premier (-1 <0), l'exception StringIndexOutOfBoundsException sera levée. Pour corriger l'erreur, nous devons vérifier le résultat, renvoyé par la fonction lastIndexOf . S'il est correct (au moins, non négatif), passez-le à la fonction de sous - chaîne .

Compteur de boucle inutilisé

V6016 Accès suspect à l'élément de l'objet 'types' par un index constant à l'intérieur d'une boucle. 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(); } 

Dans la boucle for , un index constant 0 est utilisé pour accéder à l'élément des types de tableau. Il peut être destiné à utiliser la variable i comme index pour accéder aux éléments du tableau. Mais les auteurs ont omis cela.

Do-while inutile

V6019 Code inaccessible détecté. Il est possible qu'une erreur soit présente. 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()); // <= .... } 

L'expression dans l'état de la boucle do - while (frame.readable ()) est du code inaccessible, car la méthode se termine lors de la première itération de la boucle. Plusieurs vérifications de la variable msg sont effectuées dans le corps de la boucle en utilisant if-else. Ce faisant, les valeurs if et else sont renvoyées par la méthode ou des exceptions sont levées. C'est la raison pour laquelle le corps de la boucle ne s'exécute qu'une seule fois, donc l'utilisation de cette boucle n'a aucun intérêt.

Copier-coller dans le commutateur

V6067 Deux ou plusieurs branches de cas effectuent les mêmes actions. 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: } } .... } 

Le code dans le commutateur pour les cas WAITING et TIMED_WAITING contient du code copier-coller. Si les mêmes actions doivent être effectuées, le code peut être simplifié en supprimant le contenu du bloc de cas WAITING . Par conséquent, le même code sera exécuté pour WAITING et TIMED_WAITING.

Conclusion

Quiconque souhaite utiliser RPC en Java a probablement entendu parler d'Apache Dubbo. C'est un projet open source populaire avec une longue histoire et du code, écrit par de nombreux développeurs. Le code de ce projet est de grande qualité, malgré tout l'analyseur de code statique PVS-Studio a réussi à trouver un certain nombre d'erreurs. Cela conduit au fait que l'analyse statique est très importante lors du développement de projets de moyenne et grande taille, quelle que soit la perfection de votre code.

Remarque Ces vérifications ponctuelles démontrent les capacités d'un analyseur de code statique, mais représentent une manière complètement fausse de son utilisation. Plus de détails sur cette idée sont présentés ici et ici . Utilisez l'analyse régulièrement!

Merci aux développeurs d'Apache Dubbo pour un outil aussi merveilleux. J'espère que cet article vous aidera à améliorer le code. L'article ne décrit pas tous les morceaux de code suspects, il est donc préférable pour les développeurs de vérifier le projet eux-mêmes et d'évaluer les résultats.