The Orchard Core threequel. Rechecking the project with PVS-Studio

In this article, we check the Orchard Core project with the help of the PVS-Studio static analyzer. We are going to find out if the platform code is as good as the sites created on its basis. May the force of static analysis be with us!

Introduction

Orchard Core is a modular, multi-tenant, open-source application framework and CMS for ASP.NET Core. We've already checked this project twice and found interesting warnings. We even wrote articles about these warnings — click here or here if you want to learn more. Let's see if we are going to find something wonderful this time =)

The project code is available in the repository on GitHub. We check the code with the PVS-Studio static code analyzer.

The analyzer issued 281 warnings for 3791 files with the .cs extension. 54 warnings had a high level of certainty, 143 — medium level and 84 — low level. Now, let's look at the most interesting of them.

The analysis results

Issue 1

public async Task<IActionResult> LinkExternalLogin(
                   LinkExternalLoginViewModel model,
                   string returnUrl = null)
{
  ....
  var info = await _signInManager.GetExternalLoginInfoAsync();

  var email = info.Principal.FindFirstValue(ClaimTypes.Email)
           ?? info.Principal.FindFirstValue("email");
  ....

  if (info == null)
  {
    _logger.LogWarning("Error loading external login info.");
    return NotFound();
  }
  ....
}

PVS-Studio warning: V3095 The 'info' object was used before it was verified against null. Check lines: 637, 641. AccountController.cs 637

Let's start our review with a potential dereference of a null reference — "beloved" by many developers. Take a look at the Principal property of the info object which was accessed twice in a row and a null check right in the next line. Looks elegant, doesn't it? Actually, it's easy to overlook such errors during code review. Most likely, a check for null should be performed before info is dereferenced. In this case, there would be no problems.

Issue 2

public async ValueTask<Completion> WriteToAsync(
             List<FilterArgument> argumentsList,
             IReadOnlyList<Statement> statements,
             TextWriter writer,
             TextEncoder encoder,
             LiquidTemplateContext context)
{
  if (displayFor != null)
  {
    ....
  }
  else if (removeFor != null)
  {
    ....

    if (metadata.RemoveRouteValues != null)
    {  
      if (routeValues != null)
      {
        foreach (var attribute in routeValues)
        {
          metadata.RemoveRouteValues.Add(attribute.Key, attribute.Value);
        }
      } 

        ....
    }
  }
  else if (createFor != null)
  {
    ....
    var metadata = await contentManager
                   .PopulateAspectAsync<ContentItemMetadata>(createFor);

    if (metadata.CreateRouteValues == null)                       // <=
    {
      if (routeValues != null)
      {
        foreach (var attribute in routeValues)
        {
          metadata.CreateRouteValues.Add(attribute.Key,           // <=
                                         attribute.Value);     
        }
      }
      ....
    }
  }
  ....
}

PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'metadata.CreateRouteValues'. ContentAnchorTag.cs 188

I would be remiss if I didn't mention typos in repeated nested conditionals. Here, the CreateRouteValues property of the metadata object is dereferenced right in the then block, which explicitly indicates null.

To make sure it's just an unfortunate typo, just look at the similar else if conditional given above. A proper comparison operator is used there and therefore the metadata object's properties are dereferenced without any errors.

By the way, this error ranked first in our top of errors on ASP.NET Core.

Tip: During code review, check the last block of nested conditionals twice. This block may hide an insidious last line effect!

Issue 3

public async Task<IActionResult> DeleteMediaList(string[] paths)
{
  foreach (var path in paths)
  {
    ....
  }  

  if (paths == null)
  {
    return NotFound();
  }
  ....
}

PVS-Studio warning: V3095 The 'paths' object was used before it was verified against null. Check lines: 304, 312. AdminController.cs 304

This error seems more exciting. At first glance, the code looks right. Although paths is used before a null check, the code doesn't dereference a reference to this object explicitly. It's not really that simple though. During the foreach loop iteration through the collection, the loop calls the GetEnumerator method. This leads to a NullReferenceException, and the program crashes.

Tip: Always be aware of the way different language constructs function, or use a reliable code-checking software solution.

Issue 4

