Триквел The Orchard Core. Перепроверка проекта с помощью PVS-Studio

В этой статье мы проверим проект Orchard Core с помощью статического анализатора PVS-Studio. Нам предстоит выяснить, так ли хорош код платформы, как сайты, созданные на его основе. Да пребудет с нами сила статического анализа!

Введение

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

Код проекта доступен в репозитории на GitHub. Мы проверили код статическим анализатором кода PVS-Studio.

Анализатор выдал 281 предупреждение для 3791 файла с расширением .cs. 54 предупреждения имели высокий уровень уверенности, 143 — средний уровень и 84 — низкий уровень. Теперь рассмотрим наиболее интересные из них.

Результаты анализа

Проблема 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 Объект ‘info’ был использован до того, как он был проверен на соответствие нулю. Проверьте строки: 637, 641. AccountController.cs 637

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

Проблема 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. Рассмотрите возможность проверки ‘metadata.CreateRouteValues’. ContentAnchorTag.cs 188

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

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

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

Совет: Во время просмотра кода дважды проверьте последний блок вложенных условий. Этот блок может скрывать коварный эффект последней строки!

Проблема 3

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

  if (paths == null)
  {
    return NotFound();
  }
  ....
}
Вход в полноэкранный режим Выход из полноэкранного режима

Предупреждение PVS-Studio: V3095 Объект ‘paths’ был использован до того, как он был проверен на null. Проверьте строки: 304, 312. AdminController.cs 304

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

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

Проблема 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 Переменная ‘lastProviders’ была использована после того, как ей был присвоен оператор null-conditional. Возможно возникновение NullReferenceException. ShellSettingsManager.cs 242

Хотя приведенный выше фрагмент кода содержит только присвоение объекта lastProviders и условие, ошибка незаметна. Анализатор сообщает нам, что ссылка на объект, присвоенный через оператор null-conditional, разыменовывается. Действительно, lastProviders получен из результата приведения _applicationConfiguration к IConfigurationRoot, выполненного через as. В этом случае*,* lastProviders может принимать значение null, если приведение невозможно. Разработчики специально выполняют функцию через оператор ‘.?’. Но они не добавили никаких проверок на null в условие, содержащее вызов lastProviders.Count.

Данный фрагмент кода демонстрирует распространенный паттерн ошибок, найденных PVS-Studio. Многие разработчики предпочитают использовать операторы null-conditional вместо явных проверок на null. Такой подход делает код менее громоздким и более читабельным. Но операторы null-conditional могут затеряться в большой базе кода. В этом случае может возникнуть зловещее исключение NullReferenceException.

Совет: Обращайте внимание на использование null-условных операторов. Старайтесь не упускать из виду null

Выпуск 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 Переменная ‘externalClaims’ была использована после того, как ей был присвоен оператор null-conditional. Возможно возникновение NullReferenceException. AccountController.cs 786

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

Проблема 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 Объект ‘newVersion’ был использован до того, как был проверен на соответствие null. Проверьте строки: 94, 95. BlogPostCreateDeploymentPlanTests.cs 94

Этот кусок кода показывает то, чего так боятся все разработчики — ошибки copy-paste. Здесь разработчик забыл использовать оператор null-conditional при обращении программы к объекту newVersion. Поэтому, когда программа обращается к свойству DisplayText, может быть выброшен NullReferenceException.

Это произошло, скорее всего, когда разработчик скопировал похожие блоки кода, содержащие оператор ‘?’. Однако когда появилась новая строка с объектом newVersion, оператор нулевой ссылки волшебным образом исчез.

Совет: Когда вы копируете код, стоит уделить больше времени просмотру кода. Вы также можете использовать статический анализатор, чтобы облегчить процесс проверки кода.

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

Как я уже говорил, мы дважды проверяли проект 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 Объект ‘remoteClient’ был использован до того, как был проверен на null. Проверьте строки: 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. Рассмотрите возможность проверки ‘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 Найдено два одинаковых фрагмента кода. Возможно, это опечатка и вместо переменной ‘ConsumerSecret’ TwitterClientMessageHandler.cs 51 следует использовать переменную ‘AccessTokenSecret’.

Казалось бы, еще одна ошибка копирования-вставки, но какая досадная! Вместо проверки consumerSecret во втором условии лучше проверить AccessTokenSecret, потому что 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 — любовь с первого взгляда

Стоит обратить внимание на еще одно предупреждение анализатора. Анализатор отметил разыменование значения, возвращаемого методом FirstOrDefault, без проверки на null в 39 фрагменте кода. Взгляните на следующий фрагмент кода:

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. Функция ‘_factories.FirstOrDefault’ может возвращать нулевое значение по умолчанию. AddToDeploymentPlanController.cs 77

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

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

Заключение

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

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

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

Оцените статью
devanswers.ru
Добавить комментарий