Lorsque nous avons décidé de rechercher des erreurs dans le projet Azure SDK pour .NET, nous avons été agréablement surpris par sa taille. «Trois millions et demi de lignes de code», avons-nous déclaré en étudiant les statistiques du projet. Voilà combien vous pouvez y trouver. Mais hélas, ah. Le projet s'est avéré être un secret. Quelle est la particularité du projet et comment il a été testé - lire dans cet article.
À propos du projet
J'écris cet article dans le prolongement de mon article précédent, qui portait également sur un projet lié à Microsoft Azure:
Azure PowerShell: "fondamentalement inoffensif" . Donc, cette fois, je comptais sur un nombre solide d'erreurs variées et intéressantes. En effet, pour une analyse statique, la taille du projet est généralement importante, notamment avec les contrôles ponctuels de l'ensemble du projet. Oui, en pratique, ils ne le font généralement pas. Et s'ils le font, alors seulement au stade de la mise en œuvre de l'analyseur. Dans le même temps, personne ne comprend immédiatement le grand nombre d'opérations (un nombre important d'avertissements est la norme lorsque l'analyseur démarre dans ce mode), mais les met simplement dans une dette technique en utilisant des mécanismes de suppression et de stockage des messages dans des bases de données spéciales (suppression de masse). Nous effectuons des inspections ponctuelles à des fins de recherche. Par conséquent, les grands projets d'étude sont toujours préférables aux petits.
Cependant, le projet Azure SDK pour .NET a immédiatement montré son échec en tant que banc de test. Même sa taille impressionnante n'a pas aidé, mais a même compliqué le travail. La raison est indiquée dans les statistiques de projet suivantes:
- Fichiers source .cs (non compris les tests): 16 500
- Solutions Visual Studio (.sln): 163
- Lignes de code non vides: 3 462 000
- Dont auto-générées: environ 3,3 millions
- Le référentiel de projet est disponible sur GitHub .
Environ 95% du code est généré automatiquement, une partie importante de ce code est répétée plusieurs fois. La vérification de tels projets avec un analyseur statique est généralement longue et inutile, car il y a beaucoup de code de travail, mais illogique (à première vue) et redondant. Cela conduit à un grand nombre de faux positifs.
Un grand nombre de solutions Visual Studio (163) ont été utilisées comme cerises sur le gâteau, selon lesquelles cette masse de code était «tachée». Donc, pour vérifier le code restant (non généré automatiquement), j'ai dû faire quelques efforts. Cela a aidé à ce que tout le code généré automatiquement se trouve dans les sous-dossiers de la solution le long du chemin relatif "<Dossier de la solution> \ src \ Generated". De plus, chaque fichier .cs de ce code contient un commentaire spécial dans la
balise <auto-generated> :
Pour la pureté de l'expérience, j'ai vérifié au hasard une dizaine de solutions sélectionnées au hasard avec du code généré automatiquement. Les résultats seront inférieurs.
Donc, malgré la petite quantité de code «honnête» restant, j'ai quand même réussi à y trouver un certain nombre d'erreurs. Cette fois, je ne donnerai pas de trajets dans l'ordre des numéros des diagnostics PVS-Studio. Au lieu de cela, je regrouperai les réponses selon les solutions dans lesquelles elles ont été détectées.
Voyons ce que j'ai trouvé dans le SDK Azure pour le code .NET.
Microsoft.Azure.Management.Advisor
Ceci n'est qu'une des nombreuses solutions qui contiennent du code généré automatiquement. Comme je l'ai dit plus haut, une douzaine de ces solutions ont été testées de manière sélective. Et partout, les messages étaient les mêmes et, comme prévu, inutiles. Permettez-moi de vous donner quelques exemples de telles réponses.
V3022 L' expression 'Credentials! = Null' est toujours vraie. AdvisorManagementClient.cs 204
De toute évidence, le code est redondant, et vérifier les
informations d'identification! = Null est inutile. Mais le code fonctionne. Et autogénéré. Par conséquent, rien à redire.
V3022 L' expression '_queryParameters.Count> 0' est toujours fausse. ConfigurationsOperations.cs 871
Et encore une fois, apparemment dépourvu de conception logique. Pour une raison quelconque, ils vérifient la taille de la liste
vide qui vient d'être créée. En fait - tout est en ordre. Cette vérification est inutile maintenant, mais si le générateur configure la création d'une liste, par exemple, sur la base d'une autre collection, alors la vérification aura déjà un sens. Donc - encore une fois, il n'y a aucune plainte contre le code, étant donné son origine, bien sûr.
Pour chaque solution avec du code généré automatiquement, des centaines d'avertissements similaires ont été reçus. Compte tenu de leur futilité, je pense qu'il est inutile de poursuivre la discussion sur de tels points positifs. Seules les vraies erreurs dans le code «normal» seront considérées ci-dessous.
Azure.Core
V3001 Il existe des sous-expressions identiques 'buffer.Length' à gauche et à droite de l'opérateur '<'. AzureBaseBuffersExtensions.cs 30
public static async Task WriteAsync(...., ReadOnlyMemory<byte> buffer, ....) { byte[]? array = null; .... if (array == null || buffer.Length < buffer.Length)
Une erreur dans la condition a été commise, probablement à la suite de l'utilisation du copier-coller. A en juger par le fait que plus loin dans le
tampon de code
est copié dans le
tableau , la vérification devrait ressembler à:
if (array == null || array.Length < buffer.Length)
Mais, comme je le dis toujours, l'auteur du code devrait corriger ces erreurs.
V3083 Appel non sécurisé de l'événement '_onChange', NullReferenceException est possible. Pensez à affecter un événement à une variable locale avant de l'invoquer. ClientOptionsMonitor.cs 44
private event Action<TOptions, string> _onChange; .... private void InvokeChanged(....) { .... if (_onChange != null) { _onChange.Invoke(options, name); } }
Non critique, mais une erreur. Entre la vérification de l'égalité
nulle de l'événement et son invocation, l'événement peut être désabonné.
La variable
_onChange obtiendra alors
null et une exception sera levée. Le code doit être réécrit de manière plus sûre. Par exemple, comme ceci:
private void InvokeChanged(....) { .... _onChange?.Invoke(options, name); }
Azure.Messaging.EventHubs
V3080 Déréférence nulle possible. Pensez à inspecter 'eventPropertyValue'. AmqpMessageConverter.cs 650
private static bool TryCreateEventPropertyForAmqpProperty( object amqpPropertyValue, out object eventPropertyValue) { eventPropertyValue = null; .... switch (GetTypeIdentifier(amqpPropertyValue)) { case AmqpProperty.Type.Byte: .... case AmqpProperty.Type.String: eventPropertyValue = amqpPropertyValue; return true; .... } .... switch (amqpPropertyValue) { case AmqpSymbol symbol: eventPropertyValue = ....; break; case byte[] array: eventPropertyValue = ....; break; case ArraySegment<byte> segment when segment.Count == segment.Array.Length: eventPropertyValue = ....; break; case ArraySegment<byte> segment: .... eventPropertyValue = ....; break; case DescribedType described when (described.Descriptor is AmqpSymbol): eventPropertyValue = ....; break; default: var exception = new SerializationException( string.Format(...., eventPropertyValue.GetType().FullName));
Voyons ce qui arrive à la valeur de la variable
eventPropertyValue dans l'extrait de code ci-dessus. Au début de la méthode, la variable est
nulle . En outre, dans l'une des conditions du premier
commutateur , la variable est initialisée, après quoi le procédé se termine. Le deuxième bloc de
commutateur contient beaucoup de conditions, dans chacune desquelles la variable reçoit également une nouvelle valeur. Mais dans le bloc
par défaut , la variable
eventPropertyValue est simplement utilisée sans aucune vérification, ce qui est une erreur, car pour le moment la variable est
nulle .
V3066 Ordre incorrect possible des arguments passés au constructeur 'EventHubConsumer': 'partitionId' et 'consumerGroup'. TrackOneEventHubClient.cs 394
public override EventHubConsumer CreateConsumer(....) { return new EventHubConsumer ( new TrackOneEventHubConsumer(....), TrackOneClient.EventHubName, partitionId,
L'analyseur soupçonnait que lors de l'appel du constructeur de la classe
EventHubConsumer , l'ordre des troisième et quatrième arguments était mélangé. Regardons la déclaration du constructeur:
internal EventHubConsumer(TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, // <= 3 string partitionId, // <= 4 EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... }
Les arguments sont vraiment mélangés. Je suppose que cette erreur a été commise. La faute est probablement le mauvais formatage du code. Jetez un autre coup d'œil à la
déclaration du constructeur
EventHubConsumer . En raison du fait que le premier paramètre
transportConsumer se trouve sur la même ligne que le nom de la classe, tout en regardant brièvement le code, il peut sembler que le paramètre
partitionId est à la troisième place et non à la quatrième (il n'y a pas mes commentaires avec des numéros de série de paramètres dans le code d'origine )
C'est juste une supposition, mais je changerais la mise en forme du code de déclaration du constructeur en ceci:
internal EventHubConsumer ( TransportEventHubConsumer transportConsumer, string eventHubName, string consumerGroup, string partitionId, EventPosition eventPosition, EventHubConsumerOptions consumerOptions, EventHubRetryPolicy retryPolicy) { .... }
Azure.Storage
V3112 Une anomalie dans des comparaisons similaires. Il est possible qu'une faute de frappe soit présente à l'intérieur de l'expression 'ContentLanguage == other.ContentEncoding'. BlobSasBuilder.cs 410
public struct BlobSasBuilder : IEquatable<BlobSasBuilder> { .... public bool Equals(BlobSasBuilder other) => BlobName == other.BlobName && CacheControl == other.CacheControl && BlobContainerName == other.BlobContainerName && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding &&
Une erreur commise par inattention. Trouver une erreur similaire avec la révision de code est assez difficile. L'option de vérification correcte:
.... ContentEncoding == other.ContentEncoding && ContentLanguage == other.ContentLanguage && ....
V3112 Une anomalie dans des comparaisons similaires. Il est possible qu'une faute de frappe soit présente à l'intérieur de l'expression 'ContentLanguage == other.ContentEncoding'. FileSasBuilder.cs 265
public struct FileSasBuilder : IEquatable<FileSasBuilder> { .... public bool Equals(FileSasBuilder other) => CacheControl == other.CacheControl && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding
Exactement la même erreur dans un morceau de code très similaire. Le code a probablement été copié et partiellement modifié. Mais l'erreur est restée.
Microsoft.Azure.Batch
V3053 Une expression excessive. Examinez les sous-chaînes «IList» et «List». PropertyData.cs 157
V3053 Une expression excessive. Examinez les sous-chaînes 'List' et 'IReadOnlyList'. PropertyData.cs 158
public class PropertyData { .... public bool IsTypeCollection => this.Type.Contains("IList") || this.Type.Contains("IEnumerable") || this.Type.Contains("List") ||
L'analyseur a émis deux avertissements concernant les vérifications insensées ou erronées. Dans le premier cas, la recherche de la sous-chaîne «List» après avoir recherché «IList» semble redondante. En effet, la condition:
this.Type.Contains("IList") || this.Type.Contains("List")
peut être remplacé par ceci:
this.Type.Contains("List")
Dans le second cas, la recherche de la sous-chaîne "IReadOnlyList" n'a pas de sens, car auparavant la recherche de la sous-chaîne plus courte "List" est effectuée.
Il est également possible que les sous-chaînes elles-mêmes pour la recherche aient fait des erreurs et il devrait y avoir autre chose. Dans tous les cas, la version correcte de la correction d'état, tenant compte des deux remarques, ne peut être proposée que par l'auteur du code.
V3095 L'objet 'httpRequest.Content.Headers' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 76, 79. BatchSharedKeyCredential.cs 76
public override Task ProcessHttpRequestAsync( HttpRequestMessage httpRequest, ....) { .... signature.Append(httpRequest.Content != null && httpRequest.Content.Headers.Contains("Content-Language") ? .... : ....; long? contentLength = httpRequest.Content?.Headers?.ContentLength; .... }
Au début, la variable
httpRequest.Content.Headers est utilisée sans aucun contrôle, mais plus tard dans le code, cette variable est accessible à l'aide de l'opérateur d'accès conditionnel.
V3125 L'objet 'omPropertyData' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 156, 148. CodeGenerationUtilities.cs 156
private static string GetProtocolCollectionToObjectModelCollectionString( ...., PropertyData omPropertyData, ....) { if (IsMappedEnumPair(omPropertyData?.GenericTypeParameter, ....)) { .... } if (IsTypeComplex(omPropertyData.GenericTypeParameter)) .... }
La situation inverse. Un bloc de code contient une option d'accès sécurisé au lien
omPropertyData potentiellement nul. Plus loin dans le code avec le même lien, ils fonctionnent sans aucun contrôle.
V3146 Déréférence nulle possible de 'valeur'. Le 'FirstOrDefault' peut renvoyer la valeur nulle par défaut. BatchSharedKeyCredential.cs 127
public override Task ProcessHttpRequestAsync(HttpRequestMessage httpRequest, ....) { .... foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... } .... }
En raison de la méthode
FirstOrDefault , si la recherche échoue, la valeur par défaut pour le type de
chaîne sera renvoyée, c'est-à-dire
null . La valeur sera affectée à la variable de
valeur , qui est utilisée plus tard dans le code avec la méthode
Replace sans aucune vérification. Le code devrait être rendu plus sûr. Par exemple, comme ceci:
foreach (string canonicalHeader in customHeaders) { string value = httpRequest.Headers. GetValues(canonicalHeader).FirstOrDefault(); value = value?.Replace('\n', ' ').Replace('\r', ' ').TrimStart(); .... }
Microsoft.Azure.ServiceBus
V3121 Une énumération 'BlocksUsing' a été déclarée avec l'attribut 'Flags', mais ne définit aucun initialiseur pour remplacer les valeurs par défaut. Fx.cs 69
static class Fx { .... public static class Tag { .... [Flags] public enum BlocksUsing { MonitorEnter, MonitorWait, ManualResetEvent, AutoResetEvent, AsyncResult, IAsyncResult, PInvoke, InputQueue, ThreadNeutralSemaphore, PrivatePrimitive, OtherInternalPrimitive, OtherFrameworkPrimitive, OtherInterop, Other, NonBlocking, } .... } .... }
Une énumération est déclarée avec l'attribut
Flags . Dans ce cas, les valeurs des constantes sont laissées par défaut (
MonitorEnter = 0 ,
MonitorWait = 1 ,
ManualResetEvent = 2, etc.). Cela peut conduire au fait que lorsque vous essayez d'utiliser une
combinaison d' indicateurs, par exemple, les deuxième et troisième constantes
MonitorWait (= 1) |
ManualResetEvent (= 2) , pas une valeur unique ne sera reçue, mais une constante avec une valeur de 3 par défaut (
AutoResetEvent ). Cela peut surprendre le code appelant. Si l'énumération
BlocksUsing est vraiment prévue pour être utilisée pour spécifier des combinaisons d'indicateurs (champ de bits), vous devez donner des constantes égales aux puissances de deux:
[Flags] public enum BlocksUsing { MonitorEnter = 1, MonitorWait = 2, ManualResetEvent = 4, AutoResetEvent = 8, AsyncResult = 16, IAsyncResult = 32, PInvoke = 64, InputQueue = 128, ThreadNeutralSemaphore = 256, PrivatePrimitive = 512, OtherInternalPrimitive = 1024, OtherFrameworkPrimitive = 2048, OtherInterop = 4096, Other = 8192, NonBlocking = 16384, }
V3125 L'objet 'session' a été utilisé après avoir été vérifié par rapport à null. Vérifiez les lignes: 69, 68. AmqpLinkCreator.cs 69
public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try {
Faites attention au travail avec la variable de
session dans le
bloc catch . La méthode
Abort est
appelée en toute sécurité via l'instruction d'accès conditionnel. Mais ils effectuent ensuite un appel non sécurisé à la méthode
GetInnerException . Dans ce cas, au lieu de lever une exception du type attendu, une
NullReferenceException peut être levée. Le code doit être corrigé. La méthode
AmqpExceptionHelper.GetClientException prend en charge la transmission d'une valeur
nulle pour le paramètre
innerException :
public static Exception GetClientException( Exception exception, string referenceId = null, Exception innerException = null, bool connectionError = false) { .... }
Par conséquent, il suffit d'utiliser l'opérateur d'accès conditionnel lors de l'appel de
session.GetInnerException () :
public async Task<Tuple<AmqpObject, DateTime>> CreateAndOpenAmqpLinkAsync() { .... AmqpSession session = null; try {
Conclusion
Comme vous pouvez le voir, une grande taille de projet ne garantit pas toujours un grand nombre d'erreurs. Mais pas besoin de vous détendre - vous pouvez toujours trouver quelque chose. Même dans un projet aussi complexe comme Azure SDK pour .NET. Oui, cela nécessite des efforts supplémentaires, mais plus le résultat sera agréable. Et pour que vous n'ayez pas à faire d'efforts excessifs, nous vous recommandons d'utiliser l'analyse statique et sur le lieu de travail des développeurs lors de l'écriture de nouveau code. C'est l'approche la plus efficace.
Téléchargez et essayez PVS-Studio en action. Bonne chance dans la lutte contre les bugs!
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien vers la traduction: Sergey Khrenov.
SDK Azure pour .NET: histoire d'une recherche d'erreur difficile .