Analyse du code source RPC du framework Apache Dubbo avec l'analyseur statique PVS-Studio

Apache Dubbo est l'un des projets Java les plus populaires sur GitHub. Et ce n'est pas surprenant. Il a été créé il y a 8 ans et est largement utilisé comme environnement RPC hautes performances. Bien sûr, la plupart des erreurs dans son code sont corrigées depuis longtemps 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 ce que nous avons réussi à trouver.

À 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 à mettre en œuvre. À l'heure actuelle, l'analyseur prend en charge les langages C, C ++, C #, Java et fonctionne sur les plates-formes Windows, Linux et macOS.

PVS-Studio est une solution B2B payante et est utilisée par un grand nombre d'équipes dans diverses entreprises. Si vous souhaitez évaluer les capacités de l'analyseur, téléchargez le kit de distribution et demandez une clé d'essai ici .

Il existe également des options pour utiliser PVS-Studio gratuitement dans des projets open source ou si vous êtes étudiant.

Apache Dubbo: qu'est-ce que c'est et que mange-t-il?

Actuellement, 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 avec un temps de réponse court et une quantité relativement faible de données transmises, c'est une bonne raison d'utiliser l'environnement RPC (appel de procédure distante).

Apache Dubbo est un environnement RPC (appel de procédure distante) basé sur Java et à hautes performances. Comme avec de nombreux systèmes RPC, dubbo est basé sur l'idée de créer un service pour définir des méthodes qui peuvent être appelées à distance avec leurs paramètres et types de retour. Côté serveur, une interface est implémentée et le serveur dubbo est lancé pour traiter les appels clients. Il existe un stub côté client qui fournit les mêmes méthodes que le serveur. Dubbo offre trois fonctionnalités clés qui incluent les appels distants frontaux, la tolérance aux pannes et l'équilibrage de charge, ainsi que l'enregistrement et la découverte automatiques des services.

À propos de l'analyse

La séquence des étapes de l'analyse est assez simple et ne nécessite pas beaucoup de temps:

• Got Apache Dubbo avec GitHub ;
• J'ai utilisé les instructions pour démarrer l'analyseur Java et commencé l'analyse;
• J'ai reçu un rapport d'analyseur, je l'ai analysé et j'ai mis en évidence des cas intéressants.

Résultats de l'analyse: 73 avertissements des niveaux de confiance é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. Il s'agit d'une situation normale et avant une utilisation régulière de l'analyseur, sa configuration est requise. On peut alors s'attendre à un pourcentage assez faible de faux positifs ( exemple ).

Parmi les avertissements, 9 avertissements (7 élevés et 2 moyens) par fichier de test n'ont pas été pris en compte.

En conséquence, un petit nombre d'avertissements est resté, mais parmi eux il y avait aussi des faux positifs, car je n'ai pas configuré l'analyseur. Trier 73 avertissements dans un article est une tâche longue, idiote et fastidieuse, donc les plus intéressants ont été sélectionnés.

Octet signé 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 (une valeur de -128 à 127) est comparée à une valeur de 0xff (255). Dans cette condition, il n'est pas pris en compte que le type d' octet en Java est significatif, donc la condition sera toujours remplie, ce qui signifie qu'elle n'a pas de sens.

Renvoie la même valeur

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(); } 

Méthode IsPreferIPV6Address .

 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 que l'un des cas aurait dû renvoyer true comme prévu par le programmeur, sinon la méthode n'a pas de sens.

Des contrôles dénués de sens

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 première des conditions if-else-if, une vérification "*" se produit . Equals (mask [i]) || mask [i] .equals (ipAddress [i]) . Si cette condition n'est pas remplie, le code passe à la vérification suivante if-else-if, et nous constatons que mask [i] et ipAddress [i] ne sont pas équivalents. Mais l'une des vérifications suivantes dans if-else-if vérifie simplement que le masque [i] et ipAddress [i] ne sont pas équivalents. Étant donné que mask [i] et ipAddress [i] ne reçoivent aucune valeur dans le code de méthode, la deuxième vérification n'a pas de sens.

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; } .... //   message  ! .... 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; } } } .... } 

