Для получения триального ключа
заполните форму ниже
Team License (базовая версия)
Enterprise License (расширенная версия)
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Запросите информацию о ценах
Новая лицензия
Продление лицензии
--Выберите валюту--
USD
EUR
RUB
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Бесплатная лицензия PVS-Studio для специалистов Microsoft MVP
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Для получения лицензии для вашего открытого
проекта заполните, пожалуйста, эту форму
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Мне интересно попробовать плагин на:
* Нажимая на кнопку, вы даете согласие на обработку
своих персональных данных. См. Политику конфиденциальности

Ваше сообщение отправлено.

Мы ответим вам на


Если вы так и не получили ответ, пожалуйста, проверьте папку
Spam/Junk и нажмите на письме кнопку "Не спам".
Так Вы не пропустите ответы от нашей команды.

>
>
>
Третья проверка Umbraco статическим ана…

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

14 Дек 2021

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

0898_Umbraco_ru/image1-6abc7f58fa2793dd087d80049925d89b.png

Введение

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

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

Если вас заинтересовала эта статья, возможно, вы уже знаете, что такое Umbraco, но на всякий случай повторим. Umbraco – система управления контентом с открытым исходным кодом, которая улучшает опыт редактирования содержимого сайта. Сам исходный код можно найти на GitHub.

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

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

Для анализа была взята версия проекта от 12.11.2021 и скачана архивом с GitHub. Используемая версия PVS-Studio – 7.15.54288.

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

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

Issue 1

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

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

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

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

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

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-819f964c53fbae3605d10a5ed9de11f2.png

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

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 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 могут скрываться неприметные ошибки.

Issue 3

public bool FlagOutOfDateModels
{
  get => _flagOutOfDateModels;

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

    _flagOutOfDateModels = value;
  }
}

Предупреждение PVS-Studio: 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

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 The 'routeEndpoints' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RoutableDocumentFilter.cs 198

Диагностика V3105 предупреждает о возможности возникновения NullReferenceException. _endpointDataSource проверяется на null за счёт использования оператора '?.'. Если переменная _endpointDataSource всё-таки содержит значение null, то и в routeEndpoints будет null.

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

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: V3066 Possible incorrect order of arguments passed to 'RelationType' constructor. RelateOnCopyNotificationHandler.cs 32

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

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

Похоже, что аргументы переданы не в том порядке. Аргумент RelateDocumentOnCopyAlias передаётся в параметр конструктора name, а аргумент RelateDocumentOnCopyName передаётся в параметр alias.

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: V3004 The 'then' statement is equivalent to the 'else' statement. UrlProviderExtensions.cs 274

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

0898_Umbraco_ru/image3-3484acf9016d2cb3749bad437eb5b6cb.png

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: V3137 The 'username' variable is assigned but is not used by the end of the function. MemberManager.cs 87

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

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

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: 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

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 Expression 'pageNumber > 0' is always true. EntityController.cs 625

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

Здесь же, кстати, было выдано предупреждение про недостижимый код: V3142 Unreachable code detected. It is possible that an error is present. EntityController.cs 630

Issue 10

А здесь у нас ошибка в тесте. Можно подумать, что она не так уж и важна, однако тесты – это то, что даёт некоторую уверенность в корректности работы вашего кода. Если же в них ошибка – можно ли быть уверенным в том, что программа работает правильно? И вот в такие моменты приходит на помощь статический анализ.

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 Consider reviewing the correctness of 'contentType1' item's usage. ConvertersTests.cs 115

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

Заключение

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

Если же у вас появилось желание проверить свой проект, можно взять пробный ключик у нас на сайте.

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

Популярные статьи по теме
Проверка проекта Orleans статическим анализатором PVS-Studio

Дата: 04 Июл 2022

Автор: Никита Паневин

Orleans – кроссплатформенный фреймворк для создания масштабируемых облачных приложений. Это ПО разработано компанией Microsoft, проекты которой не раз проверялись анализатором PVS-Studio. Посмотрим, …
Насколько хорошо защищены ваши пароли? Проверка проекта Bitwarden

Дата: 13 Май 2022

Автор: Никита Паневин

Bitwarden – менеджер паролей с открытым исходным кодом. Это программное обеспечение помогает генерировать уникальные пароли и управлять ими. Получится ли у анализатора PVS-Studio отыскать ошибки в та…
Зачем разработчикам игр на Unity использовать статический анализ?

Дата: 11 Май 2022

Автор: Артём Ровенский

С годами стоимость создания игр стала больше, вырос их масштаб, а следовательно, и их кодовая база. Разработчикам становится всё сложнее уследить за ошибками. А забагованная игра влечёт финансовые и …
Эволюция PVS-Studio: анализ потока данных для связанных переменных

Дата: 28 Апр 2022

Автор: Никита Липилин

Связанные переменные – одна из главных проблем статического анализа. Данная статья посвящена разбору этой темы и рассказу о том, как разработчики PVS-Studio сражаются с ложными срабатываниями, появив…
Зачем нужен статический анализ? Разбираем на примере ошибки из Akka.NET

Дата: 25 Апр 2022

Автор: Сергей Васильев

"Статический анализ нужно использовать регулярно, а не только перед релизами... Чем раньше найдена ошибка, тем дешевле её исправление..." – вы уже слышали это 100 раз. Сегодня ещё раз ответим на вопр…

Комментарии (0)

Следующие комментарии
Этот сайт использует куки и другие технологии, чтобы предоставить вам более персонализированный опыт. Продолжая просмотр страниц нашего веб-сайта, вы принимаете условия использования этих файлов. Если вы не хотите, чтобы ваши данные обрабатывались, пожалуйста, покиньте данный сайт. Подробнее →
Принять