PVS-Studio für Java wird an den Pfad gesendet. Nächster Halt ist Elasticsearch

Weit entfernt vom ersten Jahr hat das PVS-Studio-Team über Überprüfungen von Open-Source-Projekten durch den gleichnamigen statischen Code-Analysator gebloggt. Bisher wurden mehr als 300 Projekte geprüft und mehr als 12.000 Fälle in die Datenbank der gefundenen Fehler geschrieben. Zunächst wurde der Analysator implementiert, um C- und C ++ - Code zu testen. Anschließend wurde die Unterstützung der C # -Sprache angezeigt. Daher fällt unter den getesteten Projekten die Mehrheit (> 80%) auf C und C ++. In jüngerer Zeit wurde Java zu unterstützten Sprachen hinzugefügt, was bedeutet, dass PVS-Studio die Türen zu einer neuen Welt öffnet und es Zeit ist, die Datenbank mit Fehlern aus Java-Projekten zu ergänzen.

Die Java-Welt ist riesig und vielfältig, daher weiten sich meine Augen, wenn ich ein Projekt zum Testen eines neuen Analysators auswähle. Letztendlich fiel die Wahl auf die Volltextsuchmaschine und Analytics Elasticsearch. Dies ist ein ziemlich erfolgreiches Projekt, und in erfolgreichen Projekten ist es doppelt oder sogar dreifach angenehmer, Fehler zu finden. Welche Fehler hat PVS-Studio für Java festgestellt? Das Ergebnis der Überprüfung wird im Artikel erläutert.

Oberflächliche Bekanntschaft mit Elasticsearch

Elasticsearch ist eine skalierbare Open Source-Volltextsuch- und Analyse-Engine. Sie können große Datenmengen speichern und eine schnelle Suche und Analyse durchführen (fast in Echtzeit). In der Regel wird es als zugrunde liegender Mechanismus / Technologie verwendet, der Anwendungen komplexe Funktionen und Suchanforderungen bietet.

Unter den wichtigsten Websites, die Elasticsearch verwenden, sind Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM und Qbox aufgeführt.

Ich denke mit der Bekanntschaft ist genug.

Wie war es

Es gab keine Probleme mit der Überprüfung. Die Abfolge der Aktionen ist recht einfach und hat nicht viel Zeit in Anspruch genommen:

• Elasticsearch von GitHub heruntergeladen;
• Ich habe die Anweisungen zum Starten des Java-Analysators verwendet und die Analyse gestartet.
• Ich erhielt einen Analysatorbericht, analysierte ihn und hob interessante Fälle hervor.

Kommen wir jetzt zum Punkt.

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, wenn die Zeile aus dem Puffer nicht gelesen werden konnte, beim Aufrufen der Methode opensWith unter der Bedingung der if-Anweisung eine NullPointerException ausgelöst wird . Dies ist höchstwahrscheinlich ein Tippfehler, und beim Schreiben der Bedingung war der Operator && anstelle von || gemeint .

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. Bei näherer Betrachtung wurde das followIndexMetadata- Objekt nun vernietet. Die Startmethode verwendet mehrere Eingabeargumente, einschließlich unseres Verdächtigen. Auf der Grundlage der Überprüfung unseres Objekts auf Null wird dann ein neues Objekt gebildet, das an der weiteren Logik der Methode teilnimmt. Wenn Sie nach null suchen , erfahren Sie , dass followIndexMetadata immer noch mit einem null-Objekt von außen kommen kann. Ok, schau weiter.

Als nächstes wird die Validierungsmethode aufgerufen, indem viele Argumente gedrückt werden (wiederum das fragliche Objekt). Und wenn Sie sich die Implementierung der Validierungsmethode ansehen, dann passt alles zusammen. Unser potenzielles Nullobjekt wird vom dritten Argument an die validate- Methode übergeben, wo es bedingungslos dereferenziert wird. Infolgedessen eine potenzielle NullPointerException.

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

