Analyse des Apache Dubbo RPC Frameworks mit dem PVS-Studio Static Code Analyzer

Apache Dubbo ist eines der beliebtesten Java-Projekte auf GitHub. Das ist nicht überraschend. Es wurde vor 8 Jahren erstellt und wird häufig als Hochleistungs-RPC-Umgebung eingesetzt. Natürlich sind die meisten Fehler in seinem Code seit langem behoben und die Qualität des Codes wird auf einem hohen Niveau gehalten. Es gibt jedoch keinen Grund, die Überprüfung eines so interessanten Projekts mit dem statischen Code-Analysator PVS-Studio abzulehnen. Mal sehen, wie es ausgegangen ist.

Über PVS-Studio

Der statische Code-Analysator PVS-Studio gibt es seit mehr als 10 Jahren auf dem IT-Markt und ist eine multifunktionale und einfach einzuführende Softwarelösung. Derzeit unterstützt der Analyzer C, C ++, C #, Java und funktioniert unter Windows, Linux und MacOS.

PVS-Studio ist eine B2B-Lösung und wird von einer Vielzahl von Teams in verschiedenen Unternehmen eingesetzt. Wenn Sie die Funktionen des Analysators schätzen möchten, können Sie die Distribution herunterladen und hier einen Testschlüssel anfordern.

Es gibt auch Optionen, wie Sie PVS-Studio kostenlos in Open Source-Projekten oder als Student verwenden können.

Apache Dubbo: Was es ist und Hauptmerkmale

Heutzutage werden fast alle großen Softwaresysteme vertrieben . Wenn in einem verteilten System eine interaktive Verbindung zwischen Remote-Komponenten mit geringer Latenz und relativ wenig zu übertragenden Daten besteht, ist dies ein wichtiger Grund für die Verwendung einer RPC- Umgebung (Remote Procedure Call).

Apache Dubbo ist eine leistungsstarke RPC- Umgebung (Remote Procedure Call) mit Open Source-Code auf Java-Basis. Wie viele andere RPC-Systeme basiert auch Dubbo auf der Idee, einen Service zu erstellen, der einige Methoden definiert, die mit ihren Parametern und Arten von Rückgabewerten remote aufgerufen werden können. Auf der Serverseite wird eine Schnittstelle implementiert und der Server des Dubbo wird gestartet, um Kundenanfragen zu bearbeiten. Auf der Clientseite gibt es einen Stub, der dieselben Methoden wie der Server bereitstellt. Dubbo schlägt drei Schlüsselfunktionen vor, darunter Remote-Anrufe an der Schnittstelle, Fehlertoleranz und Lastausgleich sowie automatische Registrierung und Serviceerkennung.

Über die Analyse

Die Aktionssequenz der Analyse ist recht einfach und hat in meinem Fall nicht viel Zeit in Anspruch genommen:

• Heruntergeladener Apache Dubbo von GitHub ;
• Verwendete Anweisungen zum Starten des Java-Analysators und führte die Analyse aus.
• Erhielt den Analysatorbericht, überprüfte ihn und hob interessante Fälle hervor.

Die Analyseergebnisse: Für mehr als 4000 Dateien wurden 73 Warnungen mit hoher und mittlerer Sicherheit (46 bzw. 27) ausgegeben, was ein guter Indikator für die Codequalität ist.

Nicht alle Warnungen sind Fehler. Es ist ein übliches Ergebnis, da man es vor der direkten Verwendung des Analysators konfigurieren muss. Danach können Sie einen relativ geringen Prozentsatz falsch positiver Ergebnisse erwarten ( Beispiel ).

Ich habe 9 Warnungen (7 hoch und 2 mittel) in Bezug auf Dateien mit Tests nicht berücksichtigt.

Infolgedessen hatte ich eine kleine Anzahl von Warnungen, die auch falsch positive Ergebnisse enthielten, da ich den Analysator nicht konfiguriert habe. Alle 73 Warnungen mit weiteren Beschreibungen im Artikel zu untersuchen, ist lang, lächerlich und langweilig, deshalb habe ich die dümmsten ausgewählt.

Sign Byte in Java

