# PVS\-Studio checks Umbraco code for the third time

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\.

![0898_Umbraco/image1.png](https://import.viva64.com/docx/blog/0898_Umbraco/image1.png)

## Introduction

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

* [The First C\# Project Analyzed](https://pvs-studio.com/en/blog/posts/csharp/0357/),
* [Re\-analysis of Umbraco code](https://pvs-studio.com/en/blog/posts/csharp/0461/)\.

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](https://github.com/umbraco/Umbraco-CMS)\.

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

[PVS\-Studio](https://pvs-studio.com/en/) 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](https://github.com/umbraco/Umbraco-CMS)\. 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?

```cpp
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\.

```cpp
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](https://pvs-studio.com/en/docs/warnings/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\.

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

![0898_Umbraco/image2.png](https://import.viva64.com/docx/blog/0898_Umbraco/image2.png)

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

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/examples/v3021/)\.

**Issue 3**

```cpp
public bool FlagOutOfDateModels
{
  get => _flagOutOfDateModels;

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

    _flagOutOfDateModels = value;
  }
}
```

PVS\-Studio issued warning [V3008](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/v3105/) The 'routeEndpoints' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. RoutableDocumentFilter\.cs 198

Diagnostics [V3105](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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:

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

Looks like the arguments are passed in the wrong order\. The _RelateDocumentOnCopy**Alias**_ argument is passed to the _name _parameter of the constructor\. The _RelateDocumentOnCopy**Name**_ is passed to the _alias_ parameter\.

**Issue 6**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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\.

![0898_Umbraco/image3.png](https://import.viva64.com/docx/blog/0898_Umbraco/image3.png)

**Issue 7**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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**

```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/blog/posts/cpp/a0080/)\.



```cpp
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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/pvs-studio/try-free/)\.

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\.