private async Task EnsureConfigurationAsync()
{
  ....
  var lastProviders = (_applicationConfiguration as IConfigurationRoot)
                        ?.Providers.Where(p => 
                        p is EnvironmentVariablesConfigurationProvider ||
                        p is CommandLineConfigurationProvider).ToArray();
  ....
  if (lastProviders.Count() > 0)
  {
    ....
  }
  ....
}

PVS-Studio warning: V3105 The 'lastProviders' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. ShellSettingsManager.cs 242

Although the above code fragment contains only the lastProviders object's assignment and a conditional, the error is inconspicuous. The analyzer informs us that the reference to an object assigned via a null-conditional operator is dereferenced. Indeed, lastProviders is derived from the result of _applicationConfiguration cast to IConfigurationRoot executed via as. In this case*,* lastProviders can take null if the cast is not possible. The developers execute the function via the '.?' operator on purpose. But they failed to add any checks for null in the conditional that contains a call to lastProviders.Count.

This code fragment shows a common pattern of errors found by PVS-Studio. Many developers prefer the use of null-conditional operators, instead of explicit checks for null. This approach makes the code less cumbersome and more readable. But null-conditional operators may get lost in a large code base. In this case, ominous NullReferenceException may be thrown.

Tip: Pay attention to the use of null-conditional operators. Try not to lose the sight of null

Issue 5

private async Task<string> GenerateUsername(ExternalLoginInfo info)
{
  ....
  var externalClaims = info?.Principal.GetSerializableClaims();
  ....
  foreach (var item in _externalLoginHandlers)
  {
    try
    {
      var userName = await item.GenerateUserName(
                      info.LoginProvider, externalClaims.ToArray());
      ....
    }
    ....
  }
  ....
}

PVS-Studio warning: V3105 The 'externalClaims' variable was used after it was assigned through a null-conditional operator. NullReferenceException is possible. AccountController.cs 786

The analyzer warns about the potentially dangerous use of the externalClaims variable assigned through a null-conditional operator. As in the previous case, there is no protection against dereference of the null reference.

Issue 6

public async Task ShouldDiscardDraftThenCreateNewPublishedContentItemVersion()
{
  using (var context = new BlogPostDeploymentContext())
  {
    ....
    await shellScope.UsingAsync(async scope =>
    {
      ....
      var originalVersion = blogPosts.FirstOrDefault(x => 
           x.ContentItemVersionId == context.OriginalBlogPostVersionId);
      Assert.False(originalVersion?.Latest);
      Assert.False(originalVersion?.Published);

      var draftVersion = blogPosts.FirstOrDefault(x => 
           x.ContentItemVersionId == draftContentItemVersionId);
      Assert.False(draftVersion?.Latest);
      Assert.False(draftVersion?.Published);

      var newVersion = blogPosts.FirstOrDefault(x => 
           x.ContentItemVersionId == "newversion");
      Assert.Equal("new version", newVersion.DisplayText);           // <=
      Assert.True(newVersion?.Latest);                               // <=
      Assert.True(newVersion?.Published);                            // <=
    });
  }
}

PVS-Studio warning: V3095 The 'newVersion' object was used before it was verified against null. Check lines: 94, 95. BlogPostCreateDeploymentPlanTests.cs 94

This piece of code shows what all developers are so afraid of — copy-paste errors. Here a developer forgot to use the null-conditional operator when a program accessed the newVersion object. Therefore, when a program accesses the DisplayText property, NullReferenceException can be thrown.

This happened, most likely, when a developer copied similar blocks of code contained the '?.' operator. However, when a new line with the newVersion object had appeared, the null-conditional operator magically disappeared.

Tip: When you copy code, it is worth to spend more time on code review. You can also use a static analyzer to ease the process of code review.

Worthy of mention

As I said earlier, we have checked the Orchard project twice (here and here). Great that developers fixed all the errors found during the first check. However, after the second check some errors left unfixed. The PVS-Studio team feels obligated to point out these potential errors again.

Let's start with the following interesting example:

public async Task<IActionResult> Import(ImportViewModel model)
{
  ....
  var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(
                             x => x.ClientName == model.ClientName);
  var apiKey = Encoding.UTF8.GetString( _dataProtector.Unprotect(   
                                     remoteClient.ProtectedApiKey)); // <=

  if (remoteClient == null ||                                        // <=
      model.ApiKey != apiKey || 
      model.ClientName != remoteClient.ClientName)
  {
    return StatusCode((int)HttpStatusCode.BadRequest, 
                        "The Api Key was not recognized");
  }
  ....
}

