Alexander Senichkin

Oct 21 2019

Tags:

#CSharp

Scanning the code of Orchard CMS for bugs. Part 2

Oct 21 2019

Author: Alexander Senichkin

This article reviews the results of a second check of the Orchard project with the PVS-Studio static analyzer. Orchard is an open-source content manager system delivered as part of the ASP.NET Open Source Gallery under the non-profit Outercurve Foundation. Today's check is especially interesting because both the project and the analyzer have come a long way since the first check, and this time we'll be looking at new diagnostic messages and some nice bugs.

About Orchard CMS

We checked Orchard three years ago. PVS-Studio's C# analyzer has greatly evolved since then: we have improved the data flow analysis, added interprocedural analysis and new diagnostics, and fixed a number of false positives. More than that, the second check revealed that the developers of Orchard had fixed all the bugs reported in the first article, which means we had achieved our goal, i.e. had helped them make their code better.

I hope they will pay attention to this article as well and make the necessary fixes or, better still, adopt PVS-Studio for use on a regular basis. As a reminder, we provide open-source developers with a free license. By the way, there are other options that proprietary projects may enjoy as well.

Orchard's source code is available for download here. The complete project description is found here. If you don't have a PVS-Studio copy yet, you can download the trial version. I used PVS-Studio 7.05 Beta and will include some of its warnings in this article. I hope this review will convince you that PVS-Studio is a useful tool. Just keep in mind that it's meant to be used regularly.

Analysis results

Here are some of the figures from the first check of Orchard so that you don't have to switch between the two articles for comparison.

During the previous check, "we did the analysis of all source code files (3739 items) with the .cs extension. In sum total there were 214,564 lines of code. The result of the check was 137 warnings. To be more precise, there were 39 first (high) level warnings. There were also 60 second (medium) level warnings."

The current version of Orchard is made up of 2,767 .cs files, i.e. it's about one thousand files smaller. The downsizing and renaming of the repository suggests that the developers have isolated the project's core (commit 966), which is 108,287 LOC long. The analyzer issued 153 warnings: 33 first-level and 70 second-level ones. We don't typically include third-level warnings, and I'm going to stick to the tradition.

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

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

Let's start off with an infinite recursion bug, as we did in the first article. This time the exact intentions of the developer aren't clear, but I noticed that the TryValidateModel method had an overloaded version with one parameter:

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

I think that, just like in the case of the overloaded version, the developer intended to call the method through _updateModel. The compiler didn't notice the mistake; _updateModel is of type IUpdateModel, and the current class also implements this interface. Since the method doesn't include any check against StackOverflowException, it was probably never called, though I wouldn't count on that. If my assumption is correct, the fixed version should look like this:

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

PVS-Studio diagnostic message: 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 detected two assignments to the local variable content. GetChildContentAsync is a library method that is used too rarely for us to take the trouble to examine and annotate it. So, I'm afraid, neither we, nor the analyzer know anything about the method's return object and side effects. But we know for sure that assigning the return value to content makes no sense if it's not used further in the code. Perhaps it's just a redundant operation rather than a mistake. I can't say how exactly this should be fixed, so I leave it to the developers.

PVS-Studio diagnostic message: 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;
}

The analyzer detected an unsafe dereference of itemTag. This snippet is a good example of how a static analysis tool is different from a human developer doing code review. The method has a parameter named ItemTag and a local variable named itemTag. No need to tell you it makes a huge difference to the compiler! These are two different, even though related, variables. The way they are related is through a third variable, itemTagName. Here's the sequence of steps leading to the possible exception: if the ItemTag argument is equal to "-", no value will be assigned to itemTagName, so it will remain a null reference, and if it's a null reference, then the local variable itemTag will turn into a null reference too. In my opinion, it's better to have an exception thrown following the string check.

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 diagnostic message: 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 detected a dereference of remoteClient followed by a null check a couple of lines later. This is indeed a potential NullReferenceException as the FirstOrDefault method may return a default value (which is null for reference types). I guess this snippet can be fixed by simply moving the check up so that is precedes the dereference operation:

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

Or perhaps it should be fixed by replacing FirstOrDefault with First and removing the check altogether.

Warnings by PVS-Studio 7.05 Beta:

By now, we have annotated all of LINQ's orDefault methods. This information will be used by the new diagnostic we are working on: it detects cases where the values returned by these methods are dereferenced without a prior check. Each orDefault method has a counterpart that throws an exception if no matching element has been found. This exception will be more helpful in tracking down the problem than the abstract NullReferenceException.

