Алексей Авдеев

25 Авг 2022

Теги:

#CSharp

Триквел по Orchard Core. Повторная проверка при помощи PVS-Studio

25 Авг 2022

Автор: Алексей Авдеев

Введение
Результаты проверки
Достойные упоминания
FirstOrDefault – любовь с первого взгляда
Заключение

В этой статье мы исследуем проект Orchard Core c помощью статического анализатора PVS-Studio и узнаём, так ли привлекателен код платформы, как сайты, созданные на её основе. Итак, пусть поток статического анализа несёт нас вперёд!

Введение

Orchard Core – это модульная многопользовательская система приложений с открытым исходным кодом и CMS для ASP.NET Core. Мы уже дважды проверяли этот проект (здесь и здесь) и оба раза находили интересные предупреждения анализатора, о которых не стыдно рассказать публике. Будем надеяться, что и в этот раз разработчики не постеснялись оставить нам интересных ошибок. =)

Код проекта доступен в репозитории на GitHub. Для проверки использован статический анализатор кода PVS-Studio.

В результате анализа было получено 281 срабатывание на 3791 файле .cs, из которых 54 срабатывания имели уровень High, 143 – Medium и 84 – Low. Далее рассмотрим наиболее интересные из них.

Результаты проверки

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: V3095 The 'info' object was used before it was verified against null. Check lines: 637, 641. AccountController.cs 637

Начнём обзор предупреждений анализатора с любимого многими разыменования потенциальной null ссылки. Двойное обращение к свойству Principal объекта info и проверка на null буквально в следующей же строке. Элегантно, не так ли? На самом деле причиной возникновения подобных ошибок часто является обычная невнимательность. Скорее всего, проверка на null должна быть произведена до разыменования info. В таком случае никаких проблем бы не возникло.

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: V3080 Possible null dereference. Consider inspecting 'metadata.CreateRouteValues'. ContentAnchorTag.cs 188

Продолжая рассматривать тему null dereference, нельзя не упомянуть об опечатках в повторяющихся вложенных условных конструкциях. Здесь свойство CreateRouteValues объекта metadata разыменовывается прямо в then блоке, явно указывающем на равенство null.

Чтобы убедиться, что это всего лишь досадная опечатка – достаточно взглянуть на схожую конструкцию else if, приведённую выше. Там использован подходящий оператор сравнения и поэтому разыменование свойств объекта metadata не ведёт к ошибкам.

Кстати, эта ошибка завоевала золото в нашем небольшом топе ошибок на ASP.NET Core.

Tip: На этапе вычитки кода дважды проверяйте последний блок вложенных конструкций. Там часто прячется коварный эффект последней строки!

Issue 3

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

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

Предупреждение PVS-Studio: V3095 The 'paths' object was used before it was verified against null. Check lines: 304, 312. AdminController.cs 304

С этой ошибкой дела обстоят гораздо интереснее. На первый взгляд может показаться, что код корректен. Хоть paths и используется до проверки на null, ни одного явного разыменования ссылки на этот объект нет. Как бы не так! При итерации по коллекции цикл foreach вызовет у неё метод GetEnumerator, что и станет причиной для падения программы с NullReferenceException.

Tip: Всегда учитывайте специфику функционирования различных конструкций языка или держите под рукой надёжное программное решение для проверки кода.

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: V3105 The 'lastProviders' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. ShellSettingsManager.cs 242

Несмотря на то, что приведённый фрагмент кода содержит лишь инициализацию объекта lastProviders и условную конструкцию, ошибка не бросается в глаза. Анализатор сообщает нам о разыменовании ссылки на объект, инициализированный с использованием null-условного оператора. Действительно, значение lastProviders получено из результата каста _applicationConfiguration к IConfigurationRoot с использованием оператора as. В таком случае lastProviders может принять значение null, если вышеуказанный каст невозможен. Такой вариант явно предусмотрен разработчиками (использован оператор '.?'), однако далее условная конструкция содержит вызов lastProviders.Count() без каких-либо проверок на null.

Этот пример кода реализует довольно частый паттерн ошибок, обрабатываемых PVS-Studio. Разработчики повсеместно предпочитают использование null-условных операторов, вместо явных проверок на null. Такой подход делает код менее громоздким и более читаемым, однако часто null-условный оператор теряется в большом массиве кода. Это приводит к возникновению всё тех же зловещих NullReferenceException.

Tip: Обращайте внимание на использование null-условных операторов. Не дайте 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: V3105 The 'externalClaims' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. AccountController.cs 786

Анализатор предупреждает о потенциально опасном использовании переменной externalClaims, значение которой получено через null-условный оператор. Здесь, как и в предыдущем случае, не предусмотрена защита от разыменования нулевой ссылки.

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: V3095 The 'newVersion' object was used before it was verified against null. Check lines: 94, 95. BlogPostCreateDeploymentPlanTests.cs 94

На данном участке кода нам придётся рассмотреть то, чего так боятся все разработчики. А именно – ошибки копипасты. Здесь в одном из обращений к объекту newVersion не использован null-условный оператор, что может привести к NullReferenceException при обращении к свойству DisplayText.