PVS-Studio warning: V3095 The 'remoteClient' object was used before it was verified against null. Check lines: 46, 48. ImportRemoteInstanceController.cs 46

The analyzer informs us that remoteClient is dereferenced before its check for null. Most likely, the check should be performed before the dereference. Otherwise, NullReferenceException will be thrown.

Also, in the previous check, we supposed that a check for null was unnecessary, and instead of the FirstOrDefault method, it is better to use just First. This solution seemed reasonable. Although three years later the analyzer issued a warning on this code fragment again...

In the project code, the FirstOrDefault method is used without any checks for null (more on that later). Here we have an explicit check for null. So, you just need to replace the conditional and the apiKey assignment here.

Now let's take a look not at the warning itself but at the recommendation:

private async Task ExecuteAsync(HttpContext context, ....)
{
  ....
  GraphQLRequest request = null;
  ....
  if (HttpMethods.IsPost(context.Request.Method))
  {
    ....
    request = ....;
    ....
  }
  else if (HttpMethods.IsGet(context.Request.Method))
  {
    ....
    request = new GraphQLRequest();
    ....
  }
  var queryToExecute = request.Query;
  ....
}

PVS-Studio warning: V3080 Possible null dereference. Consider inspecting 'request'. GraphQLMiddleware.cs 157

The request object is initialized in each of nested conditionals. You can find the full code here. Let's take a look at the first two conditions that check the request type for compliance with IsPost and IsGet. As mentioned in the previous article, the Microsoft.AspNetCore.HttpMethods class has nine static methods to check the request type. Thus, when an unknown request is passed, NullReferenceException will be thrown.

Of course, this is not a mistake but a decision to cover only those features of the program that the developers use. However, we still would like to draw developers' attention to such cases. In the future, this may save them from the exhausting search for the crashed place in program.

Moreover, a null check and an exception throw take only a few lines =).

Let's look at the last, but not the least amusing error in this chapter:

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

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 51

It would seem just another copy paste mistake, but what an annoying one! Instead of the consumerSecret check in the second condition, it's better to check AccessTokenSecret, because AccessTokenSecret was not checked at all. However, the then block clearly indicates — the check should be here. The fixed version might look 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);
  ....
}

To conclude this section, I would like to note that the above code fragments have not been fixed for a long time. Perhaps these pieces of code contain not errors but only poorly written fragments in fully functional and secure code.

Whether the code fragments described in this article contain errors or not - it would be great if developers reviewed the code again. If you think that the analyzer would bombard you with false positives for such extraordinary code fragment, then we hurry to reassure you. PVS-Studio has a reliable mechanism for suppressing false positives that will not let you suffer=).

FirstOrDefault — love at first sight

Well, we should consider one more analyzer's warning. The analyzer mentioned a dereference of the value returned by the FirstOrDefault method without any checks for null in 39 code fragments. Take a look at the following code fragment:

public async Task<IActionResult> AddContentItem(int deploymentPlanId,
                                                string returnUrl, 
                                                string contentItemId)
{
  var step = (ContentItemDeploymentStep)_factories.FirstOrDefault(x => 
              x.Name == nameof(ContentItemDeploymentStep)).Create();
  ....
}

PVS-Studio warning: V3146 Possible null dereference. The '_factories.FirstOrDefault' can return default null value. AddToDeploymentPlanController.cs 77

The analyzer warns us that the FirstOrDefault method may return null. This would lead to a NullReferenceException. Most likely, developers do not expect null to appear during execution, so they thought no checks were required. But why not First? Because the default value may still appear? Then, where is a check for null? In fact, the analyzer found 39 such cases!

Tip: Use First instead of FirstOrDefault where the sequence contains at least one element. This approach will make code more readable. Make your code as attractive as the websites created with Orchard! =)

Conclusion

As in the previous article, I would like to mention the high quality of the Orchard project code base! It's been three years. And this time we found some warnings that we had already described in our previous articles. However, the developers did a really great job over these years.

Of course, we will continue to check open-source projects and watch how developers find and fix errors after a long period of time. But the inspection of code every three years is not enough. If you want to get the maximum from using a static code analyzer, use it regularly.

Check your project with our analyzer! Perhaps you'll find really interesting warnings.