Es ist nicht bekannt, mit welchen Argumenten die Startmethode tatsächlich aufgerufen wird. Es ist möglich, dass alle Argumente irgendwo vor dem Methodenaufruf überprüft werden und wir keine Dereferenzierung des Null-Objekts feststellen. Sie müssen jedoch zugeben, dass eine solche Code-Implementierung immer noch ziemlich 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 andere Diagnoseregel hat hier funktioniert, aber das Problem ist dasselbe: NullPointerException . Die Regel lautet: „Leute, was machst du? Wie so? Oh Ärger! Warum verwenden Sie das Objekt zuerst und überprüfen es dann in der nächsten Codezeile auf Null ?! ” Es stellt sich also heraus, dass hier ein Nullobjekt dereferenziert wird. Leider hat auch der Kommentar eines der Entwickler 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(); } .... } .... } 

Hierbei ist zu beachten, dass die getCause- Methode der Throwable- Klasse null zurückgeben kann . Ferner wird das oben betrachtete Problem wiederholt, und es macht keinen Sinn, etwas im Detail zu erklären.

Bedeutungslose 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 zurück, beginnend mit Index i . Was ist los? Wir haben eine Warnung vom Analysator, dass s.charAt (i)! = '\ T' immer wahr ist, was bedeutet, dass es immer Wahrheit und den Ausdruck (s.charAt (i)! = '' || s.charAt (i) geben wird! = '\ t') . Ist es so? Ich denke, Sie selbst können dies leicht überprüfen, indem Sie ein beliebiges Zeichen ersetzen.

Infolgedessen gibt diese Methode immer einen Index zurück, der gleich s.length () ist , was nicht wahr ist. Ich wage anzunehmen, dass diese etwas höher gelegene Methode 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; } 

Wir haben diese Methode implementiert, dann kopiert und einige kleinere Korrekturen vorgenommen, um unsere fehlerhafte findNextWhiteSpace- Methode zu erhalten. Die Methode wurde angepasst, angepasst, aber nicht angepasst. Um Abhilfe zu schaffen, müssen Sie 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; } .... } .... } 

Aus dem Zustand des kopierten Zyklus <keyLength kann festgestellt werden, dass der kopierte Zyklus immer kleiner als keyLength ist . Daher ist ein Vergleich der Gleichheit der verbleibenden Variablen mit 0 sinnlos und führt immer zu einem falschen Ergebnis, und daher wird die Bedingung nicht aus der Schleife verlassen. Lohnt es sich, diesen Code zu löschen, oder müssen Sie die Verhaltenslogik überdenken? Ich denke, nur Entwickler werden in der Lage sein, alle i zu punktieren.

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 ein bedeutungsloser Ausdruck. Damit der Lambda-Ausdruck die Zeichenfolgenvariable healthCheckDn zurückgibt, muss die Zeichenfolge healthCheckDn entsprechend der Bedingung leer sein und die Zeichenfolge mit dem Zeichen '=' befindet sich nicht an der ersten Position. Fuh, irgendwie erledigt. Und wie Sie richtig verstanden haben, ist dies unmöglich. Wir werden die Logik des Codes nicht verstehen, überlassen Sie es den Entwicklern.

Ich habe nur einige fehlerhafte Beispiele angegeben, aber darüber hinaus gab es viele Fälle, in denen die V6007- Diagnose ausgelöst wurde , die separat betrachtet und Schlussfolgerungen gezogen werden müssen.

Die Methode ist klein, aber udalenky

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

Wir haben also eine winzige Methode mit mehreren Zeilen. Aber Käfer schlafen nicht! Eine 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)

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

Problem N2. Möglicher Überlauf des Arrays, der zu einer ArrayIndexOutOfBoundsException führt . Dann stellt sich die Frage, die an der Oberfläche liegt: "Warten Sie, was ist mit der Überprüfung des Index?"

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 Variable x in die Eingabe der char64- Methode eingeht , prüft sie die Gültigkeit des Index. Wo ist die Lücke? Warum ist das Verlassen des Arrays immer noch möglich?

Das Überprüfen von (int) x> index_64.length ist nicht ganz korrekt. Wenn x mit einem Wert von 128 am Eingang der char64- Methode ankommt, schützt die Prüfung nicht vor ArrayIndexOutOfBoundsException . Vielleicht passiert das in der Realität nie. Die Prüfung ist jedoch falsch geschrieben, und Sie müssen den Operator "größer als" (>) durch "größer als oder gleich" (> =) ersetzen.