V6007 Der Ausdruck 'endKey [i] <0xff' ist immer wahr. 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); } 

Ein Wert vom Bytetyp (der Wert von -128 bis 127) wird mit dem Wert 0xff (255) verglichen. In dieser Bedingung wird nicht berücksichtigt, dass der Bytetyp in Java signiert ist. Daher ist die Bedingung immer wahr, was bedeutet, dass sie bedeutungslos ist.

Rückgabe der gleichen Werte

V6007 Ausdruck 'isPreferIPV6Address ()' ist immer falsch. 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(); } 

Die Methode ist PreferIPV6Address .

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

Die Methode isPreferIPV6Address gibt in beiden Fällen false zurück. Höchstwahrscheinlich wollte ein Entwickler, dass ein Fall true zurückgibt, andernfalls macht die Methode keinen Sinn.

Sinnlose Kontrollen

V6007 Ausdruck '! Mask [i] .equals (ipAddress [i])' ist immer wahr. 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; } 

In der Bedingung if-else-if ist das Häkchen "*". Equals (mask [i]) || mask [i] .equals (ipAddress [i]) wird ausgeführt. Wenn die Bedingung nicht erfüllt ist, erfolgt beim nächsten Einchecken von if-else-if, was zeigt, dass mask [i] und ipAddress [i] nicht gleich sind. Bei einer der folgenden Überprüfungen von if-else-if wird überprüft, ob Maske [i] und ipAddress [i] nicht gleich sind. Da mask [i] und ipAddress [i ] keine Werte zugewiesen sind, ist die zweite Überprüfung sinnlos.

V6007 Der Ausdruck 'message.length> 0' ist immer wahr. DeprecatedTelnetCodec.java (302)

V6007 Ausdruck 'message! = Null' ist immer wahr. 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; } } } .... } 

Die Prüfnachricht! = Null && message.length> 0 in Zeile 302 ist redundant. Vor dem Einchecken in Zeile 302 wird folgende Prüfung durchgeführt:

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

Wenn die Bedingung der Prüfung nicht erfüllt ist, wissen wir, dass die Nachricht nicht null und ihre Länge ungleich 0 ist. Daraus folgt, dass ihre Länge größer als 0 ist (da die Länge eines Strings nicht a sein kann negative Zahl). Der lokalen Variablennachricht wird vor Zeile 302 kein Wert zugewiesen, was bedeutet, dass in Zeile 302 der Wert der Nachrichtenvariablen der gleiche ist wie im obigen Code. Es kann gefolgert werden, dass der Ausdruck message! = Null && message.length> 0 immer wahr ist, der Code im else-Block jedoch niemals ausgeführt wird.

Festlegen des Werts eines nicht initialisierten Referenzfelds

V6007 Ausdruck '! ShouldExport ()' ist immer falsch. ServiceConfig.java (371)

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

Die shouldExport- Methode der ServiceConfig- Klasse ruft die in derselben Klasse definierte getExport- Methode auf.

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

Die Methode getExport ruft die Methode getExport der abstrakten AbstractServiceConfig- Klasse auf, die den Wert des Exportfelds vom Typ Boolean zurückgibt. Es gibt auch eine setExport- Methode zum Festlegen des Feldwerts .

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

Das Exportfeld wird im Code nur von der setExport- Methode festgelegt. Die setExport- Methode wird nur in der Build-Methode der abstrakten AbstractServiceBuilder- Klasse (Erweiterung von AbstractServiceConfig ) nur für den Fall aufgerufen, dass das Feld nicht null ist.

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

Aufgrund der Tatsache, dass alle Referenzfelder standardmäßig mit null initialisiert sind und dem Exportfeld kein Wert zugewiesen wurde, wird die setExport- Methode niemals aufgerufen.

Infolgedessen gibt die getExport- Methode der ServiceConfig- Klasse, die die AbstractServiceConfig- Klasse erweitert, immer null zurück . Der Rückgabewert wird in der shouldExport- Methode der ServiceConfig- Klasse verwendet, daher gibt die shouldExport- Methode immer true zurück . Aufgrund der Rückgabe von true ist der Wert des Ausdrucks ! ShouldExport () immer false. Es stellt sich heraus, dass das Exportfeld der ServiceConfig- Klasse erst zurückgegeben wird, wenn der folgende Code ausgeführt wird:

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

