Geekly articles weekly

Recherche d'erreurs dans le code source du SDK Amazon Web Services pour .NET

Image 1


Bienvenue à tous les fans de la corbeille du code de quelqu'un d'autre. :) Aujourd'hui, dans notre laboratoire, nous avons un nouveau matériel pour une recherche - le code source du projet AWS SDK for .NET. À l'époque, nous avons écrit un article sur la vérification d'AWS SDK pour C ++. Il n'y avait alors rien de particulièrement intéressant. Voyons ce que vaut .NET de la version AWS SDK. Encore une fois, c'est une excellente occasion de démontrer les capacités de l'analyseur PVS-Studio et de rendre le monde un peu meilleur.

Le SDK Amazon Web Services (AWS) pour .NET est un ensemble d'outils de développeur, destiné à créer des applications basées sur .NET dans l'infrastructure AWS. Cet ensemble permet de simplifier considérablement le processus d'écriture de code. Le SDK comprend des ensembles API .NET pour divers services AWS, tels que Amazon S3, Amazon EC2, DynamoDB et autres. Le code source du SDK est disponible sur GitHub .

Comme je l'ai mentionné, à l'époque, nous avions déjà écrit l' article sur la vérification d'AWS SDK pour C ++. L'article s'est avéré être petit - seulement quelques erreurs trouvées pour 512 000 lignes de code. Cette fois, nous avons affaire à une taille beaucoup plus grande du code, qui comprend environ 34 000 fichiers cs, et le nombre total de lignes de code (à l'exclusion des lignes vides) est impressionnant de 5 millions. Une petite partie du code (200 000 lignes dans des fichiers 664-cs) revient aux tests, je ne les ai pas pris en compte.

Si la qualité du code .NET de la version SDK est approximativement la même que celle de C ++ (deux erreurs par 512 KLOC), alors nous devrions obtenir un nombre d'erreurs environ 10 fois plus élevé. Bien sûr, il s'agit d'une méthodologie de calcul très inexacte, qui ne prend pas en compte les particularités linguistiques et de nombreux autres facteurs, mais je ne pense pas que le lecteur veuille maintenant entrer dans un raisonnement ennuyeux. Au lieu de cela, je suggère de passer aux résultats.

La vérification a été effectuée à l'aide de PVS-Studio 6.27. C'est tout simplement incroyable, mais le fait est que dans l'AWS SDK pour .NET, l'analyseur a réussi à détecter 40 erreurs, ce qui vaut la peine d'en parler. Il démontre non seulement une haute qualité du code SDK (environ 4 erreurs pour 512 KLOC), mais également une qualité comparable de l'analyseur C # PVS-Studio par rapport à C ++. Un super résultat!

Auteurs d'AWS SDK pour .NET, vous êtes de vrais champions! Avec chaque projet, vous démontrez une qualité exceptionnelle du code. Cela peut être un excellent exemple pour d'autres équipes. Cependant, bien sûr, je ne serais pas un développeur d'un analyseur statique, si je ne donnais pas mes 2 cents. :) Nous travaillons déjà avec une équipe Lumberyard d'Amazon sur l'utilisation de PVS-Studio. Comme il s'agit d'une très grande entreprise avec un tas d'unités dans le monde, il est très probable que l'équipe AWS SDK pour .NET n'ait jamais entendu parler de PVS-Studio. Quoi qu'il en soit, je n'ai trouvé aucun signe d'utilisation de notre analyseur dans le code SDK, bien qu'il ne dise rien. Cependant, au moins, l'équipe utilise l'analyseur intégré à Visual Studio. C'est génial, mais les révisions de code peuvent toujours être améliorées :).

En conséquence, j'ai réussi à trouver quelques bugs dans le code SDK et, enfin, il est temps de les partager.

Erreur de logique

Avertissement PVS-Studio: V3008 [CWE-563] La variable 'this.linker.s3.region' reçoit des valeurs successives deux fois. C'est peut-être une erreur. Vérifiez les lignes: 116, 114. AWSSDK.DynamoDBv2.Net45 S3Link.cs 116

public string Region { get { .... } set { if (String.IsNullOrEmpty(value)) { this.linker.s3.region = "us-east-1"; } this.linker.s3.region = value; } }

L'analyseur met en garde contre l'attribution répétée de valeurs à la même variable. À partir du code, il devient clair que cela est dû à l'erreur qui viole la logique du travail du programme: la valeur de la variable this.linker.s3.region sera toujours égale à la valeur de la valeur de la variable, quelle que soit la condition if (String.IsNullOrEmpty (value)) . La déclaration de retour a été manquée dans le corps du bloc if . Le code doit être corrigé comme suit:

 public string Region { get { .... } set { if (String.IsNullOrEmpty(value)) { this.linker.s3.region = "us-east-1"; return; } this.linker.s3.region = value; } }

Récursion infinie

Avertissement PVS-Studio: V3110 [CWE-674] Récursion infinie possible dans la propriété 'OnFailure'. AWSSDK.ElasticMapReduce.Net45 ResizeJobFlowStep.cs 171

 OnFailure? onFailure = null; public OnFailure? OnFailure { get { return this.OnFailure; } // <= set { this.onFailure = value; } }

Un exemple classique d'une faute de frappe, qui conduit à une récursion infinie dans l'accesseur get de la propriété OnFailure . Au lieu de renvoyer la valeur d'un champ privé onFailure, l'accès à la propriété OnFailure a lieu. Variante correcte:

 public OnFailure? OnFailure { get { return this.onFailure; } set { this.onFailure = value; } }

Vous pouvez demander: "Comment cela a-t-il fonctionné?" Jusqu'à présent - pas comment. La propriété n'est utilisée nulle part ailleurs, mais c'est temporaire. À un moment donné, quelqu'un commencera à l'utiliser et recevra certainement un résultat inattendu. Pour éviter de telles fautes de frappe, il est recommandé de ne pas utiliser d'identifiants qui ne diffèrent que dans le cas de la première lettre.

Un autre commentaire à cette construction est l'utilisation de l'identifiant, qui correspond complètement au nom du type OnFailure . Du point de vue du compilateur, c'est tout à fait acceptable, mais cela complique la perception du code pour une personne.

Une autre erreur similaire:

Avertissement PVS-Studio: V3110 [CWE-674] Possible récursion infinie à l'intérieur de la propriété 'SSES3'. AWSSDK.S3.Net45 InventoryEncryption.cs 37

 private SSES3 sSES3; public SSES3 SSES3 { get { return this.SSES3; } set { this.SSES3 = value; } }

La situation est identique à celle décrite ci-dessus. Cependant, ici une récursion infinie se produira lors de l'accès à la propriété SSES3 à la fois pour la lecture et l'attribution. Variante correcte:

 public SSES3 SSES3 { get { return this.sSES3; } set { this.sSES3 = value; } }

Test sur considération

Maintenant, je voudrais citer une tâche d'un développeur, prise en utilisant la méthode Copy-Paste. Jetez un œil à l'apparence du code dans l'éditeur Visual Studio et essayez de trouver une erreur.

Image 3


Avertissement PVS-Studio: V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 91, 95. AWSSDK.AppSync.Net45 CreateApiKeyResponseUnmarshaller.cs 91

J'ai réduit le corps de la méthode UnmarshallException , après avoir supprimé tout ce qui n'était pas nécessaire. Vous pouvez maintenant voir que des vérifications identiques se succèdent:

 public override AmazonServiceException UnmarshallException(....) { .... if (errorResponse.Code != null && errorResponse.Code.Equals("LimitExceededException")) { return new LimitExceededException(errorResponse.Message, innerException, errorResponse.Type, errorResponse.Code, errorResponse.RequestId, statusCode); } if (errorResponse.Code != null && errorResponse.Code.Equals("LimitExceededException")) { return new LimitExceededException(errorResponse.Message, innerException, errorResponse.Type, errorResponse.Code, errorResponse.RequestId, statusCode); } .... }

Il peut sembler que le bogue n'est pas brutal - juste une vérification supplémentaire. Néanmoins, souvent un tel schéma peut indiquer des problèmes plus graves dans le code, lorsqu'une vérification nécessaire ne sera pas effectuée.

Dans le code, il existe plusieurs erreurs similaires.

Avertissements de PVS-Studio:

  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 75, 79. AWSSDK.CloudDirectory.Net45 CreateSchemaResponseUnmarshaller.cs 75
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 105, 109. AWSSDK.CloudDirectory.Net45 GetSchemaAsJsonResponseUnmarshaller.cs 105
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 201, 205. AWSSDK.CodeCommit.Net45 PostCommentForPullRequestResponseUnmarshaller.cs 201
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 101, 105. AWSSDK.CognitoIdentityProvider.Net45 VerifySoftwareTokenResponseUnmarshaller.cs 101
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 72, 76. AWSSDK.Glue.Net45 UpdateConnectionResponseUnmarshaller.cs 72
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 123, 127. AWSSDK.Neptune.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 123
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 167, 171. AWSSDK.Neptune.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 167
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 127, 131. AWSSDK.RDS.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 127
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 171, 175. AWSSDK.RDS.Net45 RestoreDBClusterFromSnapshotResponseUnmarshaller.cs 171
  • V3029 Les expressions conditionnelles des instructions «if» situées côte à côte sont identiques. Vérifiez les lignes: 99, 103. AWSSDK.Rekognition.Net45 RecognizeCelebritiesResponseUnmarshaller.cs 99

Qu'est ce que c'est?

Avertissement PVS-Studio: V3062 Un objet 'attributeName' est utilisé comme argument de sa propre méthode. Pensez à vérifier le premier argument réel de la méthode «Contient». AWSSDK.MobileAnalytics.Net45 CustomEvent.cs 261

 /// <summary> /// Dictionary that stores attribute for this event only. /// </summary> private Dictionary<string,string> _attributes = new Dictionary<string,string>(); /// <summary> /// Gets the attribute. /// </summary> /// <param name="attributeName">Attribute name.</param> /// <returns>The attribute. Return null of attribute doesn't /// exist.</returns> public string GetAttribute(string attributeName) { if(string.IsNullOrEmpty(attributeName)) { throw new ArgumentNullException("attributeName"); } string ret = null; lock(_lock) { if(attributeName.Contains(attributeName)) // <= ret = _attributes[attributeName]; } return ret; }

L'analyseur a détecté une erreur dans la méthode GetAttribute : une chaîne est vérifiée si elle se contient. D'après la description de la méthode, il s'ensuit que si le nom d'attribut (clé d' attributName ) est trouvé (dans le dictionnaire _attributes ), la valeur d'attribut doit être retournée, sinon - null . En fait, comme la condition attributeName.Contains (attributeName) est toujours vraie, une tentative est faite pour renvoyer la valeur par une clé qui pourrait ne pas être trouvée dans un dictionnaire. Ensuite, au lieu de renvoyer null, une exception KeyNotFoundException sera levée.

Essayons de corriger ce code. Pour mieux comprendre comment procéder, vous devez envisager une autre méthode:

 /// <summary> /// Determines whether this instance has attribute the specified /// attributeName. /// </summary> /// <param name="attributeName">Attribute name.</param> /// <returns>Return true if the event has the attribute, else /// false.</returns> public bool HasAttribute(string attributeName) { if(string.IsNullOrEmpty(attributeName)) { throw new ArgumentNullException("attributeName"); } bool ret = false; lock(_lock) { ret = _attributes.ContainsKey(attributeName); } return ret; }

Cette méthode vérifie si le nom d'attribut (clé attributeName ) existe dans le dictionnaire _attributes . Revenons à la méthode GetAttribute et corrigeons l'erreur:

 public string GetAttribute(string attributeName) { if(string.IsNullOrEmpty(attributeName)) { throw new ArgumentNullException("attributeName"); } string ret = null; lock(_lock) { if(_attributes.ContainsKey(attributeName)) ret = _attributes[attributeName]; } return ret; }

Maintenant, la méthode fait exactement ce qui est indiqué dans la description.

Encore un petit commentaire sur ce fragment de code. J'ai remarqué que les auteurs utilisent le verrouillage lorsqu'ils travaillent avec le dictionnaire _attributes . Il est clair que cela est nécessaire lorsque vous disposez d'un accès multithread, mais la construction de la serrure est plutôt lente et encombrante. Au lieu d'un dictionnaire , dans ce cas, il serait peut-être plus pratique d'utiliser la version thread-safe du dictionnaire - ConcurrentDictionary . De cette façon, il n'y aura pas besoin de verrou. Mais je ne connais peut-être pas les spécificités du projet.

Comportement suspect

Avertissement PVS-Studio: V3063 [CWE-571] Une partie de l'expression conditionnelle est toujours vraie si elle est évaluée: string.IsNullOrEmpty (inferredIndexName). AWSSDK.DynamoDBv2.PCL ContextInternal.cs 802

 private static string GetQueryIndexName(....) { .... string inferredIndexName = null; if (string.IsNullOrEmpty(specifiedIndexName) && indexNames.Count == 1) { inferredIndexName = indexNames[0]; } else if (indexNames.Contains(specifiedIndexName, StringComparer.Ordinal)) { inferredIndexName = specifiedIndexName; } else if (string.IsNullOrEmpty(inferredIndexName) && // <= indexNames.Count > 0) throw new InvalidOperationException("Local Secondary Index range key conditions are used but no index could be inferred from model. Specified index name = " + specifiedIndexName); .... }

L'analyseur était préoccupé par la chaîne de contrôle.IsNullOrEmpty (inferredIndexName) . En effet, la chaîne inferredIndexName est affectée null , puis la valeur de cette variable n'est modifiée nulle part, puis pour une raison quelconque, elle est vérifiée pour null ou une chaîne vide. Semble suspect. Examinons de près le fragment de code ci-dessus. Je ne l'ai pas délibérément réduit pour mieux comprendre la situation. Ainsi, dans la première instruction if (et également dans la suivante), la variable specifiedIndexName est en quelque sorte vérifiée. En fonction des résultats des vérifications, la variable inferredIndexName obtient une nouvelle valeur. Examinons maintenant la troisième instruction if . Le corps de cette instruction (levée de l'exception) sera exécuté au cas où indexNames.Count> 0, comme première partie de la condition entière, qui est string.IsNullOrEmpty (inferredIndexName) est toujours vrai. Peut-être que les variables specifiedIndexName et inferredIndexName sont mélangées ou que la troisième vérification doit être sans autre , représentant une instruction if autonome:

 if (string.IsNullOrEmpty(specifiedIndexName) && indexNames.Count == 1) { inferredIndexName = indexNames[0]; } else if (indexNames.Contains(specifiedIndexName, StringComparer.Ordinal)) { inferredIndexName = specifiedIndexName; } if (string.IsNullOrEmpty(inferredIndexName) && indexNames.Count > 0) throw new InvalidOperationException(....);

Dans ce cas, il est difficile de donner une réponse définitive sur les options pour corriger ce code. Quoi qu'il en soit, l'auteur doit le vérifier.

NullReferenceException

Avertissement PVS-Studio: V3095 [CWE-476] L'objet 'conditionValues' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 228, 238. AWSSDK.Core.Net45 JsonPolicyWriter.cs 228

 private static void writeConditions(....) { .... foreach (....) { IList<string> conditionValues = keyEntry.Value; if (conditionValues.Count == 0) // <= continue; .... if (conditionValues != null && conditionValues.Count != 0) { .... } .... } }

C'est un classique. La variable conditionValues est utilisée sans vérification préalable de null . Plus tard dans le code, cette vérification est effectuée. Le code doit être corrigé comme suit:

 private static void writeConditions(....) { .... foreach (....) { IList<string> conditionValues = keyEntry.Value; if (conditionValues != null && conditionValues.Count == 0) continue; .... if (conditionValues != null && conditionValues.Count != 0) { .... } .... } }

J'ai trouvé plusieurs erreurs similaires dans le code.

Avertissements de PVS-Studio:

  • V3095 [CWE-476] L'objet 'ts.Listeners' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 140, 143. AWSSDK.Core.Net45 Logger.Diagnostic.cs 140
  • V3095 [CWE-476] L'objet 'obj' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 743, 745. AWSSDK.Core.Net45 JsonMapper.cs 743
  • V3095 [CWE-476] L'objet 'multipartUploadMultipartUploadpartsList' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 65, 67. AWSSDK.S3.Net45 CompleteMultipartUploadRequestMarshaller.cs 65

L'avertissement suivant est très similaire dans sa signification, mais le cas est opposé à celui discuté ci-dessus.

Avertissement PVS-Studio: V3125 [CWE-476] L'objet 'état' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 139, 127. AWSSDK.Core.Net45 RefreshingAWSCredentials.cs 139

 private void UpdateToGeneratedCredentials( CredentialsRefreshState state) { string errorMessage; if (ShouldUpdate) { .... if (state == null) errorMessage = "Unable to generate temporary credentials"; else .... throw new AmazonClientException(errorMessage); } state.Expiration -= PreemptExpiryTime; // <= .... }

L'un des fragments de code comprend la vérification de la valeur de la variable d' état pour null . Dans le code ci-dessous, la variable est utilisée pour se désinscrire de l'événement PreemptExpiryTime , cependant, une vérification de null n'est plus effectuée et le lancement de l'exception NullReferenceException devient possible. Une version plus sécurisée du code:

 private void UpdateToGeneratedCredentials( CredentialsRefreshState state) { string errorMessage; if (ShouldUpdate) { .... if (state == null) errorMessage = "Unable to generate temporary credentials"; else .... throw new AmazonClientException(errorMessage); } if (state != null) state.Expiration -= PreemptExpiryTime; .... }

Dans le code, il existe d'autres erreurs similaires:

Avertissements de PVS-Studio:

  • V3125 [CWE-476] L'objet 'wrappedRequest.Content' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 395, 383. AWSSDK.Core.Net45 HttpHandler.cs 395
  • V3125 [CWE-476] L'objet 'datasetUpdates' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 477, 437. AWSSDK.CognitoSync.Net45 Dataset.cs 477
  • V3125 [CWE-476] L'objet 'cORSConfigurationCORSConfigurationcORSRulesListValue' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 125, 111. AWSSDK.S3.Net45 PutCORSConfigurationRequestMarshaller.cs 125
  • V3125 [CWE-476] L'objet 'lifecycleConfigurationLifecycleConfigurationrulesListValue' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 157, 68. AWSSDK.S3.Net45 PutLifecycleConfigurationRequestMarshaller.cs 157
  • V3125 [CWE-476] L'objet 'this.Key' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 199, 183. AWSSDK.S3.Net45 S3PostUploadRequest.cs 199

Réalité non alternative

Avertissement PVS-Studio: V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'true'. AWSSDK.Core.Net45 Lexer.cs 651

 private static bool State19 (....) { while (....) { switch (....) { case '"': .... return true; case '\\': .... return true; default: .... continue; } } return true; }

La méthode renvoie toujours true . Voyons à quel point c'est critique pour le code appelant. J'ai vérifié les cas d'utilisation de la méthode State19 . Il est impliqué dans le remplissage du tableau de gestionnaires fsm_handler_table également avec d'autres méthodes similaires (il y en a 28 avec les noms, respectivement, en commençant par State1 à State28 ). Ici, il est important de noter qu'en plus de State19 , pour certains autres gestionnaires, les avertissements V3009 [CWE-393] ont également été émis. Ce sont des gestionnaires: State23, State26, State27, State28 . Les avertissements, émis par l'analyseur pour eux:

  • V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'true'. AWSSDK.Core.Net45 Lexer.cs 752
  • V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'true'. AWSSDK.Core.Net45 Lexer.cs 810
  • V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'true'. AWSSDK.Core.Net45 Lexer.cs 822
  • V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'true'. AWSSDK.Core.Net45 Lexer.cs 834

Voici à quoi ressemblent la déclaration et l'initialisation du tableau des gestionnaires:

 private static StateHandler[] fsm_handler_table; .... private static void PopulateFsmTables () { fsm_handler_table = new StateHandler[28] { State1, State2, .... State19, .... State23, .... State26, State27, State28 };

Pour compléter l'image, voyons le code de l'un des gestionnaires pour lesquels l'analyseur n'a eu aucune revendication, par exemple, State2 :

 private static bool State2 (....) { .... if (....) { return true; } switch (....) { .... default: return false; } }

Voici comment l'appel des gestionnaires se produit:

 public bool NextToken () { .... while (true) { handler = fsm_handler_table[state - 1]; if (! handler (fsm_context)) // <= throw new JsonException (input_char); .... } .... }

Comme nous pouvons le voir, une exception sera levée en cas de retour faux . Dans notre cas, pour les gestionnaires State19, State23, State26 State27 et State28, cela ne se produira jamais. Semble suspect. D'un autre côté, cinq gestionnaires ont un comportement similaire (retourneront toujours vrai ), alors peut-être que c'était tellement artificiel et n'est pas le résultat d'une faute de frappe.

Pourquoi vais-je si profondément dans tout cela? Cette situation est très significative dans le sens où l'analyseur statique ne peut souvent indiquer qu'une construction suspecte. Et même une personne (pas une machine), qui n'a pas suffisamment de connaissances sur le projet, n'est toujours pas en mesure de donner une réponse complète sur la présence de l'erreur, même après avoir passé du temps à apprendre le code. Un développeur doit revoir ce code.

Des contrôles sans signification

Avertissement PVS-Studio: V3022 [CWE-571] L'expression 'doLog' est toujours vraie. AWSSDK.Core.Net45 StoredProfileAWSCredentials.cs 235

 private static bool ValidCredentialsExistInSharedFile(....) { .... var doLog = false; try { if (....) { return true; } else { doLog = true; } } catch (InvalidDataException) { doLog = true; } if (doLog) // <= { .... } .... }

Faites attention à la variable doLog . Après l'initialisation avec la fausse valeur, cette variable obtiendra la vraie valeur dans tous les cas plus loin dans le code. Par conséquent, la vérification si (doLog) est toujours vraie. Peut-être, plus tôt dans la méthode, il y avait une branche, dans laquelle la variable doLog n'avait reçu aucune valeur. Au moment de la vérification, il pourrait contenir la fausse valeur reçue lors de l'initialisation. Mais maintenant, il n'y a pas une telle branche.

Une autre erreur similaire:

Avertissement PVS-Studio: V3022 L' expression '! Résultat' est toujours fausse. AWSSDK.CognitoSync.PCL SQLiteLocalStorage.cs 353

 public void PutValue(....) { .... bool result = PutValueHelper(....); if (!result) <= { _logger.DebugFormat("{0}", @"Cognito Sync - SQLiteStorage - Put Value Failed"); } else { UpdateLastModifiedTimestamp(....); } .... }

L'analyseur prétend que la valeur de la variable de résultat est toujours vraie. Cela n'est possible que si la méthode PutValueHelper renvoie toujours true . Jetez un œil à cette méthode:

 private bool PutValueHelper(....) { .... if (....)) { return true; } if (record == null) { .... return true; } else { .... return true; } }

En effet, la méthode retournera true dans toutes les conditions. De plus, l'analyseur a émis un avertissement pour cette méthode. Avertissement PVS-Studio: V3009 [CWE-393] Il est étrange que cette méthode renvoie toujours une seule et même valeur de 'true'. SQLiteLocalStorage.cs 1016

Je n'ai délibérément pas cité cet avertissement plus tôt lorsque je cherchais d'autres bogues V3009 et l'ai enregistré pour ce cas. Ainsi, l'outil a eu raison de signaler l'erreur V3022 dans le code appelant.

Copiez-collez. Encore une fois

Avertissement PVS-Studio: V3001 Il existe des sous-expressions identiques 'this.token == JsonToken.String' à gauche et à droite de '||' opérateur. AWSSDK.Core.Net45 JsonReader.cs 343

 public bool Read() { .... if ( (this.token == JsonToken.ObjectEnd || this.token == JsonToken.ArrayEnd || this.token == JsonToken.String || // <= this.token == JsonToken.Boolean || this.token == JsonToken.Double || this.token == JsonToken.Int || this.token == JsonToken.UInt || this.token == JsonToken.Long || this.token == JsonToken.ULong || this.token == JsonToken.Null || this.token == JsonToken.String // <= )) { .... } .... }

Le champ this.token est comparé deux fois avec la valeur JsonToken.String de l'énumération JsonToken . Probablement, l'une des comparaisons devrait contenir une autre valeur d'énumération. Si c'est le cas, une grave erreur a été commise ici.

Refactorisation + inattention?

Avertissement PVS-Studio: V3025 [CWE-685] Format incorrect. Un nombre différent d'éléments de format est attendu lors de l'appel de la fonction 'Format'. Arguments non utilisés: AWSConfigs.AWSRegionKey. AWSSDK.Core.Net45 AWSRegion.cs 116

 public InstanceProfileAWSRegion() { .... if (region == null) { throw new InvalidOperationException( string.Format(CultureInfo.InvariantCulture, "EC2 instance metadata was not available or did not contain region information.", AWSConfigs.AWSRegionKey)); } .... }

Peut-être que la chaîne de format de la méthode string.Format contenait précédemment l'élément de format {0}, pour lequel l'argument AWSConfigs.AWSRegionKey a été défini. Ensuite, la chaîne a été modifiée, l'élément de format a disparu, mais un développeur a oublié de supprimer l'argument. L'exemple de code donné fonctionne sans erreur (l'exception a été levée dans le cas contraire - l'élément de format sans l'argument), mais il n'a pas l'air sympa. Le code doit être corrigé comme suit:

 if (region == null) { throw new InvalidOperationException( "EC2 instance metadata was not available or did not contain region information."); }

Dangereux

Avertissement PVS-Studio: V3083 [CWE-367] L'appel non sécurisé de l'événement 'mOnSyncSuccess', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.CognitoSync.PCL Dataset.cs 827

 protected void FireSyncSuccessEvent(List<Record> records) { if (mOnSyncSuccess != null) { mOnSyncSuccess(this, new SyncSuccessEventArgs(records)); } }

Une situation courante d'un appel non sécurisé du gestionnaire d'événements. Un utilisateur peut se désinscrire entre la vérification de la variable mOnSyncSuccess pour null et l'appel d'un gestionnaire, de sorte que sa valeur deviendra nulle . La probabilité d'un tel scénario est faible, mais il vaut toujours mieux sécuriser le code:

 protected void FireSyncSuccessEvent(List<Record> records) { mOnSyncSuccess?.Invoke(this, new SyncSuccessEventArgs(records)); }

Dans le code, il existe d'autres erreurs similaires:

Avertissements de PVS-Studio:

  • V3083 [CWE-367] L'appel non sécurisé de l'événement 'mOnSyncFailure', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.CognitoSync.PCL Dataset.cs 839
  • V3083 [CWE-367] L'appel d'événement à risque, NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.PCL AmazonServiceClient.cs 332
  • V3083 [CWE-367] L'appel d'événement à risque, NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.PCL AmazonServiceClient.cs 344
  • V3083 [CWE-367] L'appel d'événement à risque, NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.PCL AmazonServiceClient.cs 357
  • V3083 [CWE-367] L'appel non sécurisé de l'événement 'mExceptionEvent', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.PCL AmazonServiceClient.cs 366
  • V3083 [CWE-367] L'appel d'événement à risque, NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.PCL AmazonWebServiceRequest.cs 78
  • V3083 [CWE-367] L'appel non sûr de l'événement 'OnRead', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.PCL EventStream.cs 97
  • V3083 [CWE-367] L'appel d'événement à risque, NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.Android NetworkReachability.cs 57
  • V3083 [CWE-367] L'appel d'événement à risque, NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.Android NetworkReachability.cs 94
  • V3083 [CWE-367] L'appel d'événement à risque, NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. AWSSDK.Core.iOS NetworkReachability.cs 54

Classe brute

Avertissement PVS-Studio: le type V3126 implémentant l'interface IEquatable <T> de type 'JsonData' ne remplace pas la méthode 'GetHashCode'. AWSSDK.Core.Net45 JsonData.cs 26

 public class JsonData : IJsonWrapper, IEquatable<JsonData> { .... }

La classe JsonData contient pas mal de code, donc je ne l'ai pas donné en entier, en ne citant que sa déclaration. Cette classe ne contient vraiment pas la méthode substituée GetHashCode, qui n'est pas sûre, car elle peut conduire à un comportement erroné lors de l'utilisation du type JsonData pour travailler, par exemple, avec des collections. Il n'y a probablement pas de problème pour le moment, mais à l'avenir ce type de stratégie pourrait changer. Cette erreur est décrite plus en détail dans la documentation .

Conclusion

Ce sont tous des bogues intéressants que j'ai pu détecter dans le code du kit AWS SDK pour .NET en utilisant l'analyseur statique PVS-Studio. Je voudrais souligner encore une fois la qualité du projet. J'ai trouvé un très petit nombre d'erreurs pour 5 millions de lignes de code. Bien qu'une analyse probablement plus approfondie des avertissements émis me permette d'ajouter quelques erreurs supplémentaires à cette liste. Néanmoins, il est également très probable que j'ai ajouté certains des avertissements aux erreurs pour rien. Dans ce cas, les conclusions non ambiguës sont toujours faites uniquement par un développeur qui se trouve dans le contexte du code vérifié.

Source: https://habr.com/ru/post/fr437514/

More articles:


All Articles