Vergleiche, die versucht haben

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

Die Unrichtigkeit hierbei ist, dass displaySize- Objekte vom Typ Integer über den Operator == verglichen werden, dh sie werden als Referenz verglichen. Es ist durchaus möglich, dass ColumnInfo- Objekte verglichen werden, für die die displaySize- Felder unterschiedliche Links, aber denselben Inhalt haben. In diesem Fall führt der Vergleich zu einem negativen Ergebnis, während wir die Wahrheit erwartet haben.

Ich würde vorschlagen, dass ein solcher Vergleich das Ergebnis eines fehlgeschlagenen Refactorings sein könnte, und anfangs war das Feld displaySize vom Typ int .

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())) // <= && ....) } 

Und wieder falscher Vergleich von Objekten. Vergleichen Sie nun Objekte, deren Typen nicht kompatibel sind ( Integer und TimeValue ). Das Ergebnis dieses Vergleichs ist offensichtlich und immer falsch. Es ist ersichtlich, dass die Felder der Klasse auf die gleiche Weise miteinander verglichen werden, es ist nur notwendig, die Feldnamen zu ändern. Daher beschloss der Entwickler, das Schreiben von Code durch Kopieren und Einfügen zu beschleunigen, gab sich jedoch den gleichen Fehler. Die Klasse implementiert einen Getter für das scrollSize- Feld. Um den Fehler zu korrigieren, besteht die richtige Lösung darin, die entsprechende Methode zu verwenden: datafeed .getScrollSize () .

Schauen wir uns ein paar weitere Beispiele für Fehler ohne Erklärung an. Das Problem ist bereits offensichtlich.

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. Der Entwickler hat eine Ausnahme ausgelöst, aber in keiner Weise weiter. Eine solche anonyme Ausnahme wird erfolgreich erstellt und auch erfolgreich und vor allem spurlos zerstört. Der Grund ist das Fehlen einer Wurfanweisung .

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 einem Konstrukt mit mehreren if-else wird eine der Bedingungen zweimal wiederholt, sodass die Situation eine kompetente Überprüfung des Codes 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. Die zweite Bedingung ist überflüssig oder muss 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)); // <= } .... } 

Stellen Sie sich sofort ein Fehlerszenario vor , das eine StringIndexOutOfBoundsException auslösen könnte . Eine Ausnahme tritt auf, wenn request.uri () eine Zeichenfolge zurückgibt, die das Zeichen '#' vor '?' Enthält. In einem solchen Fall gibt es keine Überprüfungen in der Methode, und wenn dies immer noch geschieht, wird eine Katastrophe nicht vermieden. Vielleicht wird dies aufgrund verschiedener Überprüfungen des Anforderungsobjekts außerhalb der Methode nie passieren, aber meiner Meinung nach ist es nicht die beste Idee, darauf zu hoffen.

Fazit

Im Laufe der Jahre hat PVS-Studio dazu beigetragen, Fehler im Code von kommerziellen und kostenlosen Open-Source-Projekten zu finden. In jüngerer Zeit wurde Java hinzugefügt, um analysierte Sprachen zu unterstützen. Und einer der ersten Tests für unseren Newcomer war die sich aktiv entwickelnde Elasticsearch. Wir hoffen, dass dieser Check für das Projekt nützlich und für die Leser interessant ist.

Damit sich PVS-Studio für Java schnell an eine neue Welt anpassen kann, benötigen wir neue Tests, neue Benutzer, aktives Feedback und Kunden :). Ich schlage daher vor, unseren Analysator unverzüglich auf Ihren Arbeitsentwurf herunterzuladen und zu testen!

Wenn Sie diesen Artikel einem englischsprachigen Publikum zugänglich machen möchten, verwenden Sie bitte den Link zur Übersetzung: Maxim Stefanov. PVS-Studio für Java macht sich auf den Weg. Nächster Halt ist Elasticsearch