Cet article est le résultat d'une nouvelle vérification du projet Orchard à l'aide de l'analyseur statique PVS-Studio. Orchard est un système de gestion de contenu open source qui fait partie de la Fondation Outercurve, une galerie de projets ASP.NET à but non lucratif. Le test est intéressant dans la mesure où le projet et l'analyseur ont considérablement augmenté depuis la première analyse. De nouveaux bugs positifs et intéressants vous attendent.
À propos d'Orchard CMSNous avons
testé Orchard avec PVS-Studio il y a trois ans. L'analyseur C # s'est considérablement développé depuis: nous avons amélioré l'analyse des flux de données, développé l'analyse interprocédurale, ajouté de nouveaux diagnostics et corrigé un certain nombre de faux positifs. De plus, l'analyse a montré que tous les commentaires de l'article précédent ont été corrigés. Cela signifie que l'objectif est atteint - le code est devenu meilleur.
Je veux croire que les développeurs seront attentifs à cet article et apporteront les modifications nécessaires. Encore mieux, si vous introduisez la pratique de l'utilisation régulière de PVS-Studio. Permettez-moi de vous rappeler que pour
les projets
open-source , nous fournissons une version gratuite de la licence. Soit dit en passant, il existe d'autres
options qui conviennent aux projets fermés.
Le code du projet Orchard peut être téléchargé
ici , comme je l'ai fait. Une description complète du projet peut être trouvée
ici . Si vous ne disposez pas encore de notre analyseur PVS-Studio, vous pouvez télécharger une version d'essai à
partir d'ici . J'ai utilisé la version de PVS-Studio version 7.05 Beta. Je partagerai les résultats de son travail. J'espère que vous êtes d'accord avec l'utilité d'utiliser PVS-Studio. L'essentiel est d'utiliser
régulièrement l'analyseur.
Résultats de validationJe vais vous donner quelques chiffres du dernier article afin que vous n'ayez pas à changer pour comparer.
«Tous les fichiers (3739 pièces) portant l'extension .cs ont participé au dernier contrôle. Au total, ils contenaient 214 564 lignes de code. Sur la base des résultats de l'audit, 137 avertissements ont été reçus. Au premier niveau de confiance (élevé), 39 avertissements ont été reçus. Au deuxième niveau (moyenne), 60 avertissements ont été reçus. »
Pour le moment, le projet contient 2767 fichiers .cs, c'est-à-dire qu'il est devenu moins d'un millier de fichiers. A en juger par cette diminution du nombre de fichiers et le changement de nom de référentiel, un noyau a été alloué depuis le projet (
commit 966 ). Il y a 108 287 lignes de code dans le noyau et l'analyseur a émis 153 avertissements, 33 à un niveau élevé, 70 en moyenne. Nous ne considérons généralement pas les avertissements du troisième niveau, je n'ai pas rompu la tradition.
Avertissement 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)); }
Comme dans l'article précédent, nous ouvrons la liste des erreurs à récursion infinie. Il est difficile de dire exactement ce que le développeur voulait faire dans ce cas. Mais j'ai remarqué que la méthode
TryValidateModel a une surcharge d'argument unique qui ressemble à ceci:
public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
Je suppose que, comme avec la méthode surchargée, le développeur voulait appeler des méthodes via
_updateModel. Le compilateur n'a pas vu d'erreur,
_updateModel est de type
IUpdateModel et la classe actuelle implémente également cette interface. Puisqu'il n'y a pas une seule condition dans la méthode pour se protéger contre une
StackOverflowException , la méthode n'a peut-être pas été appelée une seule fois, mais je ne compterais pas dessus. Si l'hypothèse est correcte, la version corrigée ressemblera à ceci:
public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
PVS-Studio Warning: V3008 La variable 'content' reçoit des valeurs successives deux fois. 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 vu deux affectations au
contenu de variable locale
. GetChildContentAsync est une méthode de la bibliothèque qui n'est pas assez courante pour que nous puissions la décider et l'annoter. Donc, malheureusement, ni nous ni l'analyseur ne savons quoi que ce soit sur l'objet retourné de la méthode et les effets secondaires. Mais nous pouvons certainement dire que l'attribution d'un résultat au
contenu n'a pas de sens sans utilisation. Ce n'est peut-être pas du tout une erreur, juste une opération supplémentaire. Je ne pouvais pas arriver à une conclusion sans ambiguïté sur la façon de le réparer. Je laisserai cette décision aux développeurs.
Avertissement 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; }
Dans ce code, l'analyseur a détecté un
élément dangereux de déréférencement
itemTag . Un bon exemple de la différence entre un analyseur statique et une personne testée. La méthode a un paramètre nommé
ItemTag et une variable locale appelée
itemTag . Comme vous le savez, la différence est énorme pour le compilateur! Ce sont deux variables différentes, quoique liées,. La connexion passe par la troisième variable,
itemTagName. Le scénario pour
lever une exception est le suivant: l'argument
ItemTag est "-", le
itemTagName ne reçoit pas de valeur, il restera une référence null et s'il s'agit d'une référence null, le
itemTag local deviendra également une référence null. Je pense qu'il est préférable de lever une exception ici après avoir vérifié 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; }
PVS-Studio Warning: 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 vu le déréférencement de
remoteClient et vérifie la valeur
null ci-dessous. Il s'agit vraiment d'une
NullReferenceException potentielle, car la méthode
FirstOrDefault peut renvoyer la valeur par défaut (
null pour les types de référence). Je suppose que pour corriger le code, transférez simplement la vérification ci-dessus:
public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
Cependant , il peut être utile de remplacer
FirstOrDefault par
First et de supprimer complètement le chèque.
Depuis PVS-Studio 7.05 Beta:Pour le moment, nous avons annoté toutes les méthodes
orDefault de
LINQ . Ces informations seront utilisées par de nouveaux diagnostics, notant le déréférencement du résultat de l'appel de ces méthodes sans vérification. Pour chaque méthode
orDefault , il existe un analogue qui lève une exception si un élément approprié n'a pas été trouvé. Cette exception aidera davantage à comprendre le problème que la
NullReferenceException abstraite.
Je ne peux que partager le résultat des diagnostics développés sur ce projet. 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);
PVS-Studio Warning :
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(....); } .... }
Ainsi, l'analyseur a noté le déréférencement du résultat de l'appel de la méthode
CreateScope . La méthode
CreateScope est très petite, je vais la donner dans son intégralité:
public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this);
Comme vous pouvez le voir, il peut retourner
null dans deux cas. L'analyseur ne peut pas dire dans quelle branche le programme passera, il marque donc le code comme suspect. Dans mon code, j'ajouterais immédiatement une vérification
nulle .
Je juge peut-être biaisé, mais toutes les méthodes asynchrones doivent être assurées contre
NullReferenceException le mieux possible. Déboguer de telles choses est un plaisir très douteux.
Dans ce cas, la méthode
CreateScope a quatre appels et dans deux il y a un contrôle, mais dans les deux autres il est manquant. De plus, une paire sans vérification est similaire au copier-coller (une classe, une méthode, déréférencée pour appeler UsingAsync). J'ai déjà effectué le premier appel à partir de cette paire et, bien sûr, le deuxième appel a également un avertissement similaire à l'analyseur:
V3080 Déréférence nulle possible de la valeur de retour de la méthode. Pensez à inspecter: CreateScope (). SetupService.cs 192
Avertissement 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); .... }
Copier-coller classique.
ConsumerSecret a été vérifié deux fois, et
AccessTokenSecret - pas une seule fois. De toute évidence, vous devez corriger:
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); .... }
Avertissement PVS-Studio: V3139 Deux ou plusieurs branches de cas effectuent les mêmes actions. SerialDocumentExecuter.cs 23
Une autre erreur de copier-coller. Pour plus de clarté, je vais donner toute la classe, car 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 a considéré deux branches de
cas identiques comme suspectes. En effet, il y a trois entités dans la classe, deux reviennent en faisant le tour, une non. Si cela est prévu et que la troisième option est abandonnée, vous pouvez la supprimer en combinant les deux branches 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'une erreur de copier-coller, vous devez corriger le champ renvoyé comme ceci:
switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Ou vice versa. Je ne connais pas le projet et je ne peux pas corréler les noms des types d'opérations et de stratégies.
switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
Avertissement 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; .... }
Dans le premier bloc
if , la variable de
requête obtient une valeur autre que
null à plusieurs endroits, mais partout avec des conditions imbriquées. Je n'ai pas cité toutes ces conditions, car l'exemple aurait été trop lourd. Les premières conditions qui vérifient le type http de la méthode
IsGet ou
IsPost sont suffisantes . La classe
Microsoft.AspNetCore.Http.HttpMethods possède neuf méthodes statiques pour vérifier le type de demande. Cela signifie que si, par exemple, une demande
Delete ou
Set entre dans la méthode
ExecuteAsync , une
exception NullReferenceException sera levée. Même si pour le moment de telles méthodes ne sont pas du tout prises en charge - il est préférable de faire une vérification avec une exception. Après tout, les exigences du système peuvent changer. Exemple de vérification ci-dessous:
private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
PVS-Studio Warning :
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
déclencheurs de diagnostic
V3080 sont plus faciles à voir dans l'environnement de développement. Besoin d'une navigation par méthode, d'une mise en surbrillance du type, d'une atmosphère IDE encourageante. J'essaie de garder les exemples aussi courts que possible afin que vous soyez plus à l'aise de lire. Mais si cela ne fonctionne pas pour moi, ou si vous voulez vérifier votre niveau de programmation et le découvrir par vous-même, je vous conseille de regarder comment ce diagnostic fonctionne sur n'importe quel projet ouvert ou sur votre propre projet.
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
L'analyseur ne jure que sur cette ligne. Regardons la méthode
Get: public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }
Il provoque sa surcharge. Regardons-la:
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 à la réception d'un élément de
Data par l'indexeur de
nom, nous obtenons une entité d'un type incompatible avec
JObject , alors la méthode
Get retournera
null . Je
n'oserai pas juger de la probabilité de cela, car ces types proviennent de la bibliothèque
Newtonsoft.Json , et j'ai une petite expérience avec. Mais le développeur soupçonnait que l'élément souhaité pourrait ne pas l'être. N'oubliez donc pas cette possibilité lorsque vous vous référez aux résultats de lecture. J'ajouterais un lancer d'exception au tout premier
Get , si l'on considère qu'il devrait y avoir un nœud, ou vérifier avant le déréférencement, si l'absence du nœud ne change pas la logique générale (la valeur par défaut est prise, par exemple).
Première option:
public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
Deuxième option:
public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
Avertissement 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) { .... } .... }
Un exemple assez simple par rapport au précédent. L'analyseur considère que le résultat de l'appel de
QueryAsync peut être une référence nulle. Voici la méthode:
public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
Ici
GetQueryAsync est une méthode d'interface, vous ne pouvez pas être sûr de chaque implémentation. De plus, dans le même projet, il existe une telle option:
public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
L'analyse de la méthode
GetDocumentAsync est compliquée par l'abondance d'appels de fonctions externes et d'accès au cache. Insistons sur le fait qu’il vaut la peine d’ajouter un chèque. De plus, 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 noter la haute qualité du code! Oui, il y avait d'autres lacunes que je n'ai pas abordées dans cet article, mais les plus graves ont néanmoins été prises en compte. Bien sûr, cela ne signifie pas qu'un contrôle tous les trois ans est suffisant. L'avantage maximal est obtenu avec l'utilisation
régulière d'un analyseur statique, car c'est cette approche qui vous permet de détecter et de résoudre les problèmes dès les premières étapes, lorsque le coût et la complexité de l'édition sont minimes.
Bien que les vérifications ponctuelles ne soient pas aussi efficaces que possible, je vous invite à télécharger et à essayer
PVS-Studio sur votre projet - et si vous pouvez trouver quelque chose d'intéressant?
Si vous souhaitez partager cet article avec un public anglophone, veuillez utiliser le lien de traduction: Alexander Senichkin.
Analyse du code d'Orchard CMS pour les bogues .