Six years ago, we first checked Umbraco with the PVS-Studio static analyzer for C#. Today, we decided to go where it all started and analyze the Umbraco CMS source code.

Introduction

As you guessed from the title, we wrote two articles about the Umbraco project check.

Take a look at how the error types changed with time.

If you are interested in this article, you probably know about Umbraco. Still, let me remind you. Umbraco is an open-source content management system that provides great experience of editing website content. You can find the source code on GitHub.

Let me also remind you about PVS-Studio. ;)

PVS-Studio is a static analysis tool for improving code quality, safety (SAST), and security. It works with C, C++, C#, and Java languages and runs on Windows, Linux, and macOS.

We chose the Umbraco project version of 12.11.2021 on GitHub. The PVS-Studio version used — 7.15.54288.

As usual, we selected the most interesting warnings for this article. Some of them point to obvious errors. Some point to the suspicious code. But let's get down to business and look at what we found.

How are the warnings doing?

Issue 1

Can you find an error in this fragment?

protected virtual string VisitMethodCall(MethodCallExpression m)
{
  ....
  case "SqlText":
    if (m.Method.DeclaringType != typeof(SqlExtensionsStatics))
      goto default;
    if (m.Arguments.Count == 2)
    {
      var n1 = Visit(m.Arguments[0]);
      var f = m.Arguments[2];
      if (!(f is Expression<Func<string, string>> fl))
        throw new NotSupportedException("Expression is not a proper 
                                         lambda.");
      var ff = fl.Compile();
      return ff(n1);
    }
    else if (m.Arguments.Count == 3)
    {
      var n1 = Visit(m.Arguments[0]);
      var n2 = Visit(m.Arguments[1]);
      var f = m.Arguments[2];
      if (!(f is Expression<Func<string, string, string>> fl))
        throw new NotSupportedException("Expression is not a proper 
                                         lambda.");
      var ff = fl.Compile();
      return ff(n1, n2);
    }
    else if (m.Arguments.Count == 4)
    {
      var n1 = Visit(m.Arguments[0]);
      var n2 = Visit(m.Arguments[1]);
      var n3 = Visit(m.Arguments[3]);
      var f = m.Arguments[3];
      if (!(f is Expression<Func<string, string, string, string>> fl))
        throw new NotSupportedException("Expression is not a proper 
                                         lambda.");
      var ff = fl.Compile();
      return ff(n1, n2, n3);
    }
    else
      throw new NotSupportedException("Expression is not a proper lambda.");   
  ....
}

Okay-okay, now look at the shortened version of the code.

protected virtual string VisitMethodCall(MethodCallExpression m)
{
  ....
  case "SqlText":
    ....
    if (m.Arguments.Count == 2)
    {
      var n1 = Visit(m.Arguments[0]);
      var f = m.Arguments[2];
      ....
    }
}

PVS-Studio warning: V3106 Possibly index is out of bound. The '2' index is pointing beyond 'm.Arguments' bound. ExpressionVisitorBase.cs 632

I think every developer made such mistakes at least once. The developers check that m.Arguments.Count equals 2, and immediately after that they try to access the third element. Obviously, this leads to IndexOutOfRangeException.

We found similar errors in other projects. As you see, Umbraco is no exception.

Issue 2

Let's test your attention-paying abilities. Try to find an error here yourself. The code fragment is followed by a picture. Only after that you can read the correct answer.

public static string ToXmlString(this object value, Type type)
{
  if (value == null) return string.Empty;
  if (type == typeof(string)) 
    return (value.ToString().IsNullOrWhiteSpace() ? "" : value.ToString());
  if (type == typeof(bool)) return XmlConvert.ToString((bool)value);
  if (type == typeof(byte)) return XmlConvert.ToString((byte)value);
  if (type == typeof(char)) return XmlConvert.ToString((char)value);
  if (type == typeof(DateTime)) return XmlConvert.ToString((DateTime)value,
  XmlDateTimeSerializationMode.Unspecified);
  if (type == typeof(DateTimeOffset)) 
    return XmlConvert.ToString((DateTimeOffset)value);
  if (type == typeof(decimal)) return XmlConvert.ToString((decimal)value);
  if (type == typeof(double)) return XmlConvert.ToString((double)value);
  if (type == typeof(float)) return XmlConvert.ToString((float)value);
  if (type == typeof(Guid)) return XmlConvert.ToString((Guid)value);
  if (type == typeof(int)) return XmlConvert.ToString((int)value);
  if (type == typeof(long)) return XmlConvert.ToString((long)value);
  if (type == typeof(sbyte)) return XmlConvert.ToString((sbyte)value);
  if (type == typeof(short)) return XmlConvert.ToString((short)value);
  if (type == typeof(TimeSpan)) return XmlConvert.ToString((TimeSpan)value);
  if (type == typeof(bool)) return XmlConvert.ToString((bool)value);
  if (type == typeof(uint)) return XmlConvert.ToString((uint)value);
  if (type == typeof(ulong)) return XmlConvert.ToString((ulong)value);
  if (type == typeof(ushort)) return XmlConvert.ToString((ushort)value);
  ....
}

If you have quickly found an error, you have an eagle eye! Look at the shortened version of the method:

public static string ToXmlString(this object value, Type type)
{
  ....
  if (type == typeof(bool)) return XmlConvert.ToString((bool)value);
  ....
  if (type == typeof(bool)) return XmlConvert.ToString((bool)value);
  ....
}

PVS-Studio issued warning V3021: There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless ObjectExtensions.cs 615

Not a very attractive code fragment for code review, right?

Looks like we got lucky and there's just an extra if statement. You can deduce this when analyzing the used and available overloads of the XmlConvert.ToString method. But not everyone is so lucky — sometimes copy-paste hides inconspicuous errors.

Issue 3

public bool FlagOutOfDateModels
{
  get => _flagOutOfDateModels;

  set
  {
    if (!ModelsMode.IsAuto())
    {
      _flagOutOfDateModels = false;
    }

    _flagOutOfDateModels = value;
  }
}

PVS-Studio issued warning V3008 The '_flagOutOfDateModels' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 54, 51. ModelsBuilderSettings.cs 54

As you see, set accessor has a check with the assignment of the _flagOutOfDateModels value. However, immediately after this check, another value is set to the same field. The if block has no practical use.

Issue 4

private bool MatchesEndpoint(string absPath)
{
  IEnumerable<RouteEndpoint> routeEndpoints = _endpointDataSource
    ?.Endpoints
    .OfType<RouteEndpoint>()
    .Where(x =>
    {
      ....
    });

  var routeValues = new RouteValueDictionary();

  RouteEndpoint matchedEndpoint = routeEndpoints
    .Where(e => new TemplateMatcher(
        TemplateParser.Parse(e.RoutePattern.RawText),
        new RouteValueDictionary())
      .TryMatch(absPath, routeValues))
    .OrderBy(c => c.Order)
    .FirstOrDefault();

  return matchedEndpoint != null;
}

PVS-Studio issued warning V3105 The 'routeEndpoints' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RoutableDocumentFilter.cs 198

Diagnostics V3105 warns about the possibility of a NullReferenceException. _endpointDataSource is checked for null with the '?.' operator. If the* _endpointDataSource* variable still contains the null value, then routeEndpoints is also null.

It's weird that we access routeEndpoints without the '?.' operator. As a result, if routeEndpoints is null, NullReferenceException will be thrown when we access this reference.

Issue 5

public void Handle(ContentCopiedNotification notification)
{
  ....
  if (relationType == null)
  {
    relationType = new RelationType(
      Constants.Conventions.RelationTypes.RelateDocumentOnCopyAlias,
      Constants.Conventions.RelationTypes.RelateDocumentOnCopyName,
      true,
      Constants.ObjectTypes.Document,
      Constants.ObjectTypes.Document);

    _relationService.Save(relationType);
  }
  ....
}

PVS-Studio warning: V3066 Possible incorrect order of arguments passed to 'RelationType' constructor. RelateOnCopyNotificationHandler.cs 32

In this case, the constructor is called, and arguments are passed to it. Let's look at its signature:

public RelationType(string name,
                    string alias,
                    bool isBidrectional,
                    Guid? parentObjectType,
                    Guid? childObjectType)

Looks like the arguments are passed in the wrong order. The RelateDocumentOnCopyAlias argument is passed to the name parameter of the constructor. The RelateDocumentOnCopy**Name is passed to the alias parameter.

Issue 6

private static async Task<Attempt<UrlInfo>> DetectCollisionAsync(....)
{
  ....
  if (pcr.IgnorePublishedContentCollisions)
  {
    logger.LogDebug(logMsg, url, uri, culture);
  }
  else
  {
    logger.LogDebug(logMsg, url, uri, culture);
  }
}

PVS-Studio warning: V3004 The 'then' statement is equivalent to the 'else' statement. UrlProviderExtensions.cs 274

The analyzer has found a construction where branches then and else are identical. The same code is executed regardless of the property value checked. Most likely, the developer copied the code and forgot to fix the method parameters.

Issue 7

public async Task<bool> IsMemberAuthorizedAsync(....)
{
  ....
  if (IsLoggedIn() == false)
  {
    allowAction = false;
  }
  else
  { 
    string username;
    ....
    username = currentMember.UserName;
    IList<string> allowTypesList = allowTypes as IList<string> ?? 
                                              allowTypes.ToList();
    if (allowTypesList.Any(allowType => allowType != string.Empty))
    {
      allowAction = allowTypesList.Select(x => x.ToLowerInvariant())
                                                .Contains(currentMember
                                                .MemberTypeAlias
                                                .ToLowerInvariant());
    }

    if (allowAction && allowMembers.Any())
    {
      allowAction = allowMembers.Contains(memberId);
    }
    ....
  }
  return allowAction;
}

PVS-Studio warning: V3137 The 'username' variable is assigned but is not used by the end of the function. MemberManager.cs 87

We noticed an interesting warning. The developer declares the username variable and assigns a value to it. After that username is never used.

Most likely the developers didn't delete it after refactoring. However, there's a probability that some logic was not implemented, or a tricky error is hidden here.

Issue 8

public async Task<ActionResult<UserDisplay>> PostInviteUser(UserInvite userSave)
{
  if (_securitySettings.UsernameIsEmail)
  {
    userSave.Username = userSave.Email;
  }
  else
  {
    var userResult = CheckUniqueUsername(userSave.Username, u => 
                                          u.LastLoginDate != default 
                                       || u.EmailConfirmedDate.HasValue);
                                         
    if (!(userResult.Result is null))
    {
      return userResult.Result;
    }

    user = userResult.Value;
  }
  user = CheckUniqueEmail(userSave.Email, u => u.LastLoginDate != default ||    
                                          u.EmailConfirmedDate.HasValue);
  ....
}

PVS-Studio warning V3008 The 'user' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 446, 444. UsersController.cs 446

In the else block of the conditional expression, the *user *value is assigned. Right after the conditional expression is completed, user is assigned again. Therefore, the previously assigned value is not used and is immediately overwritten. It's not clear whether the userResult.Value value should have been used, and some logic is missing, or it's just a redundant code. Anyway, we are a bit suspicious about this code fragment.

Issue 9

public ActionResult<PagedResult<EntityBasic>> GetPagedChildren(....
                                                               int pageNumber,
                                                               ....)
{
  if (pageNumber <= 0)
  {
    return NotFound();
  }
  ....
  if (objectType.HasValue)
  {
    if (id == Constants.System.Root &&
        startNodes.Length > 0 &&
        startNodes.Contains(Constants.System.Root) == false &&
        !ignoreUserStartNodes)
    {
      if (pageNumber > 0)  // <=
      {
        return new PagedResult<EntityBasic>(0, 0, 0);
      }
      IEntitySlim[] nodes = _entityService.GetAll(objectType.Value, 
                                                  startNodes).ToArray();
      if (nodes.Length == 0)
      {
        return new PagedResult<EntityBasic>(0, 0, 0);
      }

      if (pageSize < nodes.Length)
      {
        pageSize = nodes.Length; // bah
      }

      var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
      {
        Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
      };
      return pr;
    }
  }
}

PVS-Studio warning: V3022 Expression 'pageNumber > 0' is always true. EntityController.cs 625

The developer checks that pageNumber is less than or equal to 0. If it's true, they exit from the method. Further on, the code checks whether pageNumber is greater than 0. Of course, this condition is always true. Therefore, the method exits. The code written after the if statement (a lot of code*,* by the way) is never executed.

Here the analyzer also issued a warning about unreachable code: V3142 Unreachable code detected. It is possible that an error is present. EntityController.cs 630

Issue 10

Here an error hides in the test. You may think that it's not so important, but tests ensure that your code works in a defined way. If tests have errors, can we be sure that the program works correctly? At such moments static analysis comes to the rescue.

Public void SimpleConverter3Test()
{
  ....
  IpublishedContentType contentType1 =
    contentTypeFactory.CreateContentType(Guid.NewGuid(),
    1002, "content1", t => CreatePropertyTypes(t, 1));

  IpublishedContentType contentType2 =
    contentTypeFactory.CreateContentType(Guid.NewGuid(),
    1003, "content2", t => CreatePropertyTypes(t, 2));
  ....
  var cnt1 = new InternalPublishedContent(contentType1) // <=
  {
    Id = 1003,
    Properties = new[]
    {
      new InternalPublishedProperty {Alias = "prop1",
        SolidHasValue = true, SolidValue = "val1"}
    }
  };
  var cnt2 = new InternalPublishedContent(contentType1) // <=
  {
    Id = 1004,
    Properties = new[]
    {
      new InternalPublishedProperty {Alias = "prop2",
        SolidHasValue = true, SolidValue = "1003"}
    }
  };
}

PVS-Studio warning: V3056 Consider reviewing the correctness of 'contentType1' item's usage. ConvertersTests.cs 115

Most likely, it's a copy-paste error: contentType1 is used instead of contentType2 when we declare the cnt2 variable. Agree, it's a bit weird.

Conclusion

It was a pleasure to check the Umbraco code again. By the way, judging by the code comments, the developers started using ReSharper. However, PVS-Studio still found interesting errors. Conclusion — you can profit more by using several tools simultaneously. ;)

If you want to check your project, you can request a trial key on our website.

And do not forget that one-time checks are better than none. But the maximum benefit from static analysis is achieved with its regular use and implementation in processes.

PVS-Studio checks Umbraco code for the third time