Cet article passe en revue les résultats d'une deuxième vérification du projet Orchard avec l'analyseur statique PVS-Studio. Orchard est un système de gestion de contenu open source fourni dans le cadre de la galerie Open Source ASP.NET sous la fondation à but non lucratif Outercurve Foundation. La vérification d'aujourd'hui est particulièrement intéressante car le projet et l'analyseur ont parcouru un long chemin depuis la première vérification, et cette fois, nous allons examiner de nouveaux messages de diagnostic et de beaux bugs.
À propos d'Orchard CMSNous avons
vérifié Orchard il y a trois ans. L'analyseur C # de PVS-Studio a considérablement évolué depuis: nous avons amélioré l'analyse du flux de données, ajouté une analyse interprocédurale et de nouveaux diagnostics, et corrigé un certain nombre de faux positifs. Plus que cela, la deuxième vérification a révélé que les développeurs d'Orchard avaient corrigé tous les bogues signalés dans le premier article, ce qui signifie que nous avions atteint notre objectif, c'est-à-dire leur avait fait améliorer leur code.
J'espère qu'ils prêteront également attention à cet article et apporteront les correctifs nécessaires ou, mieux encore, adopteront PVS-Studio pour une utilisation régulière. Pour rappel, nous proposons aux développeurs
open source une licence gratuite. Soit dit en passant, il existe d'autres
options que les projets propriétaires peuvent également apprécier.
Le code source d'Orchard est disponible en téléchargement
ici . La description complète du projet se trouve
ici . Si vous n'avez pas encore de copie PVS-Studio, vous pouvez
télécharger la version d'essai. J'ai utilisé PVS-Studio 7.05 Beta et inclurai certains de ses avertissements dans cet article. J'espère que cette revue vous convaincra que PVS-Studio est un outil utile. N'oubliez pas qu'il est destiné à être utilisé
régulièrement .
Résultats d'analyseVoici quelques chiffres de la première vérification d'Orchard afin que vous n'ayez pas à basculer entre les deux articles pour comparaison.
Lors de la vérification précédente, "nous avons effectué l'analyse de tous les fichiers de code source (3739 éléments) avec l'extension .cs. Au total, il y avait 214 564 lignes de code. Le résultat de la vérification a été de 137 avertissements. Pour être plus précis, il y a eu 39 avertissements de premier niveau (élevé). Il y avait également 60 avertissements de niveau (moyen). »
La version actuelle d'Orchard est composée de 2767 fichiers .cs, c'est-à-dire qu'elle est environ mille fichiers plus petite. La réduction et le changement de nom du référentiel suggèrent que les développeurs ont isolé le cœur du projet (
commit 966 ), qui fait 108 287 LOC. L'analyseur a émis 153 avertissements: 33 de premier niveau et 70 de deuxième niveau. Nous n'incluons généralement pas les avertissements de troisième niveau, et je vais m'en tenir à la tradition.
Message de diagnostic PVS-Studio: V3110 Récursion infinie possible dans la méthode 'TryValidateModel'. PrefixedModuleUpdater.cs 48
public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); }
Commençons par un bogue de récursion infini, comme nous l'avons fait dans le premier article. Cette fois, les intentions exactes du développeur ne sont pas claires, mais j'ai remarqué que la méthode
TryValidateModel avait une version surchargée avec un paramètre:
public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
Je pense que, tout comme dans le cas de la version surchargée, le développeur avait l'intention d'appeler la méthode via
_updateModel. Le compilateur n'a pas remarqué l'erreur;
_updateModel est de type
IUpdateModel , et la classe actuelle implémente également cette interface. Étant donné que la méthode n'inclut aucune vérification contre
StackOverflowException , elle n'a probablement jamais été appelée, bien que je ne compte pas sur cela. Si mon hypothèse est correcte, la version fixe devrait ressembler à ceci:
public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
Message de diagnostic PVS-Studio: V3008 La variable «contenu» se voit attribuer deux fois de suite des valeurs. C'est peut-être une erreur. Vérifiez les lignes: 197, 190. DynamicCacheTagHelper.cs 197
public override async Task ProcessAsync(....) { .... IHtmlContent content; .... try { content = await output.GetChildContentAsync(); } finally { _cacheScopeManager.ExitScope(); } content = await ProcessContentAsync(output, cacheContext); .... }
L'analyseur a détecté deux affectations au
contenu de variable locale
. GetChildContentAsync est une méthode de bibliothèque qui est utilisée trop rarement pour que nous prenions la peine de l'examiner et de l'annoter. Donc, je le crains, ni nous, ni l'analyseur ne savons quoi que ce soit sur l'objet de retour de la méthode et ses effets secondaires. Mais nous savons avec certitude que l'attribution de la valeur de retour au
contenu n'a aucun sens si elle n'est pas utilisée plus loin dans le code. C'est peut-être juste une opération redondante plutôt qu'une erreur. Je ne peux pas dire exactement comment cela devrait être corrigé, je laisse donc le soin aux développeurs.
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible. Pensez à inspecter «itemTag». CoreShapes.cs 92
public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = String.IsNullOrEmpty(itemTagName) ? null : ....; .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }
L'analyseur a détecté une déréférence dangereuse de
itemTag . Cet extrait est un bon exemple de la façon dont un outil d'analyse statique est différent d'un développeur humain effectuant une révision de code. La méthode a un paramètre nommé
ItemTag et une variable locale nommée
itemTag . Inutile de vous dire que cela fait une énorme différence pour le compilateur! Ce sont deux variables différentes, bien que liées. La façon dont ils sont liés se fait via une troisième variable,
itemTagName. Voici la séquence d'étapes menant à l'exception possible: si l'argument
ItemTag est égal à "-", aucune valeur ne sera affectée à
itemTagName , il restera donc une référence null, et s'il s'agit d'une référence null, alors la variable locale
itemTag deviendra également une référence nulle. À mon avis, il est préférable d'avoir une exception levée après la vérification de la chaîne.
public async Task<IHtmlContent> List(....string ItemTag....) { .... string itemTagName = null; if (ItemTag != "-") { itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag; } var index = 0; foreach (var item in items) { var itemTag = ....; if(String.IsNullOrEmpty(itemTag)) throw .... .... itemTag.InnerHtml.AppendHtml(itemContent); listTag.InnerHtml.AppendHtml(itemTag); ++index; } return listTag; }
Message de diagnostic PVS-Studio: V3095 L'objet 'remoteClient' a été utilisé avant d'être vérifié par rapport à null. Vérifiez les lignes: 49, 51. ImportRemoteInstanceController.cs 49
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); var apiKey = Encoding.UTF8.GetString(....(remoteClient.ProtectedApiKey)); if (remoteClient == null || ....) { .... } .... }
L'analyseur a détecté une déréférence de
remoteClient suivie d'une vérification nulle quelques lignes plus tard. Il s'agit en effet d'une
NullReferenceException potentielle car la méthode
FirstOrDefault peut renvoyer une valeur par défaut (qui est
nulle pour les types de référence). Je suppose que cet extrait peut être corrigé en déplaçant simplement le chèque vers le haut afin qu'il précède l'opération de déréférencement:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
Ou peut-être que cela devrait être corrigé en remplaçant
FirstOrDefault par
First et en supprimant complètement le chèque.
Avertissements de PVS-Studio 7.05 Beta:À ce jour, nous avons annoté toutes les méthodes
orDefault de
LINQ . Ces informations seront utilisées par le nouveau diagnostic sur lequel nous travaillons: il détecte les cas où les valeurs renvoyées par ces méthodes sont déréférencées sans contrôle préalable. Chaque méthode
orDefault a un équivalent qui lève une exception si aucun élément correspondant n'a été trouvé. Cette exception sera plus utile pour rechercher le problème que la
NullReferenceException abstraite.
Je ne peux que partager les résultats que j'ai obtenus de ce diagnostic sur le projet Orchard. Il y a 27 endroits potentiellement dangereux. En voici quelques uns:
ContentTypesAdminNodeNavigationBuilder.cs 71:
var treeBuilder = treeNodeBuilders.Where(....).FirstOrDefault(); await treeBuilder.BuildNavigationAsync(childNode, builder, treeNodeBuilders);
ListPartDisplayDriver.cs 217:
var contentTypePartDefinition = ....Parts.FirstOrDefault(....); return contentTypePartDefinition.Settings....;
ContentTypesAdminNodeNavigationBuilder.cs 113:
var typeEntry = node.ContentTypes.Where(....).FirstOrDefault(); return AddPrefixToClasses(typeEntry.IconClass);
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible de la valeur de retour de la méthode. Pensez à inspecter: CreateScope (). SetupService.cs 136
public async Task<string> SetupInternalAsync(SetupContext context) { .... using (var shellContext = await ....) { await shellContext.CreateScope().UsingAsync(....); } .... }
L'analyseur a mentionné une déréférence de la valeur renvoyée par la méthode
CreateScope .
CreateScope est une toute petite méthode, voici donc son implémentation complète:
public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this);
Comme vous pouvez le voir, il existe deux cas où il peut retourner
null . L'analyseur ne sait pas quelle branche le flux d'exécution suivra, il joue donc en toute sécurité et signale le code comme suspect. Si je devais écrire du code comme ça, j'écrirais tout de suite un chèque nul.
Peut-être que mon opinion est biaisée, mais je pense que chaque méthode asynchrone devrait être protégée contre
NullReferenceException autant que possible parce que le débogage de trucs comme ça est loin d'être agréable.
Dans ce cas particulier, la méthode
CreateScope est appelée quatre fois: deux de ces appels sont accompagnés de vérifications et les deux autres ne le sont pas. Les deux derniers appels (sans vérifications) semblent être des clones copier-coller (même classe, même méthode, même manière de déréférencer le résultat pour appeler UsingAsync). Le premier de ces deux appels a été montré ci-dessus, et vous pouvez être sûr que le second a déclenché le même avertissement:
V3080 Déréférence nulle possible de la valeur de retour de la méthode. Pensez à inspecter: CreateScope (). SetupService.cs 192
Message de diagnostic PVS-Studio: V3127 Deux fragments de code similaires ont été trouvés. Il s'agit peut-être d'une faute de frappe et la variable «AccessTokenSecret» devrait être utilisée à la place de «ConsumerSecret» TwitterClientMessageHandler.cs 52
public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }
C'est une erreur de copier-coller classique.
ConsumerSecret a été vérifié deux fois, tandis que
AccessTokenSecret n'a pas été vérifié du tout. De toute évidence, cela est résolu comme suit:
public async Task ConfigureOAuthAsync(HttpRequestMessage request) { .... if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret)) settings.ConsumerSecret = protrector.Unprotect(settings.ConsumerSecret); if (!string.IsNullOrWhiteSpace(settings.AccessTokenSecret)) settings.AccessTokenSecret = protrector.Unprotect(settings.AccessTokenSecret); .... }
Message de diagnostic PVS-Studio: V3139 Deux ou plusieurs branches de cas effectuent les mêmes actions. SerialDocumentExecuter.cs 23
Un autre bug de copier-coller. Pour plus de clarté, voici l'implémentation complète de la classe (elle est petite).
public class SerialDocumentExecuter : DocumentExecuter { private static IExecutionStrategy ParallelExecutionStrategy = new ParallelExecutionStrategy(); private static IExecutionStrategy SerialExecutionStrategy = new SerialExecutionStrategy(); private static IExecutionStrategy SubscriptionExecutionStrategy = new SubscriptionExecutionStrategy(); protected override IExecutionStrategy SelectExecutionStrategy(....) { switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; } } }
L'analyseur n'aimait pas les deux branches de
cas identiques. En effet, la classe a trois entités, tandis que l'instruction switch n'en renvoie que deux. Si ce comportement est voulu et que la troisième entité n'est pas réellement destinée à être utilisée, le code peut être amélioré en supprimant la troisième branche après avoir fusionné les deux comme suit:
switch (context.Operation.OperationType) { case OperationType.Query: case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
S'il s'agit d'un bogue copier-coller, le premier des champs de retour en double doit être corrigé comme suit:
switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Ou ce devrait être la deuxième branche du cas. Je ne connais pas les détails du projet et ne peux donc pas déterminer la corrélation entre les noms des types d'opérations et des stratégies.
switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible. Envisagez d'inspecter la «demande». GraphQLMiddleware.cs 148
private async Task ExecuteAsync(HttpContext context....) { .... GraphQLRequest request = null; .... if (HttpMethods.IsPost(context.Request.Method)) { .... } else if (HttpMethods.IsGet(context.Request.Method)) { .... request = new GraphQLRequest(); .... } var queryToExecute = request.Query; .... }
La variable de
requête se voit attribuer une valeur différente de
null plusieurs fois dans le premier bloc
if , mais à chaque fois avec des conditions imbriquées. Inclure toutes ces conditions rendrait l'exemple trop long, nous allons donc simplement passer aux premières, qui vérifient le type de la méthode http
IsGet ou
IsPost . La classe
Microsoft.AspNetCore.Http.HttpMethods a neuf méthodes statiques pour vérifier le type de requête. Par conséquent, la transmission, par exemple, d'une requête
Delete ou
Set à la méthode
ExecuteAsync conduirait à
déclencher une
exception NullReferenceException . Même si de telles méthodes ne sont actuellement pas du tout prises en charge, il serait toujours judicieux d'ajouter une vérification de levée d'exceptions. Après tout, la configuration système requise peut changer. Voici un exemple d'une telle vérification:
private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible de la valeur de retour de la méthode. Pensez à inspecter: Get <ContentPart> (...). ContentPartHandlerCoordinator.cs 190
La plupart des avertissements du
V3080 sont plus pratiques à afficher dans l'environnement de développement, car vous avez besoin de la navigation par méthode, de la mise en surbrillance des types et de l'atmosphère conviviale de l'EDI. J'essaie de réduire le plus possible le texte des exemples pour les garder lisibles. Mais si je ne le fais pas correctement ou si vous voulez tester vos compétences en programmation et tout comprendre par vous-même, je vous recommande de vérifier le résultat de ce diagnostic sur tout projet open-source ou tout simplement votre propre code.
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
L'analyseur signale cette ligne. Jetons un coup d'œil à la méthode
Get :
public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }
Il appelle sa version surchargée. Vérifions-le aussi:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... }
Il s'avère que si nous obtenons une entité d'un type incompatible avec
JObject à partir de
Data en utilisant l'indexeur de
noms , la méthode
Get retournera
null . Je ne sais pas avec certitude quelle est la probabilité car ces types proviennent de la bibliothèque
Newtonsoft.Json , avec laquelle je n'ai pas beaucoup travaillé. Mais l'auteur du code soupçonnait que l'élément recherché pourrait ne pas exister, nous devons donc garder cela à l'esprit lors de l'accès au résultat de l'opération de lecture également. Personnellement, j'aurais une exception levée dans le tout premier
Get si nous pensons que le nœud doit être présent, ou ajouter une vérification avant la déréférence si la non-existence du nœud ne change pas la logique globale (par exemple, nous obtenons un valeur par défaut).
Solution 1:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
Solution 2:
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
Message de diagnostic PVS-Studio: V3080 Déréférence nulle possible. Pensez à inspecter les «résultats». ContentQueryOrchardRazorHelperExtensions.cs 19
public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); .... foreach (var result in results) { .... } .... }
Ceci est un exemple assez simple par rapport au précédent. L'analyseur soupçonne que la méthode
QueryAsync peut renvoyer une référence nulle. Voici l'implémentation de la méthode:
public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
Étant donné que
GetQueryAsync est une méthode d'interface, vous ne pouvez pas être sûr de chaque implémentation, surtout si nous considérons que le projet comprend également la version suivante:
public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
Les multiples appels aux fonctions externes et aux accès au cache rendent l'analyse de
GetDocumentAsync difficile, alors disons simplement que la vérification est nécessaire - d'autant plus que la méthode est asynchrone.
public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); if(results == null) throw ....; .... foreach (var result in results) { .... } .... }
ConclusionJe ne peux que mentionner la haute qualité du code d'Orchard! Certes, il y avait d'autres défauts, dont je n'ai pas parlé ici, mais je vous ai montré tous les bugs les plus graves. Bien sûr, cela ne veut pas dire que vérifier votre code source une fois tous les trois ans est suffisant. Vous tirerez le meilleur parti de l'analyse statique si vous l'utilisez
régulièrement, car c'est ainsi que vous êtes assuré de détecter et de corriger les bogues dès les premiers stades de développement, où la correction des bogues est la moins chère et la plus simple.
Même si les vérifications ponctuelles n'aident pas beaucoup, je vous encourage toujours à télécharger
PVS-Studio et à l'essayer sur votre projet: qui sait, vous trouverez peut-être aussi des bogues intéressants.