Vérification du message! = Null && message.length> 0 sur la ligne 302 n'a aucun sens. Avant la vérification, la ligne 302 vérifie:

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

Si la condition de vérification n'est pas remplie, alors nous saurons que le message n'est pas nul et sa longueur n'est pas 0. Il résulte de ces informations que sa longueur est supérieure à zéro (puisque la longueur de la 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 utilisée, comme dans le code ci-dessus. De tout cela, nous pouvons conclure que l'expression message! = Null && message.length> 0 sera toujours vraie , ce qui signifie que le code dans le 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 de type Boolean . Il existe également une méthode setExport pour définir la valeur d'un champ.

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

Seule la méthode setExport définit le champ d' exportation dans le code. La méthode setExport est appelée uniquement dans la méthode de génération de la classe abstraite AbstractServiceBuilder (étendant AbstractServiceConfig ) et uniquement si le champ d'exportation 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 sont initialisés à null par défaut et que le champ d' exportation n'a reçu aucune valeur n'importe où dans le code, la méthode setExport ne sera jamais appelée.

Par conséquent, la méthode getExport de la classe ServiceConfig étendant la classe AbstractServiceConfig renverra toujours null . La valeur renvoyée est utilisée dans la méthode shouldExport de la classe ServiceConfig , donc la méthode shouldExport renverra toujours true . En raison du retour de true, la valeur de l'expression ! ShouldExport () sera toujours false. Il s'avère qu'il n'y aura jamais de retour de la méthode d' exportation de la classe ServiceConfig avant l'exécution du code:

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

Valeur de paramètre non négative

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 par le deuxième paramètre à la fonction de sous - chaîne , dont le deuxième paramètre ne doit pas être un nombre négatif, bien que lastIndexOf puisse renvoyer -1 s'il ne trouve pas la valeur 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), une exception StringIndexOutOfBoundsException sera levée . Pour corriger l'erreur, vous devez vérifier le résultat renvoyé par la fonction lastIndexOf , et s'il est correct (au moins non négatif), puis le transmettre à la fonction de sous - chaîne .

Compteur de cycles 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(); } 

La boucle for utilise l'index constant 0 pour faire référence à l'élément du tableau types . Peut-être était-il destiné à utiliser la variable i comme index pour accéder aux éléments du tableau, mais ils ne l'ont pas ignoré, comme ils disent.

Do-while sans signification

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 la condition de boucle do - while (frame.readable ()) est du code inaccessible, car la méthode quitte toujours la première itération de la boucle. Dans le corps de la boucle, plusieurs vérifications de la variable msg sont effectuées à l'aide de if-else, et dans if et in else toujours renvoyer une valeur de la méthode ou lever une exception. C'est pour cette raison que le corps du cycle ne sera exécuté qu'une seule fois, par conséquent, l'utilisation de la boucle do-while n'a pas de sens.

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 de commutateur pour WAITING et TIMED_WAITING contient un code copier-coller. Si vous devez vraiment faire les mêmes choses, vous pouvez simplifier le code en supprimant le contenu dans le bloc de cas pour WAITING . Par conséquent, le même code enregistré en une seule copie sera exécuté pour WAITING et TIMED_WAITING .

Conclusion

Quiconque souhaite utiliser RPC en Java a probablement entendu parler d'Apache Dubbo. Il s'agit d'un projet open source populaire avec une longue histoire et du code écrit par de nombreux développeurs. Le code de ce projet est d'excellente qualité, mais l'analyseur statique PVS-Studio a réussi à trouver un certain nombre d'erreurs. De cela, nous pouvons conclure que l'analyse statique est très importante lors du développement de projets de moyenne et grande envergure, peu importe la perfection de votre code.

Remarque De telles vérifications ponctuelles démontrent les capacités d'un analyseur de code statique, mais sont une manière complètement erronée de l'utiliser. Cette idée est présentée plus en détail ici et ici . Utilisez l'analyse régulièrement!

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

Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Valery Komarov. Analyse du framework Apache Dubbo RPC par l'analyseur de code statique PVS-Studio .