Произошло это, скорее всего, из-за переиспользования написанных ранее схожих блоков кода, в которых оператор '?.' присутствует, однако стоило появиться новой строчке с использованием объекта newVersion как null-условный оператор магическим образом исчез.

Tip: При использовании копирования кода стоит потратить время на вычитку или же доверить это статическому анализатору.

Достойные упоминания

Как отмечалось ранее, проект Orchard дважды проверялся нами (здесь и здесь). Заметим, что все проблемные моменты, найденные при первой проверке, были исправлены. На этот раз ситуация не столь однозначная. Команда PVS-Studio считает своим долгом указать на эти потенциальные ошибки вновь.

Начнём с довольно интересного примера:

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: V3095 The 'remoteClient' object was used before it was verified against null. Check lines: 46, 48. ImportRemoteInstanceController.cs 46

Анализатор сообщает нам, что remoteClient разыменован до его явной проверки на null. Скорее всего, проверка должна располагаться выше, предотвращая возникновение NullReferenceException.

Также в прошлой проверке мы предполагали, что, возможно, проверка на null не нужна, а вместо метода FirstOrDefault стоит использовать просто First. Такое решение выглядит разумно на фоне того, что спустя три года анализатор снова выдал предупреждение на этом участке кода.

В коде проекта метод FirstOrDefault множество раз использован без каких-либо проверок результата на null (об этом далее). Здесь же явная проверка на null присутствует, поэтому остановимся на том, что необходимо поменять местами условную конструкцию и инициализацию apiKey.

Теперь посмотрим не столько на срабатывание об ошибке, сколько на рекомендацию:

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: V3080 Possible null dereference. Consider inspecting 'request'. GraphQLMiddleware.cs 157

Здесь объект request инициализируется внутри каждой из вложенных условных конструкций. Приводить полный код бессмысленно из соображений читаемости. Поэтому обратимся к первым двум условиям, проверяющим тип запроса на соответствие IsPost и IsGet. Как уже упоминалось в предыдущей статье, класс Microsoft.AspNetCore.HttpMethods имеет девять статических методов для проверки типа запроса. Таким образом, при получении необрабатываемого запроса будет выброшено исключение типа NullReferenceException.

Конечно, это не ошибка, а решение покрывать только используемый функционал. Однако мы всё равно обращаем внимание разработчиков на подобные места. В будущем это может избавить их от выматывающего поиска места падения программы.

Тем более, что проверка на null и выброс исключения занимают всего несколько строк =).

Итак, последняя ошибка в этом разделе, но не последняя по занимательности:

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: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'AccessTokenSecret' variable should be used instead of 'ConsumerSecret' TwitterClientMessageHandler.cs 51

Казалось бы, просто очередная ошибка копипасты, но какая живучая! Вместо ConsumerSecret во втором условии нужно проверить AccessTokenSecret, так как проверки с этим свойством нигде нет. Однако then блок явно намекает, что ей место здесь. Таким образом, исправленная версия может выглядеть так:

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

Завершая раздел, хотелось бы отметить, что указанные выше места в коде не были исправлены по прошествии длительного времени. А значит присутствует вероятность того, что это вовсе не ошибки, а лишь неудачно оформленные участки вполне работоспособного и безопасного кода.

Независимо от того, содержат ли описанные выше участки кода ошибки или нет, на них стоит обратить внимание разработчика. И если на этом моменте вы с ужасом представили как анализатор будет засыпать вас ложными предупреждениями об участке неординарного кода, то мы спешим вас успокоить. В PVS-Studio реализован надёжный механизм подавления ложных срабатываний, который не даст вас в обиду =).

FirstOrDefault – любовь с первого взгляда

Раз уж мы взялись давать разработчикам Orchard рекомендации, то нельзя не упомянуть о том, что анализатор выдал 39 предупреждений о небезопасном разыменовании результата выполнения метода FirstOrDefault – без каких-либо проверок на null. Например, такой фрагмент кода:

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

Предупреждение PVS-Studio: V3146 Possible null dereference. The '_factories.FirstOrDefault' can return default null value. AddToDeploymentPlanController.cs 77

Анализатор предупреждает нас о том, что метод FirstOrDefault потенциально может вернуть значение null, которое может привести к падению с NullReferenceException. Скорее всего, разработчик не ожидает появления null в процессе выполнения, поэтому никаких проверок не требуется. Но мы в свою очередь резонно возразим: почему не First? Или же default значение всё-таки может появиться? Тогда почему нет проверки на null? И таких мест анализатор обнаружил 39!

Tip: Используйте First вместо FirstOrDefault там, где последовательность точно содержит хотя бы один элемент. Такой подход улучшит читаемость кода. Пусть ваш код будет таким же красивым, как и сайты, сделанные с использованием Orchard! =)

Заключение

Как и в прошлой статье, хотелось бы отметить высокое качество кодовой базы проекта Orchard! Да, в этот раз проект не может похвастаться исправлением всех предупреждений, найденных при предыдущей проверке, но это ни в коем случае не умаляет работы, проделанной разработчиками за эти три года.

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

Рекомендуем вам попробовать проверить свои проекты с помощью нашего анализатора. Вы обнаружите много интересных предупреждений, уж поверьте!

#CSharp