I can't but share the results I've got from this diagnostic on the Orchard project. There are 27 potentially dangerous spots. 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 diagnostic message: 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(....);
  }
  ....
}

The analyzer mentioned a dereference of the value returned by the CreateScope method. CreateScope is a tiny method, so here's its complete implementation:

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, there are two cases where it can return null. The analyzer doesn't know which branch the flow of execution will follow, so it plays safe and reports the code as suspicious. If I were to write code like that, I'd write a null check right away.

Perhaps my opinion is biased, but I believe that every asynchronous method should be protected from NullReferenceException as much as possible because debugging stuff like that is far from enjoyable.

In this particular case, the CreateScope method is called four times: two of those calls are accompanied by checks and the other two are not. The latter two calls (without checks) seem to be copy-paste clones (same class, same method, same way of dereferencing the result to call UsingAsync). The first of those two calls was shown above, and you may be sure the second one triggered the same warning:

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

PVS-Studio diagnostic message: 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);
  ....
}

That's a classic copy-paste mistake. ConsumerSecret was checked twice, while AccessTokenSecret was not checked at all. Obviously, this is fixed as follows:

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 diagnostic message: V3139 Two or more case-branches perform the same actions. SerialDocumentExecuter.cs 23

Another copy-paste bug. For clarity, here's the complete class implementation (it's 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 didn't like the two identical case branches. Indeed, the class has three entities, while the switch statement returns only two of them. If this behavior is intended and the third entity isn't actually meant to be used, the code can be improved by removing the third branch having merged the two of them 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 bug, the first of the duplicate return fields should be fixed as follows:

switch (context.Operation.OperationType)
{
  case OperationType.Query:
    return ParallelExecutionStrategy;

  case OperationType.Mutation:
    return SerialExecutionStrategy;

  case OperationType.Subscription:
    return SubscriptionExecutionStrategy;

  default:
    throw ....;
}

Or it should be the second case branch. I don't know the project's details and therefore can't determine the correlation between the names of the operation types 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 diagnostic message: 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;
  ....
}

The request variable is assigned a value different from null several times in the first if block, but each time with nested conditions. Including all those conditions would make the example too long, so we'll just go with the first few ones, which check the type of the http method IsGet or IsPost. The Microsoft.AspNetCore.Http.HttpMethods class has nine static methods for checking the query type. Therefore, passing, for example, a Delete or Set query to the ExecuteAsync method would lead to raising a NullReferenceException. Even if such methods are currently not supported at all, it would still be wise to add an exception-throwing check. After all, the system requirements may change. Here's an example of such a check:

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

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

Most of the V3080 warnings are more convenient to view within the development environment because you need the method navigation, type highlighting, and the friendly atmosphere of the IDE. I'm trying to reduce the text of examples as much as possible to keep them readable. But if I'm not doing it right or if you want to test your programming skill and figure it all out for yourself, I recommend checking out this diagnostic's result on any open-source project or just your own code.

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

The analyzer reports this line. Let's take a look at the Get method:

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

It calls its overloaded version. Let's check it too:

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

It turns out that if we get an entity of a type incompatible with JObject from Data using the name indexer, the Get method will return null. I don't know for sure how likely that is because these types are from the Newtonsoft.Json library, which I haven't worked much with. But the author of the code was suspecting that the sought element might not exist, so we should keep that in mind when accessing the result of the read operation as well. Personally, I'd have an exception thrown in the very first Get if we believe the node must be present, or add a check before the dereference if the node's non-existence doesn't change the overall logic (for example, we get a default value).

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

PVS-Studio diagnostic message: 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)
  {
    ....
  }
 ....
}

This is quite a simple example in comparison with the previous one. The analyzer suspects that the QueryAsync method might return a null reference. Here's the method's implementation:

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

Since GetQueryAsync is an interface method, you can't be sure about each implementation, especially if we consider that the project also includes the following version:

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

The multiple calls to external functions and cache accesses make the analysis of GetDocumentAsync difficult, so let's just say that the check is needed - all the more so because the method is an asynchronous one.

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't but mention the high quality of Orchard's code! True, there were some other defects, which I didn't discuss here, but I showed you all of the most severe bugs. Of course, this is not to say that checking your source code once in three years is enough. You'll get the most out of static analysis if you use it regularly because this is the way you are guaranteed to catch and fix bugs at the earliest development stages, where bug fixing is cheapest and easiest.

Even though one-time checks don't help much, I still encourage you to download PVS-Studio and try it on your project: who knows, maybe you'll find some interesting bugs too.

#CSharp