We search and analyze errors in the Orchard CMS code

This article is the result of re-checking the Orchard project using the PVS-Studio static analyzer. Orchard is an open source content management system that is part of the Outercurve Foundation, a nonprofit ASP.NET gallery of projects. The test is interesting in that the project and the analyzer have grown significantly since the first analysis. New positives and interesting bugs are waiting for you.



About Orchard CMS



We tested Orchard with PVS-Studio three years ago. The C # analyzer has since developed greatly: we have improved data flow analysis, developed interprocedural analysis, added new diagnostics and fixed a number of false positives. In addition, the analysis showed that all the comments from the previous article were corrected. This means that the goal is achieved - the code has become better.



I want to believe that the developers will pay attention to this article and make the necessary changes. Even better, if you introduce the practice of regular use of PVS-Studio. Let me remind you that for open-source projects we provide a free version of the license. By the way, there are other options that are suitable for closed projects.



Orchard project code can be downloaded from here , as I did. A full description of the project can be found here . If you do not have our PVS-Studio analyzer yet, you can download a trial version from here . I used the version of PVS-Studio version 7.05 Beta. I will share the results of her work. I hope you agree with the usefulness of using PVS-Studio. The main thing is to use the analyzer regularly .



Validation Results



I’ll give you some figures from the last article so that you don’t have to switch to compare.



“All files (3739 pieces) with the .cs extension took part in the last check. In total, they contained 214,564 lines of code. Based on the audit results, 137 warnings were received. At the first (high) confidence level, 39 warnings were received. At the second level (average), 60 warnings were received. ”



At the moment, the project has 2767 .cs files, that is, the project has become less than a thousand files. Judging by this decrease in the number of files and the change of repository name, a kernel was allocated from the project ( commit 966 ). There are 108,287 lines of code in the kernel and the analyzer generated 153 warnings, 33 at a high level, 70 at an average. We usually do not consider warnings of the third level, I did not break the tradition.



PVS-Studio Warning : V3110 Possible infinite recursion inside 'TryValidateModel' method. PrefixedModuleUpdater.cs 48

public bool TryValidateModel(object model, string prefix) { return TryValidateModel(model, Prefix(prefix)); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

As in the previous article, we open the list of errors with infinite recursion. It is difficult to say what exactly the developer wanted to do in this case. But I noticed that the TryValidateModel method has a single argument overload that looks like this:

 public bool TryValidateModel(object model) { return _updateModel.TryValidateModel(model); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

I assume that, as with the overloaded method, the developer wanted to call methods via _updateModel. The compiler did not see an error, _updateModel is of type IUpdateModel and the current class also implements this interface. Since there is not a single condition in the method to protect against a StackOverflowException , the method may not have been called even once, but I would not count on it. If the assumption is correct, the corrected version will look like this:

 public bool TryValidateModel(object model, string prefix) { return _updateModel.TryValidateModel(model, Prefix(prefix)); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning: V3008 The 'content' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 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); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer saw two assignments to the local variable content. GetChildContentAsync is a method from the library that is not common enough for us to decide on and annotate it. So, unfortunately, neither we nor the analyzer know anything about the returned method object and side effects. But we can definitely say that assigning a result in content does not make sense without use. Perhaps this is not a mistake at all, just an extra operation. I could not come to an unambiguous conclusion about the method of correction. I will leave this decision to the developers.



PVS-Studio Warning : V3080 Possible null dereference. Consider inspecting '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; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

In this code, the analyzer detected a dangerous dereferencing itemTag . A good example of the difference between a static analyzer and a testing person. The method has a parameter named ItemTag and a local variable called itemTag . As you know, the difference is huge for the compiler! These are two different, albeit related, variables. The connection goes through the third variable, itemTagName. The scenario for throwing an exception is this: the ItemTag argument is "-", the itemTagName is not assigned a value, it will remain a null reference, and if it is a null reference, the local itemTag will also become a null reference. I think it's better to throw an exception here after checking the string.

 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 The 'remoteClient' object was used before it was verified against null. Check lines: 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 || ....) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer saw the dereferencing remoteClient and checking for null below. This is really a potential NullReferenceException , because the FirstOrDefault method can return the default value ( null for reference types). I suppose, to fix the code, just transfer the check above:

 public async Task<IActionResult> Import(ImportViewModel model) { .... var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....); if (remoteClient != null) var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey); else if (....) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Although, it may be worth replacing FirstOrDefault with First and removing the check completely.



From PVS-Studio 7.05 Beta:



At the moment, we have annotated all orDefault methods from LINQ . This information will be used by new diagnostics, noting the dereferencing of the result of calling these methods without checking. For each orDefault method there is an analogue that throws an exception if a suitable element was not found. This exception will help more in understanding the problem than the abstract NullReferenceException .



