Loin de la première année, l'équipe PVS-Studio a blogué sur les contrôles de projets open-source par l'analyseur de code statique du même nom. À ce jour, plus de 300 projets ont été vérifiés et plus de 12 000 cas ont été enregistrés dans la base de données des erreurs trouvées. Initialement, l'analyseur a été implémenté pour tester le code C et C ++, puis le support du langage C # est apparu. Par conséquent, parmi les projets testés, la majorité (> 80%) tombe sur C et C ++. Plus récemment, Java a été ajouté aux langages pris en charge, ce qui signifie que PVS-Studio ouvre les portes à un nouveau monde, et il est temps de compléter la base de données avec des erreurs de projets Java.
Le monde Java est immense et diversifié, donc mes yeux s'écarquillent lorsque je choisis un projet pour tester un nouvel analyseur. En fin de compte, le choix s'est porté sur le moteur de recherche en texte intégral et l'analyse Elasticsearch. C'est un projet assez réussi, et dans les projets réussis, trouver des erreurs est doublement, voire triple, plus agréable. Quels sont les défauts détectés par PVS-Studio pour Java? Le résultat de la vérification sera discuté dans l'article.
Connaissance superficielle d'Elasticsearch
Elasticsearch est un moteur de recherche et d'analyse en plein texte évolutif et open source. Il vous permet de stocker de grandes quantités de données, d'effectuer parmi elles une recherche et des analyses rapides (presque en 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, Wikimedia, StumbleUpon, Quora, Foursquare, SoundCloud, GitHub, Netflix, Amazon, IBM, Qbox sont notés.
Je pense que la connaissance suffit.
Comment c'était
Il n'y a eu aucun problème avec la vérification. La séquence d'actions est assez simple et n'a pas pris beaucoup de temps:
- Elasticsearch téléchargé depuis GitHub ;
- J'ai utilisé les instructions pour démarrer l'analyseur Java et lancé l'analyse;
- J'ai reçu un rapport d'analyseur, je l'ai analysé et j'ai mis en évidence des cas intéressants.
Venons-en maintenant au fait.
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 s'ils ne pouvaient pas lire la ligne à partir du tampon, alors appeler la méthode
startsWith dans la condition de l'
instruction if lèverait une
NullPointerException . Il s'agit très probablement d'une faute de frappe, et lors de l'écriture de la condition, l'opérateur
&& était destiné 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 . Maintenant, un examen plus attentif a riveté l'objet
followIndexMetadata . La méthode
start prend plusieurs arguments d'entrée, y compris notre suspect. Ensuite, sur la base de la vérification de la
nullité de notre objet, un nouvel objet est formé, qui participe à la logique ultérieure de la méthode. La vérification de
null nous indique que
followIndexMetadata peut toujours provenir de l'extérieur avec un objet null. Ok, regardez plus loin.
Ensuite, la méthode de validation est appelée en poussant de nombreux arguments (encore une fois, parmi lesquels se trouve l'objet en question). Et si vous regardez la mise en œuvre de la méthode de validation, alors tout se met en place. Notre objet nul potentiel est passé par le troisième argument à la méthode
validate , où il est déréférencé inconditionnellement. En conséquence, une
NullPointerException potentielle
. static void validate( final ResumeFollowAction.Request request, final IndexMetaData leaderIndex, final IndexMetaData followIndex,
On ne sait pas avec quels arguments la méthode de
démarrage est réellement appelée. Il est possible que la vérification de tous les arguments se fasse quelque part avant l'appel de méthode, et nous ne rencontrons aucun déréférencement de l'objet null. Mais, vous devez admettre qu'une telle implémentation de code semble encore peu fiable et mérite l'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 a fonctionné ici, mais le problème est le même:
NullPointerException . La règle dit: «Les gars, que faites-vous? Comment ça? Oh trouble! Pourquoi utilisez-vous d'abord l'objet, puis dans la ligne de code suivante, vérifiez qu'il est
nul ?! ” Il s'avère donc ici de déréférencer un objet nul. Hélas, même le commentaire de l'un des développeurs 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();
Il convient de noter ici que la méthode
getCause de la classe
Throwable peut retourner
null . De plus, le problème considéré ci-dessus se répète et il est inutile d'expliquer quelque chose en détail.
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 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 qu'il y aura toujours la vérité et l'expression
(s.charAt (i)! = '' || s.charAt (i)! = '\ t') . En est-il ainsi? Je pense que vous-même pouvez facilement vérifier cela en remplaçant n'importe quel caractère.
Par conséquent, cette méthode retournera toujours un index égal à
s.length () , ce qui n'est pas vrai. J'ose supposer que cette méthode située légèrement plus haut est à blâmer:
private static int skipWhiteSpace(int i, String s) { for (; i < s.length() && (s.charAt(i) == ' ' || s.charAt(i) == '\t'); i++) {
Nous avons implémenté cette méthode, puis l'avons copiée et avons apporté quelques corrections mineures pour obtenir notre méthode
findNextWhiteSpace erronée. La méthode a été ajustée, ajustée, mais pas ajustée. Pour remédier à la situation, vous devez 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) {
De la condition du cycle
copié <keyLength, on peut noter que
copié sera toujours inférieur à
keyLength . Par conséquent, une comparaison sur l'égalité de la variable
restante avec 0 est inutile et donnera toujours un faux résultat, et donc la condition ne sera pas sortie de la boucle. Ce code vaut-il la peine d'être supprimé ou devez-vous reconsidérer la logique du comportement? Je pense que seuls les développeurs seront en mesure de dot tout ce que je.
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 ....; }, ....); .... }
Encore une expression dénuée de sens. Selon la condition, pour que l'expression lambda renvoie la variable de chaîne
healthCheckDn , la chaîne
healthCheckDn doit être à la fois vide et la chaîne contenant le caractère «=» n'est pas en première position. Fuh, en quelque sorte réglé. Et comme vous l'avez bien compris, c'est impossible. Nous ne comprendrons pas la logique du code, laissez-le aux développeurs.
Je n'ai donné que quelques exemples erronés, mais en plus de cela, il y avait de nombreux cas où les diagnostics du
V6007 ont été
déclenchés , qui doivent être considérés séparément et des conclusions tirées.
La méthode est petite, mais udalenky
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 une petite méthode de plusieurs lignes. Mais les insectes ne dorment pas! Une 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, oui, encore
V6007 ). La variable
x ne peut pas être négative, car elle est de type
char . Le type
char est un entier non signé. Cela ne peut pas être qualifié de véritable erreur, mais, néanmoins, le chèque est redondant et peut être supprimé.
Problème N2.
Débordement possible du tableau conduisant à une
ArrayIndexOutOfBoundsException . Ensuite, la question se pose, qui se trouve à la surface: "Attendez, qu'en est-il 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 variable
x entre en entrée de la méthode
char64 , elle vérifie la validité de l'index. Où est l'écart? Pourquoi est-il toujours possible de sortir de la baie?
La vérification
(int) x> index_64.length n'est pas entièrement correcte. Si
x avec une valeur de 128
arrive à l'entrée de la méthode
char64 , la vérification ne protégera pas contre
ArrayIndexOutOfBoundsException . Peut-être que cela ne se produit jamais en réalité. Toutefois, la vérification n'est pas correctement orthographiée et vous devez remplacer l'opérateur "supérieur à" (>) par "supérieur ou égal à" (> =).
Des comparaisons qui ont essayé
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 &&
L'erreur ici est que les objets
displaySize de type
Integer sont comparés via l'opérateur
== , c'est-à-dire qu'ils sont comparés par référence. Il est tout à fait possible que les objets
ColumnInfo soient comparés, pour lesquels les champs
displaySize ont des liens différents mais le même contenu. Et dans ce cas, la comparaison nous donnera un résultat négatif, alors que nous nous attendions à la vérité.
Je me risquerais à suggérer 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()))
Et encore une comparaison incorrecte des objets. Comparez maintenant les objets dont les types sont incompatibles (
Integer et
TimeValue ). Le résultat de cette comparaison est évident, et il est toujours faux. On peut voir que les champs de la classe sont comparés de la même manière entre eux, il suffit de changer les noms de champs. Ainsi, le développeur a décidé d'accélérer le processus d'écriture de code avec du copier-coller, mais il s'est attribué le même bug. La classe implémente un getter pour le champ
scrollSize , donc pour corriger l'erreur, la bonne solution serait d'utiliser la méthode appropriée:
datafeed .getScrollSize () .
Regardons quelques autres exemples d'erreurs sans aucune explication. Le problème est déjà évident.
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. Le développeur a levé une exception, mais ne la jette en aucun cas plus loin. Une telle exception anonyme sera créée avec succès, mais aussi avec succès et, surtout, détruite sans laisser de trace. La raison en est l'absence de déclaration 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 une
construction if-else multiple
, l' une des conditions est répétée deux fois, de sorte que la situation nécessite une révision compétente du code.
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 superflue, ou est-il nécessaire d'utiliser un type différent 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érez immédiatement un scénario d'erreur qui pourrait
lever une
StringIndexOutOfBoundsException . Une exception se produit lorsque
request.uri () renvoie une chaîne contenant le caractère «#» avant «?». Dans un tel cas, il n'y a pas de vérification de la méthode, et si cela se produit toujours, le désastre ne sera pas évité. Peut-être que cela ne se produira jamais en raison de diverses vérifications de l'objet de
demande en dehors de la méthode, mais à mon avis, espérer que ce n'est pas la meilleure idée.
Conclusion
Au fil des ans, PVS-Studio a aidé à trouver des failles dans le code des projets open source commerciaux et gratuits. Plus récemment, Java a été ajouté pour prendre en charge les langages analysés. Et l'un des premiers tests pour notre nouveau venu a été le développement actif d'Elasticsearch. Nous espérons que cette vérification sera utile pour le projet et intéressante pour les lecteurs.
Pour que PVS-Studio pour Java s'adapte rapidement à un nouveau monde pour lui-même, nous avons besoin de nouveaux tests, de nouveaux utilisateurs, de commentaires actifs et de clients :). Je vous propose donc, sans tarder, de
télécharger et tester notre analyseur sur votre projet de travail!
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Maxim Stefanov.
PVS-Studio pour Java prend la route. Le prochain arrêt est Elasticsearch