PVS-Studio für Java macht sich auf den Weg. Nächster Halt ist Elasticsearch

Das PVS-Studio-Team führt seit vielen Jahren den Blog über die Überprüfung von Open-Source-Projekten durch den gleichnamigen statischen Code-Analysator. Bisher wurden mehr als 300 Projekte geprüft, die Fehlerbasis enthält mehr als 12000 Fälle. Ursprünglich wurde der Analysator zur Überprüfung von C- und C ++ - Code implementiert, die Unterstützung von C # wurde später hinzugefügt. Daher entfällt bei allen geprüften Projekten die Mehrheit (> 80%) auf C und C ++. Vor kurzem wurde Java zur Liste der unterstützten Sprachen hinzugefügt, was bedeutet, dass es jetzt eine völlig neue offene Welt für PVS-Studio gibt. Daher ist es an der Zeit, die Basis mit Fehlern aus Java-Projekten zu ergänzen.

Die Java-Welt ist riesig und vielfältig, sodass man nicht einmal weiß, wo man zuerst suchen muss, wenn man ein Projekt zum Testen des neuen Analysators auswählt. Letztendlich fiel die Wahl auf die Volltextsuche und Analyse-Engine Elasticsearch. Es ist ein ziemlich erfolgreiches Projekt, und es ist sogar besonders angenehm, Fehler in bedeutenden Projekten zu finden. Welche Fehler konnte PVS-Studio für Java erkennen? Weitere Gespräche über die Ergebnisse der Überprüfung werden richtig sein.

Kurz über Elasticsearch

Elasticsearch ist eine verteilte, RESTful-Such- und Analyse-Engine mit Open Source-Code, die eine wachsende Anzahl von Anwendungsfällen lösen kann. Sie können damit große Datenmengen speichern, eine schnelle Suche und Analyse durchführen (fast im Echtzeitmodus). In der Regel wird es als zugrunde liegender Mechanismus / Technologie verwendet, der Anwendungen mit komplexen Funktionen und Suchanforderungen versorgt.

Zu den wichtigsten Websites, die Elasticsearch verwenden, gehören Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM und Qbox.

Gut, genug Einführung.

Die ganze Geschichte, wie die Dinge waren

Es gab keine Probleme mit dem Scheck selbst. Die Abfolge der Aktionen ist recht einfach und hat nicht viel Zeit in Anspruch genommen:

• Elasticsearch von GitHub heruntergeladen;
• Befolgen Sie die Anweisungen zum Ausführen des Java-Analysators und führen Sie die Analyse aus.
• Erhielt den Bericht des Analysators, vertiefte sich in ihn und wies auf interessante Fälle hin.

Kommen wir nun zum Hauptpunkt.

Achtung! Mögliche NullPointerException

V6008 Null-Dereferenzierung von 'Linie'. GoogleCloudStorageFixture.java (451)

