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

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

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

## Введение

Orchard Core – это модульная многопользовательская система приложений с открытым исходным кодом и CMS для ASP\.NET Core\. Мы уже дважды проверяли этот проект \([здесь](https://pvs-studio.ru/ru/blog/posts/csharp/0456/) и [здесь](https://pvs-studio.ru/ru/blog/posts/csharp/0681/)\) и оба раза находили интересные предупреждения анализатора, о которых не стыдно рассказать публике\. Будем надеяться, что и в этот раз разработчики не постеснялись оставить нам интересных ошибок\. \=\)

Код проекта доступен в [репозитории на GitHub](https://github.com/OrchardCMS/OrchardCore/tree/v1.4.0)\. Для проверки использован статический анализатор кода PVS\-Studio\.

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

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

**Issue 1**

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'metadata\.CreateRouteValues'\. ContentAnchorTag\.cs 188

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

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

Кстати, эта ошибка завоевала золото в нашем небольшом [топе ошибок на ASP\.NET Core](https://habr.com/en/company/pvs-studio/blog/654395/)\.

**Tip: На этапе вычитки кода дважды проверяйте последний блок вложенных конструкций\. Там часто прячется коварный [эффект последней строки](https://pvs-studio.ru/ru/blog/posts/cpp/0260/)\!**

**Issue 3**

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

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

Предупреждение PVS\-Studio: [V3095](https://pvs-studio.ru/ru/docs/warnings/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**

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

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

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

**Issue 6**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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 дважды проверялся нами \([здесь](https://pvs-studio.ru/ru/blog/posts/csharp/0456/) и [здесь](https://pvs-studio.ru/ru/blog/posts/csharp/0681/)\)\. Заметим, что все проблемные моменты, найденные при первой проверке, были исправлены\. На этот раз ситуация не столь однозначная\. Команда PVS\-Studio считает своим долгом указать на эти потенциальные ошибки вновь\. 

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

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'request'\. GraphQLMiddleware\.cs 157

Здесь объект _request_ инициализируется внутри каждой из вложенных условных конструкций\. Приводить [полный код](https://github.com/OrchardCMS/OrchardCore/blob/v1.4.0/src/OrchardCore.Modules/OrchardCore.Apis.GraphQL/GraphQLMiddleware.cs) бессмысленно из соображений читаемости\. Поэтому обратимся к первым двум условиям, проверяющим тип запроса на соответствие _IsPost_ и _IsGet_\. Как уже упоминалось в [предыдущей статье](https://pvs-studio.ru/ru/blog/posts/csharp/0681/), класс _Microsoft\.AspNetCore\.HttpMethods_ имеет девять статических методов для проверки типа запроса\. Таким образом, при получении необрабатываемого запроса будет выброшено исключение типа _NullReferenceException_\.

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

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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 блок явно намекает, что ей место здесь\. Таким образом, исправленная версия может выглядеть так:

```cpp
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 реализован надёжный механизм [подавления ложных срабатываний](https://pvs-studio.ru/ru/docs/manual/0032/), который не даст вас в обиду \=\)\.

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

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

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\! \=\)**

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

Как и в [прошлой статье](https://pvs-studio.ru/ru/blog/posts/csharp/0681/), хотелось бы отметить высокое качество кодовой базы проекта Orchard\! Да, в этот раз проект не может похвастаться исправлением всех предупреждений, найденных при предыдущей проверке, но это ни в коем случае не умаляет работы, проделанной разработчиками за эти три года\.

Само собой, мы будем продолжать проверять открытые проекты, дабы наблюдать процесс поиска и исправления ошибок на длительном промежутке времени\. Увы, но проверок раз в три года недостаточно\. Для получения максимального эффекта от использования статического анализатора кода его стоит [использовать регулярно](https://pvs-studio.ru/ru/blog/posts/a0067/)\. 

Рекомендуем вам [попробовать](https://pvs-studio.ru/ru/pvs-studio/try-free/) проверить свои проекты с помощью нашего анализатора\. Вы обнаружите много интересных предупреждений, уж поверьте\!