I cannot but share the result of the developed diagnostics on this project. 27 potentially dangerous places. Here are some of them:



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 Possible null dereference of method return value. Consider inspecting: CreateScope (). SetupService.cs 136

 public async Task<string> SetupInternalAsync(SetupContext context) { .... using (var shellContext = await ....) { await shellContext.CreateScope().UsingAsync(....); } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

So, the analyzer noted the dereferencing of the result of calling the CreateScope method. The CreateScope method is quite small, I will give it in its entirety:

 public ShellScope CreateScope() { if (_placeHolder) { return null; } var scope = new ShellScope(this); // A new scope can be only used on a non released shell. if (!released) { return scope; } scope.Dispose(); return null; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

As you can see, it can return null in two cases. The analyzer cannot say which branch the program will go through, so it marks the code as suspicious. In my code, I would immediately add a null check.



Perhaps I judge biased, but all asynchronous methods should be insured against NullReferenceException as best as possible. Debugging such things is a very dubious pleasure.



In this case, the CreateScope method has four calls and in two there is a check, but in the other two it is missing. Moreover, a pair without verification is similar to copy-paste (one class, one method, dereferenced to call UsingAsync). I already made the first call from this pair and, of course, the second call also has a similar analyzer warning:



V3080 Possible null dereference of method return value. Consider inspecting: CreateScope (). SetupService.cs 192



PVS-Studio Warning: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'AccessTokenSecret' variable should be used instead of '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); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Classic copy-paste. ConsumerSecret was checked twice, and AccessTokenSecret - not once. Obviously, you need to fix:

 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); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning: V3139 Two or more case-branches perform the same actions. SerialDocumentExecuter.cs 23



Another copy-paste mistake. To make it clearer, I will give the whole class, since it is small.

 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 ....; } } }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer considered two identical case branches suspicious. Indeed, there are three entities in the class, two are returned when going around, one is not. If this is planned and the third option is abandoned, then you can delete it by combining the two branches as follows:

 switch (context.Operation.OperationType) { case OperationType.Query: case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

If this is a copy-paste error, you need to correct the returned field like this:

 switch (context.Operation.OperationType) { case OperationType.Query: return ParallelExecutionStrategy; case OperationType.Mutation: return SerialExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Or vice versa. I am not familiar with the project and cannot correlate the names of the types of operations and strategies.

 switch (context.Operation.OperationType) { case OperationType.Query: return SerialExecutionStrategy; case OperationType.Mutation: return ParallelExecutionStrategy; case OperationType.Subscription: return SubscriptionExecutionStrategy; default: throw ....; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3080 Possible null dereference. Consider inspecting 'request'. 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; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

In the first if block, the request variable gets a value other than null in several places, but everywhere with nested conditions. I did not cite all these conditions, since the example would have been too cumbersome. The first conditions that check the http type of the IsGet or IsPost method are enough . The Microsoft.AspNetCore.Http.HttpMethods class has nine static methods for checking the type of request. This means that if, for example, a Delete or Set request gets into the ExecuteAsync method, a NullReferenceException will be thrown. Even if at the moment such methods are not supported at all - it is better to do a check with an exception. After all, the requirements for the system may change. Verification example below:

 private async Task ExecuteAsync(HttpContext context....) { .... if (request == null) throw ....; var queryToExecute = request.Query; .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3080 Possible null dereference of method return value. Consider inspecting: Get <ContentPart> (...). ContentPartHandlerCoordinator.cs 190



Most V3080 diagnostic triggers are easier to see in the development environment. Need method navigation, type highlighting, an encouraging IDE atmosphere. I try to keep the examples as short as possible so that you are more comfortable reading. But if it doesn’t work out for me, or if you want to check your programming level and figure it out on your own, I advise you to see how this diagnostic works on any open or your own project.

 public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) .Weld(fieldName, fieldActivator.CreateInstance()); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analyzer swears on this line. Let's look at the Get method :

 public static TElement Get<TElement>(this ContentElement contentElement....) where TElement : ContentElement { return (TElement)contentElement.Get(typeof(TElement), name); }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

It causes its overload. Let's look at her:

 public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { return null; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

It turns out that if upon receiving an element from Data by the name indexer we get an entity of a type incompatible with JObject , then the Get method will return null . I will not venture to judge the likelihood of this, since these types are from the Newtonsoft.Json library, and I have a little experience with it. But the developer suspected that the desired item might not be. So, do not forget about this possibility when referring to the reading results. I would add an exception throw to the very first Get if we think that the node should be, or check before dereferencing, if the absence of the node does not change the general logic (the default value, for example) is taken.



Option one:

 public static ContentElement Get(this ContentElement contentElement....) { .... var elementData = contentElement.Data[name] as JObject; if (elementData == null) { throw.... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Option two:

 public override async Task LoadingAsync(LoadContentContext context) { .... context.ContentItem.Get<ContentPart>(typePartDefinition.Name) ?.Weld(fieldName, fieldActivator.CreateInstance()); .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

PVS-Studio Warning : V3080 Possible null dereference. Consider inspecting 'results'. ContentQueryOrchardRazorHelperExtensions.cs 19

 public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); .... foreach (var result in results) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

A fairly simple example relative to the previous one. The analyzer considers that the result of calling QueryAsync may be a null reference. Here is the method:

 public static async Task<IEnumerable> QueryAsync(....) { .... var query = await queryManager.GetQueryAsync(queryName); if (query == null) { return null; } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Here GetQueryAsync is an interface method, you cannot be sure of every implementation. Moreover, in the same project there is such an option:

 public async Task<Query> GetQueryAsync(string name) { var document = await GetDocumentAsync(); if(document.Queries.TryGetValue(name, out var query)) { return query; } return null; }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

The analysis of the GetDocumentAsync method is complicated by the abundance of calls to external functions and cache accesses. Let us dwell on the fact that it’s worth adding a check. Moreover, the method is asynchronous.

 public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....) { var results = await orchardHelper.QueryAsync(queryName, parameters); if(results == null) throw ....; .... foreach (var result in results) { .... } .... }
      
      

        
        
        
      

    
        
        
        
      
      

        
        
        
      

    
     

Conclusion



I can not help but note the high quality of the code! Yes, there were other shortcomings that I did not touch upon in this article, but the most serious ones were nevertheless considered. Of course, this does not mean that a check every three years is enough. The maximum benefit is achieved with the regular use of a static analyzer, since it is this approach that allows you to detect and fix problems at the earliest stages, when the cost and complexity of editing is minimal.



Although one-time checks are not as effective as possible, I urge you to download and try PVS-Studio on your project - what if you can find something interesting?

If you want to share this article with an English-speaking audience, then please use the link to the translation: Alexander Senichkin. Scanning the code of Orchard CMS for Bugs .