private static PathTrie<RequestHandler> defaultHandlers(....) { .... handlers.insert("POST /batch/storage/v1", (request) -> { .... // Reads the body line = reader.readLine(); byte[] batchedBody = new byte[0]; if ((line != null) || (line.startsWith("--" + boundary) == false)) // <= { batchedBody = line.getBytes(StandardCharsets.UTF_8); } .... }); .... } 

Der Fehler in diesem Codefragment besteht darin, dass der Aufruf der Methode launchWith unter der Bedingung der if- Anweisung dazu führt, dass die NullPointerException- Ausnahme ausgelöst wird, wenn die Zeichenfolge aus dem Puffer nicht gelesen wurde. Dies ist höchstwahrscheinlich ein Tippfehler, und beim Schreiben einer Bedingung meinten Entwickler den Operator && anstelle von || .

V6008 Mögliche Null-Dereferenzierung von 'followIndexMetadata'. TransportResumeFollowAction.java (171), TransportResumeFollowAction.java (170), TransportResumeFollowAction.java (194)

 void start( ResumeFollowAction.Request request, String clusterNameAlias, IndexMetaData leaderIndexMetadata, IndexMetaData followIndexMetadata, ....) throws IOException { MapperService mapperService = followIndexMetadata != null // <= ? .... : null; validate(request, leaderIndexMetadata, followIndexMetadata, // <= leaderIndexHistoryUUIDs, mapperService); .... } 

Eine weitere Warnung der V6008- Diagnose. Das Objekt followIndexMetadata hat mein Interesse geweckt. Die Startmethode akzeptiert mehrere Argumente als Eingabe, unser Verdächtiger ist genau richtig. Danach wird basierend auf der Überprüfung unseres Objekts auf Null ein neues Objekt erstellt, das an der weiteren Methodenlogik beteiligt ist. Die Prüfung auf Null zeigt uns, dass followIndexMetadata immer noch als Null-Objekt von außen kommen kann. Nun, schauen wir weiter.

Dann werden mehrere Argumente an die Validierungsmethode gesendet (wieder befindet sich unser betrachtetes Objekt unter ihnen). Wenn wir uns die Implementierung der Validierungsmethode ansehen, passt alles zusammen. Unser potenzielles Nullobjekt wird als drittes Argument an die validate- Methode übergeben, wo es bedingungslos dereferenziert wird. Mögliche NullPointerException als Ergebnis.

 static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex, // <= ....) { .... Map<String, String> ccrIndexMetadata = followIndex.getCustomData(....); // <= if (ccrIndexMetadata == null) { throw new IllegalArgumentException(....); } .... }} 

Wir wissen nicht genau, mit welchen Argumenten die Startmethode aufgerufen wird. Es ist durchaus möglich, dass alle Argumente vor dem Aufruf der Methode irgendwo überprüft werden und keine Nullobjekt-Dereferenzierung auftritt. Auf jeden Fall sollten wir zugeben, dass eine solche Code-Implementierung immer noch unzuverlässig aussieht und Aufmerksamkeit verdient.

V6060 Die ' Knoten' -Referenz wurde verwendet, bevor sie gegen Null verifiziert wurde. RestTasksAction.java (152), RestTasksAction.java (151)

 private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNodes discoveryNodes, TaskInfo taskInfo) { .... DiscoveryNode node = discoveryNodes.get(nodeId); .... // Node information. Note that the node may be null because it has // left the cluster between when we got this response and now. table.addCell(fullId ? nodeId : Strings.substring(nodeId, 0, 4)); table.addCell(node == null ? "-" : node.getHostAddress()); table.addCell(node.getAddress().address().getPort()); table.addCell(node == null ? "-" : node.getName()); table.addCell(node == null ? "-" : node.getVersion().toString()); .... } 

Eine weitere Diagnoseregel mit dem gleichen Problem wurde hier ausgelöst. NullPointerException . Die Regel lautet für Entwickler: "Leute, was machst du? Wie kannst du das machen? Oh, es ist schrecklich! Warum verwenden Sie das Objekt zuerst und prüfen in der nächsten Zeile, ob es null ist? Hier erfahren Sie, wie eine Nullobjekt-Dereferenzierung auftritt. Leider hat auch der Kommentar eines Entwicklers nicht geholfen.

V6060 Die Referenz "Ursache" wurde verwendet, bevor sie gegen Null verifiziert wurde. StartupException.java (76), StartupException.java (73)

 private void printStackTrace(Consumer<String> consumer) { Throwable originalCause = getCause(); Throwable cause = originalCause; if (cause instanceof CreationException) { cause = getFirstGuiceCause((CreationException)cause); } String message = cause.toString(); // <= consumer.accept(message); if (cause != null) { // <= // walk to the root cause while (cause.getCause() != null) { cause = cause.getCause(); } .... } .... } 

In diesem Fall sollten wir berücksichtigen, dass die getCause- Methode der Throwable- Klasse möglicherweise null zurückgibt . Das obige Problem wiederholt sich weiter, so dass seine Erklärung unnötig ist.

Sinnlose Bedingungen

V6007 Ausdruck 's.charAt (i)! =' \ T '' ist immer wahr. Cron.java (1223)

 private static int findNextWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) { // intentionally empty } return i; } 

Die betrachtete Funktion gibt den Index des ersten Leerzeichens beginnend mit dem i- Index zurück. Was ist los? Wir haben den Analysator, der warnt, dass s.charAt (i)! = '\ T' immer wahr ist, was den Ausdruck (s.charAt (i)! = '' || s.charAt (i)! = '\ T bedeutet ') wird auch immer wahr sein. Ist das wahr? Ich denke, Sie können dies leicht sicherstellen, indem Sie ein beliebiges Zeichen ersetzen.

Infolgedessen gibt diese Methode immer den Index zurück, der gleich s.length () ist , was falsch ist. Ich würde es wagen vorzuschlagen, dass die obige Methode hier schuld ist:

 private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) { // intentionally empty } return i; } 

Die Entwickler haben die Methode implementiert, dann kopiert und unsere fehlerhafte Methode findNextWhiteSpace erhalten, nachdem einige Änderungen vorgenommen wurden. Sie haben die Methode immer wieder repariert, aber nicht repariert. Um dies richtig zu machen, sollte man den Operator && anstelle von || verwenden .

V6007 Der Ausdruck 'verbleibend == 0' ist immer falsch. PemUtils.java (439)

 private static byte[] generateOpenSslKey(char[] password, byte[] salt, int keyLength) { .... int copied = 0; int remaining; while (copied < keyLength) { remaining = keyLength - copied; .... copied += bytesToCopy; if (remaining == 0) { // <= break; } .... } .... } 

Anhand der Bedingung der kopierten <keyLength- Schleife können wir feststellen, dass kopiert immer kleiner als keyLength ist . Daher ist es sinnlos, die verbleibende Variable mit 0 zu vergleichen, und es ist immer falsch. An diesem Punkt wird die Schleife nicht durch eine Bedingung beendet. Code entfernen oder Verhaltenslogik überdenken? Ich denke, nur Entwickler werden in der Lage sein, alle Punkte über das i zu setzen.

V6007 Der Ausdruck 'healthCheckDn.indexOf (' = ')> 0' ist immer falsch. ActiveDirectorySessionFactory.java (73)

 ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException { super(...., () -> { if (....) { final String healthCheckDn = ....; if (healthCheckDn.isEmpty() && healthCheckDn.indexOf('=') > 0) { return healthCheckDn; } } return ....; }, ....); .... } 

Wieder bedeutungsloser Ausdruck. Entsprechend der Bedingung muss die Zeichenfolge healthCheckDn leer sein und das Zeichen '=' nicht an der ersten Position enthalten, damit der Lambda-Ausdruck die Zeichenfolgenvariable healthCheckDn zurückgibt . Puh, das ist alles dann! Wie Sie wahrscheinlich verstanden haben, ist es unmöglich. Wir werden nicht tief in den Code eintauchen, sondern es dem Ermessen der Entwickler überlassen.

Ich habe nur einige der fehlerhaften Beispiele angeführt, aber darüber hinaus gab es viele diagnostische Auslösungen des V6007 , die einzeln betrachtet werden sollten, um relevante Schlussfolgerungen zu ziehen.

Kleine Methode kann einen langen Weg gehen

 private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; } 

Hier haben wir also eine winzige Methode mit mehreren Zeilen. Aber Bugs sind auf der Hut! Die Analyse dieser Methode ergab das folgende Ergebnis:

1. V6007 Ausdruck '(int) x <0' ist immer falsch. BCrypt.java (429)
2. V6025 Möglicherweise ist der Index '(int) x' außerhalb der Grenzen. BCrypt.java (431)

Ausgabe N1. Der Ausdruck (int) x <0 ist immer falsch (Ja, wieder V6007 ). Die Variable x kann nicht negativ sein, da sie vom Typ char ist . Der Zeichentyp ist eine Ganzzahl ohne Vorzeichen. Es kann nicht als echter Fehler bezeichnet werden, dennoch ist die Prüfung redundant und kann entfernt werden.

Ausgabe N2. Möglicher Array-Index außerhalb der Grenzen, was zur Ausnahme ArrayIndexOutOfBoundsException führt . Dann stellt sich die Frage, die an der Oberfläche liegt: "Warten Sie, wie wäre es mit der Indexprüfung?"

Wir haben also ein Array mit fester Größe von 128 Elementen:

 private static final byte index_64[] = { -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, -1, 0, 1, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, -1, -1, -1, -1, -1, -1, -1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, -1, -1, -1, -1, -1, -1, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, -1, -1, -1, -1, -1 }; 

Wenn die char64- Methode die Variable x empfängt, wird die Gültigkeit des Index überprüft. Wo ist der Fehler? Warum ist ein Array-Index außerhalb der Grenzen noch möglich?

Die Prüfung (int) x> index_64.length ist nicht ganz korrekt. Wenn die char64-Methode x mit dem Wert 128 empfängt, schützt die Prüfung nicht vor ArrayIndexOutOfBoundsException. Vielleicht passiert das in der Realität nie. Die Prüfung ist jedoch falsch geschrieben, und man muss den Operator "größer als" (>) durch "größer als oder gleich (> =)" ändern.

Vergleiche, die ihr Bestes gaben

V6013 Die Nummern 'displaySize' und 'that.displaySize' werden anhand der Referenz verglichen. Möglicherweise war ein Gleichstellungsvergleich beabsichtigt. ColumnInfo.java (122)

 .... private final String table; private final String name; private final String esType; private final Integer displaySize; .... @Override public boolean equals(Object o) { if (this == o) { return true; } if (o == null || getClass() != o.getClass()) { return false; } ColumnInfo that = (ColumnInfo) o; return displaySize == that.displaySize && // <= Objects.equals(table, that.table) && Objects.equals(name, that.name) && Objects.equals(esType, that.esType); } 

Was hier falsch ist, ist, dass displaySize- Objekte vom Typ Integer mit dem Operator == verglichen werden, dh als Referenz. Es ist durchaus möglich, dass ColumnInfo- Objekte verglichen werden, deren displaySize- Felder unterschiedliche Referenzen, aber denselben Inhalt haben. In diesem Fall liefert der Vergleich das negative Ergebnis, wenn wir erwartet haben, dass es wahr wird.

Ich wage zu vermuten, dass ein solcher Vergleich das Ergebnis eines fehlgeschlagenen Refactorings sein könnte und das Feld displaySize anfangs vom Typ int war .

V6058 Die Funktion 'equals' vergleicht Objekte inkompatibler Typen: Integer, TimeValue. DatafeedUpdate.java (375)

 .... private final TimeValue queryDelay; private final TimeValue frequency; .... private final Integer scrollSize; .... boolean isNoop(DatafeedConfig datafeed) { return (frequency == null || Objects.equals(frequency, datafeed.getFrequency())) && (queryDelay == null || Objects.equals(queryDelay, datafeed.getQueryDelay())) && (scrollSize == null || Objects.equals(scrollSize, datafeed.getQueryDelay())) // <= && ....) } 

Wieder falscher Objektvergleich. Dieses Mal werden Objekte mit inkompatiblen Typen verglichen ( Integer und TimeValue ). Das Ergebnis dieses Vergleichs ist offensichtlich und immer falsch. Sie können sehen, dass Klassenfelder miteinander verglichen werden, nur die Namen der Felder müssen geändert werden. Hier ist der Punkt: Ein Entwickler hat beschlossen, den Prozess durch Kopieren und Einfügen zu beschleunigen, und einen Fehler in das Geschäft aufgenommen. Die Klasse implementiert einen Getter für das scrollSize- Feld. Um den Fehler zu beheben, sollte die Methode datafeed .getScrollSize () verwendet werden.

Schauen wir uns ein paar fehlerhafte Beispiele ohne Erklärung an. Probleme liegen auf der Hand.

V6001 Links und rechts vom Operator '==' befinden sich identische Unterausdrücke 'takeInMillis'. TermVectorsResponse.java (152)

 @Override public boolean equals(Object obj) { .... return index.equals(other.index) && type.equals(other.type) && Objects.equals(id, other.id) && docVersion == other.docVersion && found == other.found && tookInMillis == tookInMillis // <= && Objects.equals(termVectorList, other.termVectorList); } 

V6009 Funktion 'gleich' erhält ein ungerades Argument. Ein Objekt 'shardId.getIndexName ()' wird als Argument für seine eigene Methode verwendet. SnapshotShardFailure.java (208)

 @Override public boolean equals(Object o) { .... return shardId.id() == that.shardId.id() && shardId.getIndexName().equals(shardId.getIndexName()) && // <= Objects.equals(reason, that.reason) && Objects.equals(nodeId, that.nodeId) && status.getStatus() == that.status.getStatus(); } 

Verschiedenes

V6006 Das Objekt wurde erstellt, wird jedoch nicht verwendet. Das Schlüsselwort 'throw' könnte fehlen. JdbcConnection.java (88)

 @Override public void setAutoCommit(boolean autoCommit) throws SQLException { checkOpen(); if (!autoCommit) { new SQLFeatureNotSupportedException(....); } } 

Der Fehler ist offensichtlich und bedarf keiner Erklärung. Ein Entwickler hat eine Ausnahme erstellt, diese aber nirgendwo anders ausgelöst. Eine solche anonyme Ausnahme wird erfolgreich erstellt und vor allem nahtlos entfernt. Der Grund ist das Fehlen des Wurfoperators .

V6003 Die Verwendung des Musters 'if (A) {....} else if (A) {....}' wurde erkannt. Es besteht die Wahrscheinlichkeit eines logischen Fehlers. MockScriptEngine.java (94), MockScriptEngine.java (105)

 @Override public <T> T compile(....) { .... if (context.instanceClazz.equals(FieldScript.class)) { .... } else if (context.instanceClazz.equals(FieldScript.class)) { .... } else if(context.instanceClazz.equals(TermsSetQueryScript.class)) { .... } else if (context.instanceClazz.equals(NumberSortScript.class)) .... } 

In mehreren Fällen wird eine der Bedingungen zweimal wiederholt, sodass die Situation eine kompetente Codeüberprüfung erfordert.

V6039 Es gibt zwei ' if' -Anweisungen mit identischen bedingten Ausdrücken. Die erste 'if'-Anweisung enthält die Methodenrückgabe. Dies bedeutet, dass die zweite 'if'-Anweisung sinnlos ist. SearchAfterBuilder.java (94), SearchAfterBuilder.java (93)

 public SearchAfterBuilder setSortValues(Object[] values) { .... for (int i = 0; i < values.length; i++) { if (values[i] == null) continue; if (values[i] instanceof String) continue; if (values[i] instanceof Text) continue; if (values[i] instanceof Long) continue; if (values[i] instanceof Integer) continue; if (values[i] instanceof Short) continue; if (values[i] instanceof Byte) continue; if (values[i] instanceof Double) continue; if (values[i] instanceof Float) continue; if (values[i] instanceof Boolean) continue; // <= if (values[i] instanceof Boolean) continue; // <= throw new IllegalArgumentException(....); } .... } 

Die gleiche Bedingung wird zweimal hintereinander verwendet. Ist die zweite Bedingung überflüssig oder sollte anstelle von Boolean ein anderer Typ verwendet werden?

V6009 Die Funktion 'Teilzeichenfolge' empfängt ungerade Argumente. Das Argument 'queryStringIndex + 1' sollte nicht größer als 'queryStringLength' sein. LoggingAuditTrail.java (660)

 LogEntryBuilder withRestUriAndMethod(RestRequest request) { final int queryStringIndex = request.uri().indexOf('?'); int queryStringLength = request.uri().indexOf('#'); if (queryStringLength < 0) { queryStringLength = request.uri().length(); } if (queryStringIndex < 0) { logEntry.with(....); } else { logEntry.with(....); } if (queryStringIndex > -1) { logEntry.with(...., request.uri().substring(queryStringIndex + 1,// <= queryStringLength)); // <= } .... } 

Betrachten wir hier das fehlerhafte Szenario, das die Ausnahme StringIndexOutOfBoundsException verursachen kann . Die Ausnahme tritt auf, wenn request.uri () eine Zeichenfolge zurückgibt, die das Zeichen '#' vor '?' Enthält. Es gibt keine Überprüfungen in der Methode. In diesem Fall brauen sich die Probleme zusammen. Möglicherweise wird dies aufgrund verschiedener Überprüfungen des Objekts außerhalb der Methode niemals passieren, aber es ist nicht die beste Idee, Hoffnungen darauf zu setzen.

Fazit

Seit vielen Jahren hilft PVS-Studio dabei, Fehler im Code von kommerziellen und kostenlosen Open-Source-Projekten zu finden. Erst kürzlich wurde Java in die Liste der unterstützten Sprachen für die Analyse aufgenommen. Elasticsearch wurde einer der ersten Tests für unseren Newcomer. Wir hoffen, dass dieser Check für das Projekt nützlich und für die Leser interessant ist.

PVS-Studio für Java benötigt neue Herausforderungen, neue Benutzer, aktives Feedback und Kunden, um sich schnell an die neue Welt anzupassen :). Deshalb lade ich Sie ein, unseren Analysator sofort für Ihr Arbeitsprojekt herunterzuladen und zu testen!