L'équipe PVS-Studio tient le blog sur les vérifications des projets open source par l'analyseur de code statique du même nom depuis de nombreuses années. A ce jour, plus de 300 projets ont été vérifiés, la base d'erreurs contient plus de 12 000 cas. Initialement, l'analyseur a été implémenté pour vérifier le code C et C ++, le support de C # a été ajouté plus tard. Par conséquent, parmi tous les projets vérifiés, la majorité (> 80%) représente C et C ++. Tout récemment, Java a été ajouté à la liste des langages pris en charge, ce qui signifie qu'il existe maintenant un tout nouveau monde ouvert pour PVS-Studio, il est donc temps de compléter la base avec des erreurs de projets Java.
Le monde Java est vaste et varié, donc on ne sait même pas où chercher en premier lors du choix d'un projet pour tester le nouvel analyseur. Finalement, le choix s'est porté sur le moteur de recherche et d'analyse de texte intégral Elasticsearch. C'est un projet assez réussi et il est même particulièrement agréable de trouver des erreurs dans des projets importants. Alors, quels défauts PVS-Studio pour Java a-t-il réussi à détecter? D'autres discussions auront raison sur les résultats de la vérification.
En bref sur elasticsearch
Elasticsearch est un moteur de recherche et d'analyse distribué et RESTful avec du code open source, capable de résoudre un nombre croissant de cas d'utilisation. Il vous permet de stocker de grandes quantités de données, d'effectuer une recherche et des analyses rapides (presque en mode temps réel). En règle générale, il est utilisé comme mécanisme / technologie sous-jacent, qui fournit aux applications des fonctions complexes et des exigences de recherche.
Parmi les principaux sites utilisant Elasticsearch, il y a Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox.
Très bien, assez d'introduction.
Toute l'histoire de la façon dont les choses étaient
Il n'y a eu aucun problème avec le chèque lui-même. La séquence d'actions est assez simple et n'a pas pris beaucoup de temps:
- Elasticsearch téléchargé depuis GitHub ;
- J'ai suivi les instructions pour exécuter l'analyseur Java et exécuté l'analyse;
- Reçu le rapport de l'analyseur, s'y est plongé et a signalé des cas intéressants.
Passons maintenant au point principal.
Attention! Possible NullPointerException
V6008 Déréférence nulle de «ligne». GoogleCloudStorageFixture.java (451)
private static PathTrie<RequestHandler> defaultHandlers(....) { .... handlers.insert("POST /batch/storage/v1", (request) -> { ....
L'erreur dans ce fragment de code est que si la chaîne du tampon n'a pas été lue, l'appel de la méthode
startsWith dans la condition de l'instruction
if entraînera la
levée de l' exception
NullPointerException . Il s'agit très probablement d'une faute de frappe et lors de l'écriture d'une condition, les développeurs signifiaient l'opérateur
&& au lieu de
|| .
V6008 Déréférence nulle potentielle de '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
Un autre avertissement du diagnostic
V6008 . L'objet
followIndexMetadata a suscité mon intérêt. La méthode
start accepte plusieurs arguments en entrée, notre suspect en fait partie. Après cela, basé sur la vérification de notre objet pour
null, un nouvel objet est créé, qui est impliqué dans une logique de méthode supplémentaire. La vérification de
null nous montre que
followIndexMetadata peut toujours provenir de l'extérieur en tant qu'objet null. Eh bien, regardons plus loin.
Ensuite, plusieurs arguments sont poussés vers la méthode
validate (encore une fois, il y a notre objet considéré parmi eux). Si nous regardons la mise en œuvre de la méthode de validation, tout se met en place. Notre objet null potentiel est transmis à la méthode
validate comme troisième argument, où il est déréférencé sans condition. Résultat
NullPointerException potentiel.
static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex,
Nous ne savons pas avec certitude avec quels arguments la méthode de
démarrage est appelée. Il est fort possible que tous les arguments soient vérifiés quelque part avant d'appeler la méthode et aucune déréférence d'objet nul ne se produira. Quoi qu'il en soit, nous devons admettre qu'une telle implémentation de code semble toujours peu fiable et mérite notre attention.
V6060 La référence 'node' a été utilisée avant d'être vérifiée par rapport à null. RestTasksAction.java (152), RestTasksAction.java (151)
private void buildRow(Table table, boolean fullId, boolean detailed, DiscoveryNodes discoveryNodes, TaskInfo taskInfo) { .... DiscoveryNode node = discoveryNodes.get(nodeId); ....
Une autre règle de diagnostic avec le même problème s'est déclenchée ici.
NullPointerException . La règle crie pour les développeurs: "Les gars, que faites-vous? Comment as-tu pu faire ça? Oh, c'est affreux! Pourquoi utilisez-vous d'abord l'objet et vérifiez si
null dans la ligne suivante? Voici comment le déréférencement d'objet nul se produit. Hélas, même le commentaire d'un développeur n'a pas aidé.
V6060 La référence «cause» a été utilisée avant d'être vérifiée par rapport à null. 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();
Dans ce cas, nous devons tenir compte du fait que la méthode
getCause de la classe
Throwable peut retourner
null . Le problème ci-dessus se répète, son explication est donc inutile.
Des conditions insignifiantes
V6007 L' expression s.charAt (i)! = '\ T' 'est toujours vraie. Cron.java (1223)
private static int findNextWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) != ' ' || s.charAt(i) != '\t'); i++) {
La fonction considérée renvoie l'index du premier caractère d'espace, à partir de l'index
i . Qu'est-ce qui ne va pas? L'analyseur nous avertit que
s.charAt (i)! = '\ T' est toujours vrai, ce qui signifie que l'expression
(s.charAt (i)! = '' || s.charAt (i)! = '\ T ') sera toujours vrai aussi. Est-ce vrai? Je pense que vous pouvez facilement vous en assurer en remplaçant n'importe quel caractère.
Par conséquent, cette méthode retournera toujours l'index, égal à
s.length () , ce qui est faux. Je me risquerais à suggérer que la méthode ci-dessus est à blâmer ici:
private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) {
Les développeurs ont implémenté la méthode, puis ont copié et obtenu notre méthode erronée
findNextWhiteSpace, après avoir apporté quelques modifications. Ils ont continué à corriger et à corriger la méthode, mais ne l'ont pas corrigée. Pour bien faire les choses, il faut utiliser l'opérateur
&& au lieu de
|| .
V6007 L' expression «restant == 0» est toujours fausse. 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) {
D'après l'état de la boucle
copiée <keyLength , nous pouvons noter que cette
copie sera toujours inférieure à
keyLength . Par conséquent, il est inutile de comparer la variable restante avec 0, et ce sera toujours faux, auquel cas la boucle ne sortira pas par une condition. Supprimer le code ou reconsidérer la logique de comportement? Je pense que seuls les développeurs pourront mettre tous les points sur le i.
V6007 L' expression 'healthCheckDn.indexOf (' = ')> 0' est toujours fausse. 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 ....; }, ....); .... }
Une expression vide de sens. Selon la condition, la chaîne
healthCheckDn doit être à la fois vide et contenir le caractère «=» pas en première position, de sorte que l'expression lambda renvoie la variable de chaîne
healthCheckDn . Ouf, c'est tout alors! Comme vous l'avez probablement compris, c'est impossible. Nous n'allons pas aller au fond du code, laissons la discrétion aux développeurs.
Je n'ai cité que quelques exemples erronés, mais au-delà, il y avait beaucoup de déclenchements diagnostiques du
V6007 , qui devraient être considérés un par un, en tirant des conclusions pertinentes.
Une petite méthode peut aller loin
private static byte char64(char x) { if ((int)x < 0 || (int)x > index_64.length) return -1; return index_64[(int)x]; }
Nous avons donc ici une toute petite méthode de plusieurs lignes. Mais les bugs sont de garde! L'analyse de cette méthode a donné le résultat suivant:
- V6007 L'expression '(int) x <0' est toujours fausse. BCrypt.java (429)
- V6025 L'index '(int) x' est peut-être hors limites. BCrypt.java (431)
Problème N1. L'expression
(int) x <0 est toujours fausse (Oui,
V6007 à nouveau). La variable
x ne peut pas être négative, car elle est de type
char . Le type
char est un entier non signé. Elle ne peut pas être qualifiée d'erreur réelle, mais, néanmoins, le chèque est redondant et peut être supprimé.
Problème N2. Index de tableau possible hors limites, entraînant l'exception
ArrayIndexOutOfBoundsException . Ensuite, il pose la question, qui se trouve à la surface: "Attendez, que diriez-vous de la vérification de l'index?"
Nous avons donc un tableau de taille fixe de 128 éléments:
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 };
Lorsque la méthode
char64 reçoit la variable
x , la validité de l'index est vérifiée. Où est la faille? Pourquoi un index de tableau hors limites est-il toujours possible?
La vérification (int) x> index_64.length n'est pas tout à fait correcte. Si la méthode char64 reçoit x avec la valeur 128, la vérification ne protège pas contre ArrayIndexOutOfBoundsException. Peut-être que cela ne se produit jamais en réalité. Cependant, le chèque est mal écrit et il faut changer l'opérateur "supérieur à" (>) par "supérieur ou égal à (> =).
Les comparaisons, qui ont fait de leur mieux
V6013 Les nombres 'displaySize' et 'that.displaySize' sont comparés par référence. Une comparaison de l'égalité était peut-être envisagée. 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 &&
Ce qui est incorrect ici, c'est que les objets
displaySize de type
Integer sont comparés à l'aide de l'opérateur
== , c'est-à-dire par référence. Il est fort possible que les objets
ColumnInfo , dont les champs
displaySize aient des références différentes mais le même contenu, soient comparés. Dans ce cas, la comparaison nous donnera le résultat négatif, quand nous nous attendions à devenir vrai.
Je me risquerais à deviner qu'une telle comparaison pourrait être le résultat d'un refactoring échoué et initialement le champ
displaySize était de type
int .
V6058 La fonction 'equals' compare des objets de types incompatibles: 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()))
Comparaison d'objets incorrecte à nouveau. Cette fois, les objets avec des types incompatibles sont comparés (
Integer et
TimeValue ). Le résultat de cette comparaison est évident, et il est toujours faux. Vous pouvez voir que les champs de classe sont comparés les uns aux autres, seuls les noms des champs doivent être modifiés. Voici le point - un développeur a décidé d'accélérer le processus en utilisant le copier-coller et a obtenu un bug dans l'affaire. La classe implémente un getter pour le champ
scrollSize , donc pour corriger l'erreur, il faut utiliser la méthode
datafeed .getScrollSize ().Regardons quelques exemples erronés sans aucune explication. Les problèmes sont assez évidents.
V6001 Il existe des sous-expressions identiques 'takeInMillis' à gauche et à droite de l'opérateur '=='. 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
V6009 La fonction «égal» reçoit un argument impair. Un objet 'shardId.getIndexName ()' est utilisé comme argument de sa propre méthode. SnapshotShardFailure.java (208)
@Override public boolean equals(Object o) { .... return shardId.id() == that.shardId.id() && shardId.getIndexName().equals(shardId.getIndexName()) &&
Divers
V6006 L'objet a été créé mais il n'est pas utilisé. Le mot clé «throw» est peut-être manquant. JdbcConnection.java (88)
@Override public void setAutoCommit(boolean autoCommit) throws SQLException { checkOpen(); if (!autoCommit) { new SQLFeatureNotSupportedException(....); } }
Le bug est évident et ne nécessite aucune explication. Un développeur a créé une exception, mais ne l'a pas lancée ailleurs. Une telle exception anonyme est créée avec succès et, plus important encore, sera supprimée de manière transparente. La raison en est le manque de l'opérateur de
lancer .
V6003 L'utilisation du
modèle 'if (A) {....} else if (A) {....}' a été détectée. Il y a une probabilité de présence d'erreur logique. 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)) .... }
Dans le cas
contraire, l' une des conditions est répétée deux fois, de sorte que la situation nécessite un examen du code compétent.
V6039 Il existe deux instructions 'if' avec des expressions conditionnelles identiques. La première instruction «if» contient la méthode return. Cela signifie que la deuxième déclaration «si» est insensée. 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;
La même condition est utilisée deux fois de suite. La deuxième condition est-elle superflue ou faut-il utiliser un autre type au lieu de
booléen ?
V6009 La fonction 'substring' reçoit des arguments impairs. L'argument 'queryStringIndex + 1' ne doit pas être supérieur à 'queryStringLength'. 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,
Considérons ici le scénario erroné qui peut provoquer l'exception
StringIndexOutOfBoundsException . L'exception se produira lorsque
request.uri () renvoie une chaîne qui contient le caractère «#» avant «?». Il n'y a aucun contrôle dans la méthode, donc au cas où cela se produirait, le problème est en train de se préparer. Peut-être que cela ne se produira jamais en raison de diverses vérifications de l'objet en dehors de la méthode, mais placer des espoirs sur ce n'est pas la meilleure idée.
Conclusion
Depuis de nombreuses années, PVS-Studio aide à trouver des défauts dans le code des projets open source commerciaux et gratuits. Récemment, Java a rejoint la liste des langages pris en charge pour l'analyse. Elasticsearch est devenu l'un des premiers tests pour notre nouveau venu. Nous espérons que cette vérification sera utile pour le projet et intéressante pour les lecteurs.
PVS-Studio pour Java a besoin de nouveaux défis, de nouveaux utilisateurs, de commentaires actifs et de clients afin de s'adapter rapidement au nouveau monde :). Je vous invite donc à
télécharger et essayer notre analyseur sur votre projet de travail tout de suite!