# Третья проверка Umbraco статическим анализатором PVS\-Studio

6 лет назад была написана первая статья про проверку проекта с помощью анализатора PVS\-Studio для C\#\. Мы решили оглянуться назад и вновь вернуться туда, откуда всё начиналось – к анализу исходного кода Umbraco CMS\.

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

## Введение

Как уже понятно из названия, ранее мы уже посвятили проекту Umbraco две статьи:

* [Первая статья про проверку C\# проекта](https://pvs-studio.ru/ru/blog/posts/csharp/0357/),
* [Снова проверяем исходный код Umbraco](https://pvs-studio.ru/ru/blog/posts/csharp/0461/)\.

Предлагаю вам ознакомиться с ними, чтобы понаблюдать, как меняются типы ошибок со временем\.

Если вас заинтересовала эта статья, возможно, вы уже знаете, что такое Umbraco, но на всякий случай повторим\. Umbraco – система управления контентом с открытым исходным кодом, которая улучшает опыт редактирования содержимого сайта\. Сам исходный код можно найти на [GitHub](https://github.com/umbraco/Umbraco-CMS)\.

Напомним и про PVS\-Studio\. ;\)

[PVS\-Studio](https://pvs-studio.ru/ru/) – статический анализатор для улучшения качества, защищённости \(SAST\) и безопасности кода\. Работает с языками C, C\+\+, C\#, Java под управлением Windows, Linux, macOS\.

Для анализа была взята версия проекта от 12\.11\.2021 и скачана архивом с [GitHub](https://github.com/umbraco/Umbraco-CMS)\.  Используемая версия PVS\-Studio – 7\.15\.54288\.

Среди срабатываний мы, как обычно, отобрали самые интересные\. Какие\-то из них указывают на явные ошибки, а какие\-то – на код, который выглядит как минимум подозрительно\. Но давайте перейдём ближе к делу и посмотрим, что же удалось обнаружить\.

## Что там с предупреждениями?

**Issue 1**

А вы сможете найти ошибку в данном примере?

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

Ладно\-ладно, давайте взглянем на укороченный вариант кода\.

```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: [V3106](https://pvs-studio.ru/ru/docs/warnings/v3106/) Possibly index is out of bound\. The '2' index is pointing beyond 'm\.Arguments' bound\. ExpressionVisitorBase\.cs 632

Думаю, что все когда\-либо в жизни сталкивались с такими промахами\. Разработчики проверяют, что _m\.Arguments\.Count_ равно 2, а затем почти сразу же пытаются получить доступ к 3\-ему элементу\. Очевидно, что это приведёт к исключению _IndexOutOfRangeException_\.

Подобные проблемы обнаруживались и в других проектах, и, как видите, Umbraco не стал исключением\.

**Issue 2**

Предлагаю проверить свою внимательность и найти проблему самостоятельно\. Сразу за кодом будет картинка, а уже за ней – правильный ответ\.

```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_ru/image2.png](https://import.viva64.com/docx/blog/0898_Umbraco_ru/image2.png)

Если вы быстро справились с этой задачкой, то у вас орлиное зрение\! Взглянем на сокращённый вариант метода:

```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: [V3021](https://pvs-studio.ru/ru/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

Не очень привлекательный фрагмент кода для тщательного вычитывания на code review, не правда ли?

Похоже, что в данном случае повезло и здесь просто избыточное условие\. К этому выводу можно прийти при анализе используемых и доступных перегрузок метода _XmlConvert\.ToString_\. Однако везёт не всем и не всегда – порой за copy\-paste могут [скрываться неприметные ошибки](https://pvs-studio.ru/ru/blog/examples/v3021/)\.

**Issue 3**

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

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

    _flagOutOfDateModels = value;
  }
}
```

Предупреждение PVS\-Studio: [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The '\_flagOutOfDateModels' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 54, 51\. ModelsBuilderSettings\.cs 54

Как мы видим, в set accessor присутствует проверка c присваиванием значения _\_flagOutOfDateModels_, но сразу после неё для этого же поля устанавливается новое значение\. Следовательно, блок _if_ не несёт никакой практической пользы\.

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

Диагностика [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/) предупреждает о возможности возникновения _NullReferenceException_\. _\_endpointDataSource_ проверяется на _null_ за счёт использования оператора '?\.'\. Если переменная _\_endpointDataSource_ всё\-таки содержит значение _null_, то и в _routeEndpoints_ будет _null\._

Странным здесь выглядит то, что обращение к _routeEndpoints_ происходит уже без оператора '?\.'\. Как следствие, если _routeEndpoints_ – _null_, при обращении по этой ссылке будет выброшено исключение типа _NullReferenceException_\.

**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: [V3066](https://pvs-studio.ru/ru/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'RelationType' constructor\. RelateOnCopyNotificationHandler\.cs 32

В данном случае происходит вызов конструктора и передача ему аргументов\. Взглянем на его сигнатуру:

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

Похоже, что аргументы переданы не в том порядке\. Аргумент _RelateDocumentOnCopy**Alias**_ передаётся в параметр конструктора _name_, а аргумент _RelateDocumentOnCopy**Name**_ передаётся в параметр _alias_\.

**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: [V3004](https://pvs-studio.ru/ru/docs/warnings/v3004/) The 'then' statement is equivalent to the 'else' statement\. UrlProviderExtensions\.cs 274

Анализатор нашёл конструкцию, в которой ветви _then_ и _else_ совпадают\. Получается так, что вне зависимости от значения проверяемого свойства выполняется один и тот же код\. Скорее всего, разработчик копировал код и забыл исправить параметры метода\.

![0898_Umbraco_ru/image3.png](https://import.viva64.com/docx/blog/0898_Umbraco_ru/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: [V3137](https://pvs-studio.ru/ru/docs/warnings/v3137/) The 'username' variable is assigned but is not used by the end of the function\. MemberManager\.cs 87

Было замечено вот такое срабатывание\. Разработчик объявляет переменную _username_ и присваивает ей значение\. После этого _username_ больше нигде не используется\. 

Возможно, разработчики просто не убрали это после рефакторинга кода\. Однако есть некоторая \(пусть и небольшая\) вероятность, что какая\-то логика не была реализована или где\-то здесь запряталась хитрая ошибка\.

**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: [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/) The 'user' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 446, 444\. UsersController\.cs 446

В блоке _else_ условного выражения происходит присваивание значения _user\._ Сразу после завершения условного выражения опять идёт присваивание _user_\. Следовательно, присвоенное ранее значение не используется и сразу же затирается\. Должно ли было использоваться значение _userResult\.Value,_ и здесь пропущена какая\-то логика, или это просто избыточный код – вопрос открытый\. Но в любом случае какие\-то подозрения на счёт этого кода сразу закрадываются\.

**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: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'pageNumber \> 0' is always true\. EntityController\.cs 625

Разработчик делает проверку, что _pageNumber_ меньше или равен 0, если это так, то мы выходим\. Следуя по коду далее, мы натыкаемся на проверку _pageNumber_ больше 0_\._ Естественно, это условие будет всегда истинным, а следовательно, будет безусловно вызван _return_\. Код, который написан после этого оператора _if_ \(а его достаточно много\)_,_ никогда не будет выполняться\. 

Здесь же, кстати, было выдано предупреждение про недостижимый код: [V3142](https://pvs-studio.ru/ru/docs/warnings/v3142/) Unreachable code detected\. It is possible that an error is present\. EntityController\.cs 630

**Issue 10**

А здесь у нас ошибка в тесте\. Можно подумать, что она не так уж и важна, однако тесты – это то, что даёт некоторую уверенность в корректности работы вашего кода\. Если же в них ошибка – можно ли быть уверенным в том, что программа работает правильно? И вот в такие моменты [приходит на помощь](https://pvs-studio.ru/ru/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: [V3056](https://pvs-studio.ru/ru/docs/warnings/v3056/) Consider reviewing the correctness of 'contentType1' item's usage\. ConvertersTests\.cs 115

Скорее всего, это copy\-paste ошибка: вместо _contentType2_ при объявлении переменной _cnt2_ используется _contentType1\._ Довольно странно, не так ли? 



## Заключение

Было приятно принять эстафету проверки кода Umbraco\. Кстати, судя по комментариям, встречающимся в коде, разработчики проекта взяли на своё вооружение ReSharper\. Что, впрочем, не помешало PVS\-Studio найти интересные ошибки\. Вывод – совместное использование нескольких инструментов может дать больше профита\. ;\)

Если же у вас появилось желание проверить свой проект, можно взять пробный ключик у нас на [сайте](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.

И не забывайте, что разовые проверки – лучше, чем их отсутствие, но максимальная польза от статического анализа достигается при регулярном использовании и внедрении в процессы\.