Wert des nicht negativen Parameters

V6009 Die Funktion 'Teilzeichenfolge' könnte den Wert '-1' empfangen, während ein nicht negativer Wert erwartet wird. Argument überprüfen: 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); } } } } 

Das Ergebnis der Funktion lastIndexOf wird als zweiter Parameter an die Teilzeichenfolgenfunktion übergeben , deren zweiter Parameter keine negative Zahl sein darf, obwohl lastIndexOf -1 zurückgeben kann, wenn der gesuchte Wert in der Zeichenfolge nicht gefunden wird. Wenn der zweite Parameter der Teilstring- Methode kleiner als der erste ist (-1 <0), wird die Ausnahme StringIndexOutOfBoundsException ausgelöst. Um den Fehler zu beheben, müssen wir das Ergebnis überprüfen, das von der Funktion lastIndexOf zurückgegeben wird. Wenn es korrekt ist (zumindest nicht negativ), übergeben Sie es an die Teilzeichenfolgenfunktion .

Unbenutzter Schleifenzähler

V6016 Verdächtiger Zugriff auf ein Element vom Typ 'Typ' durch einen konstanten Index innerhalb einer Schleife. 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(); } 

In der for- Schleife wird ein 0- Konstantenindex verwendet, um auf das Element der Array- Typen zuzugreifen. Möglicherweise wurde die Variable i als Index für den Zugriff auf die Array-Elemente verwendet. Aber die Autoren haben das weggelassen.

Sinnlose Pause

V6019 Nicht erreichbarer Code erkannt. Möglicherweise liegt ein Fehler vor. 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()); // <= .... } 

Der Ausdruck im Zustand der Schleife do - while (frame.readable ()) ist nicht erreichbarer Code, da die Methode während der ersten Iteration der Schleife beendet wird. Mit if-else werden im Hauptteil der Schleife mehrere Überprüfungen der msg- Variablen durchgeführt. Dabei werden sowohl if- als auch else- Werte von der Methode zurückgegeben oder Ausnahmen ausgelöst. Das ist der Grund, warum der Schleifenkörper nur einmal ausgeführt wird, sodass die Verwendung dieser Schleife keinen Sinn hat.

Kopieren-Einfügen in Schalter

V6067 Zwei oder mehr Fallzweige führen dieselben Aktionen aus. 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: } } .... } 

Der Code in switch für die Fälle WAITING und TIMED_WAITING enthält Code zum Einfügen und Einfügen. Wenn dieselben Aktionen ausgeführt werden müssen, kann der Code vereinfacht werden, indem der Inhalt des WAITING- Fallblocks entfernt wird. Infolgedessen wird der gleiche Code für WAITING und TIMED_WAITING ausgeführt.

Fazit

Jeder, der sich für die Verwendung von RPC in Java interessiert, hat wahrscheinlich von Apache Dubbo gehört. Es ist ein beliebtes Open Source-Projekt mit einer langen Geschichte und Code, das von vielen Entwicklern geschrieben wurde. Der Code dieses Projekts ist von hoher Qualität, dennoch konnte der statische Code-Analysator PVS-Studio eine bestimmte Anzahl von Fehlern finden. Dies führt dazu, dass die statische Analyse bei der Entwicklung von mittelgroßen und großen Projekten sehr wichtig ist, unabhängig davon, wie perfekt Ihr Code ist.

Hinweis Solche einmaligen Überprüfungen demonstrieren die Fähigkeiten eines statischen Code-Analysators, stellen jedoch eine völlig falsche Art seiner Verwendung dar. Weitere Details zu dieser Idee finden Sie hier und hier . Verwenden Sie die Analyse regelmäßig!

Vielen Dank an die Entwickler von Apache Dubbo für dieses wundervolle Tool. Ich hoffe, dieser Artikel wird Ihnen helfen, den Code zu verbessern. Der Artikel beschreibt nicht alle verdächtigen Codeteile. Entwickler sollten daher das Projekt selbst überprüfen und die Ergebnisse bewerten.