rip_m 4 мар 2022 в 17:35

Топ-10 ошибок, найденных PVS-Studio в проектах на ASP.NET Core

13 мин

5.6K

Миллионы людей используют веб-приложения, построенные на основе ASP.NET Core. Поэтому мы решили улучшить работу PVS-Studio при анализе таких проектов. Для демонстрации результата нашей работы мы проверим несколько open source проектов и представим топ срабатываний.

Введение

Мы часто рассказываем про используемые нами технологии статического анализа. Одной из них является аннотирование. Что же это такое и зачем нужно?

Часто бывает, что при разборе кода невозможно раскрыть тело метода. Например, если метод объявлен в библиотеке, исходный код которой недоступен. Однако даже если код открыт, некоторые высокоуровневые выводы о том, как работает функция, анализатор просто не может сделать, и лучше вручную подсказать ему. В таких случаях аннотации – простой и удобный способ получения анализатором информации об особенностях работы этого метода. И мы, как разработчики PVS-Studio, можем заложить в него нужную информацию. Это могут быть данные о возвращаемых методом значениях или о том, какие значения стоит или не стоит передавать в качестве аргументов.

Раньше уже выходила заметка по аннотированию методов из Unity. В ней мы рассказали про трудности, с которыми пришлось столкнуться, и о том, как, передав null в аргументе метода, получилось привести к падению самого редактора :). Данная статья будет немного отличаться.

Сегодня мы не только расскажем о том, как улучшили анализ проектов, использующих ASP.NET Core, но и посмотрим на интересные проблемы в коде, которые удалось обнаружить в ходе работы. Некоторые из них анализатор начал находить благодаря аннотациям. Другие он уже умел искать. Их мы выписали, так как эти предупреждения тоже показались заслуживающими внимания. Кстати, проекты, на которых мы проверяли работу анализатора, были взяты отсюда. Основные критерии отбора: над проектом ведётся работа на момент проверки и при этом отсутствуют ошибки компиляции.

Аннотирование методов ASP.NET Core

Как и в случае с Unity, пока что мы решили проаннотировать наиболее часто используемые классы. Для нахождения подходящих классов мы воспользовались полезной утилитой, написанной нами на основе Roslyn. Про неё более подробно можно прочитать в ранее упомянутой заметке об аннотировании методов из Unity. Воспользовавшись нашей утилитой, мы смогли найти классы, которые использовались в 17 отобранных ASP.NET Core проектах:

Microsoft.AspNetCore.Mvc.ControllerBase
Microsoft.AspNetCore.Mvc.Controller
Microsoft.AspNetCore.Identity.UserManager
Microsoft.AspNetCore.Builder.ControllerEndpointRouteBuilderExtensions
Microsoft.AspNetCore.Builder.EndpointRoutingApplicationBuilderExtensions
Microsoft.AspNetCore.Mvc.ModelBinding.ModelStateDictionary
Microsoft.AspNetCore.Identity.SignInManager
И т.д.

Именно эти классы являлись приоритетными при аннотировании.

Для примера возьмём метод PhysicalFile(String, String) класса ControllerBase. Если мы посмотрим на документацию, то станет понятно, что данный метод принимает абсолютный путь к файлу и тип содержимого файла. Также не стоит забывать, что данный метод имеет возвращаемое значение. Это уже полезно для аннотирования, но можно узнать ещё больше.

Для получения дополнительной информации можно использовать 2 способа:

найти исходники на GitHub и попросту поглядеть, как работает функция;
протестировать работу функции вручную, передавая разные комбинации значений в качестве аргументов.

В итоге мы получили следующую информацию:

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

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

Проверка проектов

Места в нашем топе расставлены субъективно — вы всегда можете предложить своё видение ошибки. Может, некоторые предупреждения вы бы перенесли на другое место топа. Расскажите нам об этом в комментариях.

Ну хватит предисловий! Перейдём к самому интересному — проверке проектов.

10-е место

Открывают данный топ предупреждения, которые были сгенерированы анализатором на проекте Cloudscribe. Да-да, здесь не одно срабатывание, а сразу два. Поэтому технически сегодня мы разбираем одиннадцать предупреждений, а не десять. :)

protected override ValidationResult IsValid(....)
{
  if (field != null)
  {
    ....

    // compare the value against the target value
    if ((dependentValue == null && TargetValue == null) ||
        (dependentValue != null && 
         (TargetValue.Equals("*") ||
          dependentValue.Equals(TargetValue))))
    {
      ....
    }
  }

  return ValidationResult.Success;
}

Предупреждение анализатора: V3080 Possible null dereference. Consider inspecting 'TargetValue'. RequiredWhenAttribute.cs 78

Анализатор обратил внимание на тот факт, что существует возможность разыменования нулевой ссылки. Если переменная dependentValue не равна null, а TargetValue — равна, то будет выброшено всеми любимое исключение NullReferenceException.

Вот ещё один пример доступа по нулевой ссылке:

public async Task<IActionResult> Index(ConsentInputModel model)
{
  // user clicked 'no' - send back the standard
  // 'access_denied' response
  if (model.Button == "no")
  {
    response = ConsentResponse.Denied;
  }
  // user clicked 'yes' - validate the data
  else if (model.Button == "yes" && model != null)
  {
    ....
  }
  ....
}

Предупреждение анализатора: V3027 The variable 'model' was utilized in the logical expression before it was verified against null in the same logical expression. ConsentController.cs 87

В данном случае переменная model сначала используется, а потом её значение проверяется на равенство null, хотя должно быть наоборот.

Стоит отметить, что предупреждения, связанные с NullReferenceException, были выданы анализатором и на другие проекты. Однако они были менее примечательны и их было не так много.

9-е место

Перейдём к следующему срабатыванию. На этот раз проект eShopOnContainers.

private bool CheckSameOrigin(string urlHook, string url)
{
  var firstUrl = new Uri(urlHook, UriKind.Absolute);
  var secondUrl = new Uri(url, UriKind.Absolute);

  return firstUrl.Scheme == secondUrl.Scheme &&
         firstUrl.Port == secondUrl.Port &&
         firstUrl.Host == firstUrl.Host;
}

Предупреждение анализатора: V3001 There are identical sub-expressions 'firstUrl.Host' to the left and to the right of the '==' operator. GrantUrlTesterService.cs 48

Довольно лёгкая для поиска "глазами" ошибка, хотя, наверное, для этого требуется знать, что в методе кроется ошибка. Анализатор обнаружил фрагмент кода, в котором имеется череда сравнений и последнее из них является аномальным. Свойство Host объекта firstUrl сравнивается само с собой. Сложно определить, насколько критична данная оплошность, но, скорее всего, где-то имеются нарушения в логике из-за неправильного возвращаемого значения.

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

8-е место

Анализатор выдал данное предупреждение на проекте Cloudscribe благодаря новым аннотациям.

public async Task<IdentityResult> TryCreateAccountForExternalUser(....)
{
  ....

  var user = new SiteUser
  {
    SiteId = Site.Id,
    UserName = userName,
    Email = email,
    FirstName = info.Principal.FindFirstValue(ClaimTypes.GivenName),
    LastName = info.Principal.FindFirstValue(ClaimTypes.Surname),
    AccountApproved = Site.RequireApprovalBeforeLogin ? false : true
  };
  
  user.DisplayName = _displayNameResolver.ResolveDisplayName(user);

  var result = await CreateAsync(user as TUser);
  if(result.Succeeded)
  {
    result = await AddLoginAsync(user as TUser, info);
  }

  return result;
}

Предупреждение анализатора: V3156 The first argument of the 'AddLoginAsync' method is not expected to be null. Potential null value: user as TUser. SiteUserManager.cs 257

Давайте более детально разберём данное предупреждение.

В метод может передаваться null при вызове AddLoginAsync. Оператор as в случае неудачного преобразования выдаст null.

Кстати, именно благодаря тому, что мы проаннотировали этот метод, у анализатора имеется информация о запрете на передачу null первому параметру.

Также интересный момент с кастом объекта user класса SiteUser к TUser, который является generic-параметром. Взглянем, что из себя представляет универсальный параметр:

public class SiteUserManager<TUser> : UserManager<TUser> where TUser : SiteUser

Идея в том, что на место TUser можно подставить SiteUser или любой тип, который наследует его.

Давайте ещё раз взглянем на код:

public async Task<IdentityResult> TryCreateAccountForExternalUser(....)
{
  ....

  var user = new SiteUser
  {
    ....
  };
  
  user.DisplayName = _displayNameResolver.ResolveDisplayName(user);

  var result = await CreateAsync(user as TUser);
  if(result.Succeeded)
  {
    result = await AddLoginAsync(user as TUser, info);
  }

  return result;
}

Получается, что в методы CreateAsync и AddLoginAsync отправляется null вообще во всех случаях, когда на месте TUser не SiteUser, а какой-нибудь его наследник.

В таком случае возникает вопрос. Зачем использовать generic-параметр, если код работает только лишь с одним конкретным типом? Возможно, это особенность только данной функции, но она не очень очевидна.

7-е место

На 7 строчке нашего рейтинга ошибка из проекта Piranha. Давайте сыграем в небольшую игру для самых внимательных. Попробуйте найти в следующем фрагменте ошибку.

public override async Task InitializeAsync()
{
  using (var api = CreateApi())
  {
    // Import content types
    new ContentTypeBuilder(api)
        .AddType(typeof(BlogArchive))
        .Build();
    new ContentTypeBuilder(api)
        .AddType(typeof(BlogPost))
        .Build();
    
    // Add site
    var site = new Site
    {
      Id = SITE_ID,
      Title = "Comment Site",
      InternalId = "CommentSite",
      IsDefault = true
    };
    await api.Sites.SaveAsync(site);  

    // Add archive
    var blog = await BlogArchive.CreateAsync(api);
    blog.Id = BLOG_ID;
    blog.SiteId = SITE_ID;
    blog.Title = "Blog";
    blog.EnableComments = true;
    blog.Published = DateTime.Now;
    await api.Pages.SaveAsync(blog);

    var news = await BlogArchive.CreateAsync(api);
    news.Id = NEWS_ID;
    news.SiteId = SITE_ID;
    news.Title = "News";
    blog.EnableComments = true;
    news.Published = DateTime.Now;
    await api.Pages.SaveAsync(news);

    // Add posts
    var blogPost = await BlogPost.CreateAsync(api);
    blogPost.Id = BLOGPOST_ID;
    blogPost.BlogId = BLOG_ID;
    blogPost.Category = "The Category";
    blogPost.Title = "Welcome To The Blog";
    blogPost.Published = DateTime.Now;
    await api.Posts.SaveAsync(blogPost);

    var newsPost = await BlogPost.CreateAsync(api);
    newsPost.Id = NEWSPOST_ID;
    newsPost.BlogId = NEWS_ID;
    newsPost.Category = "The Category";
    newsPost.Title = "Welcome To The News";
    newsPost.Published = DateTime.Now;
    await api.Posts.SaveAsync(newsPost);
  }
}

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

public override async Task InitializeAsync()
{
  using (var api = CreateApi())
  { 
    ....
    // Add archive
    var blog = await BlogArchive.CreateAsync(api);
    blog.Id = BLOG_ID;
    blog.SiteId = SITE_ID;
    blog.Title = "Blog";
    blog.EnableComments = true;
    blog.Published = DateTime.Now;
    await api.Pages.SaveAsync(blog);

    var news = await BlogArchive.CreateAsync(api);
    news.Id = NEWS_ID;
    news.SiteId = SITE_ID;
    news.Title = "News";
    blog.EnableComments = true;    // <=
    news.Published = DateTime.Now;
    await api.Pages.SaveAsync(news);
    ....
  }
}

Предупреждение анализатора: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'news' variable should be used instead of 'blog' CommentTests.cs 94

Имеются два похожих по структуре блока кода. Анализатор сигнализирует о возможной опечатке во втором блоке в строке blog.EnableComments = true. Скорее всего разработчик сделал эту ошибку из-за copy-paste похожего блока кода, идущего раньше, и просто забыл поменять в одном месте blog на news. Очень занимателен тот факт, что подобные ошибки делают все программисты, независимо от опыта.

6-е место

Следующая ошибка была обнаружена в проекте OrchardCore.

public async Task ConfigureOAuthAsync(HttpRequestMessage request)
{
  var container = await _siteService.GetSiteSettingsAsync();
  var settings = container.As<TwitterSettings>();
  var protrector = _dataProtectionProvider
                   .CreateProtector(TwitterConstants
                                    .Features
                                    .Twitter);
  var queryString = request.RequestUri.Query;

  if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret))
    settings.ConsumerSecret =
    protrector.Unprotect(settings.ConsumerSecret);
  if (!string.IsNullOrWhiteSpace(settings.ConsumerSecret))
    settings.AccessTokenSecret =   
    protrector.Unprotect(settings.AccessTokenSecret);
  ....
}

Предупреждение анализатора: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'AccessTokenSecret' variable should be used instead of 'ConsumerSecret' TwitterClientMessageHandler.cs 51

Анализатор предупреждает о двух одинаковых проверках. У объекта settings вызывается свойство ConsumerSecret, хотя, судя по всему, должно быть использовано AccessTokenSecret, которое действительно существует.

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

5-е место

Вот мы и прошли первую половину топа. Дальше будет ещё интересней. На пятом месте у нас снова предупреждение из Squidex.

public Task EnhanceAsync(UploadAssetCommand command)
{
  try
  {
    using (var file = Create(new FileAbstraction(command.File),
                                                 ReadStyle.Average))
    {
      ....
      var pw = file.Properties.PhotoWidth;
      var ph = file.Properties.PhotoHeight;

      if (pw > 0 && pw > 0)                        // <=
      {
        command.Metadata.SetPixelWidth(pw);
        command.Metadata.SetPixelHeight(ph);
      }
      ....
    }
    return Task.CompletedTask;
  }
  catch
  {
    return Task.CompletedTask;
  }
}

Предупреждение анализатора: V3001 There are identical sub-expressions 'pw > 0' to the left and to the right of the '&&' operator. FileTagAssetMetadataSource.cs 80

Анализатор подсказывает, что слева и справа от оператора имеются два одинаковых подвыражения. Скорее всего, в if должна происходить проверка, что ширина и высота больше 0. Вместо этого два раза проверяется ширина. Следовательно, искажается работа программы, т. к. проверка изображения на соответствие размерам не происходит должным образом.

4-е место

А вот это предупреждение на проекте BTCPay Server было выдано благодаря добавлению новых аннотаций методов.

public async Task<IActionResult> CalculateAmount(....)
{
  try
  {
    ....
    while (true)
    {
      if (callCounter > 10)
      {
        BadRequest();                                         // <=
      }
      var computedAmount = await client.GetExchangeAmount(....);
      callCounter++;
    
      if (computedAmount < toCurrencyAmount)
      {
        ....
      }
      else
      {
        return Ok(currentAmount);
      }
    }
  }
  catch (Exception e)
  {
    return BadRequest(new BitpayErrorModel()
    {
      Error = e.Message
    });
  }
}

Предупреждение анализатора: V3010 The return value of function 'BadRequest' is required to be utilized. ChangellyController.cs 72

PVS-Studio сообщает, что вызов не имеет смысла без использования возвращаемого значения. Анализатор не может раскрыть тело метода BadRequest, но, благодаря аннотированию, у него появилась информация о том, что возвращаемое значение нужно использовать.

Похоже, что здесь пропустили return. Возможно, что из-за данной оплошности ломается логика работы метода CalculateAmount. Пропущенный return у BadRequest приводит, как минимум, к большему количеству итераций, а как максимум, к зависанию программы.

3-е место

Вот мы и дошли до тройки лучших предупреждений. На 3-е место мы поставили предупреждение, выданное анализатором на код из проекта Squidex.

private static AssetFolderDto CreateLinks(AssetFolderDto response,
                                          Resources resources)
{
  var values = new { app = resources.App, id = response.Id };

  if (resources.CanUpdateAsset)
  {
    response.AddPutLink("update", resources.Url<AssetFoldersController>(x =>
                                  nameof(x.PutAssetFolder), values));

    response.AddPutLink("move", resources.Url<AssetFoldersController>(x =>
                                nameof(x.PutAssetFolderParent), values));
  }
            
  if (resources.CanUpdateAsset)
  {
    response.AddDeleteLink("delete", resources.Url<AssetFoldersController>(x =>
                                     nameof(x.DeleteAssetFolder), values));
  }

  return response;
}

Предупреждение анализатора: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 50, 57. AssetFolderDto.cs 50

Анализатор обнаружил два рядом стоящих if с одинаковыми условиями. Такое написание явно вызывает подозрения. Думаю, все согласятся, что ожидаешь в условии второго if увидеть resources.CanDeleteAsset. Такое свойство действительно доступно, и оно используется в похожем методе.

private static AssetDto CreateLinks(AssetDto response,
                                    Resources resources)
{
  ....
  if (resources.CanUpdateAsset)
   ....

  if (resources.CanUploadAsset)
   ....

  if (resources.CanDeleteAsset)
    ....
  ....
}

2-е место

Серебро в этот раз получает ошибка, которая была найдена в проекте Squidex.

private IEnumerable<IMigration?> ResolveMigrators(int version)
{
  yield return serviceProvider.GetRequiredService<StopEventConsumers>();

  // Version 06: Convert Event store. Must always be executed first.
  if (version < 6)
  {
    yield return serviceProvider.GetRequiredService<ConvertEventStore>();
  }

  // Version 22: Integrate Domain Id.
  if (version < 22)
  {
    yield return serviceProvider.GetRequiredService<AddAppIdToEventStream>();
  }

  // Version 07: Introduces AppId for backups.
  else if (version < 7)                                 // <=
  {
    yield return serviceProvider
                 .GetRequiredService<ConvertEventStoreAppId>();
  }

  // Version 05: Fixes the broken command architecture and requires a
  // rebuild of all snapshots.
  if (version < 5)
  {
    yield return serviceProvider.GetRequiredService<RebuildSnapshots>();
  }
  else
  {
    // Version 09: Grain indexes.
    if (version < 9)
    {
      yield return serviceProvider.GetService<ConvertOldSnapshotStores>();
    }

    ....
  }

  // Version 13: Json refactoring
  if (version < 13)
  {
    yield return serviceProvider.GetRequiredService<ConvertRuleEventsJson>();
  }

  yield return serviceProvider.GetRequiredService<StartEventConsumers>();
}

Предупреждение анализатора: V3022 Expression 'version < 7' is always false. MigrationPath.cs 55

Сразу скажу, что на месте "...." находятся ещё несколько проверок — я решил их сократить для улучшения читаемости. Полный код метода можно найти тут.

Анализатор указывает на то, что условие version < 7 всегда ложно. Ветвь else никогда не будет выполнена, т. к. условие version < 22 включает в себя и то, что version < 7. Подобные ошибки бывает тяжело найти при написании кода, особенно при таком количестве условных конструкций, но они сразу видны, когда вам на них указывают.

1-е место

С небольшим отрывом от предыдущего места золото забирает интересная ошибка из OrchardCore.

public async ValueTask<Completion> WriteToAsync(....)
{
  ....
  if (displayFor != null)
  {
    ....
  }
  else if (editFor != null)
  {
    ....
  }
  else if (adminFor != null)
  {
    ....
  }
  else if (removeFor != null)
  {
    contentItem = removeFor;
    var metadata =
      await contentManager
            .PopulateAspectAsync<ContentItemMetadata>(removeFor);

    if (metadata.RemoveRouteValues != null)
    {
      if (routeValues != null)
      {
        foreach (var attribute in routeValues)
        {
          metadata.RemoveRouteValues.Add(attribute.Key, attribute.Value);
        }
      }

      customAttributes["href"] = urlHelper
                                 .Action(metadata.RemoveRouteValues["action"]
                                 .ToString(), metadata.RemoveRouteValues);
    }
  }
  else if (createFor != null)
  {
    contentItem = createFor;
    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);
        }
      }

      customAttributes["href"] = urlHelper
                                 .Action(metadata.CreateRouteValues["action"]
                                 .ToString(), metadata.CreateRouteValues);
    }
  }
  ....
}

Предупреждение анализатора: V3080 Possible null dereference. Consider inspecting 'metadata.CreateRouteValues'. ContentAnchorTag.cs 188

Анализатор обнаружил код, который может привести к доступу по нулевой ссылке.

Пример кода выше хоть и сокращённый, но всё ещё достаточно большой. Упростим его ещё немного:

public async ValueTask<Completion> WriteToAsync(....)
{
  ....
  if (metadata.CreateRouteValues == null)
  {
    if (routeValues != null)
    {
      foreach (var attribute in routeValues)
      {
        metadata.CreateRouteValues.Add(attribute.Key, attribute.Value);
      }
    }
    ....
  }
  ....
}

Имеется проверка: если свойство metadata.CreateRouteValues равно null, то далее для него вызывается метод Add. Естественно, это ошибка. Примечательно то, что похожих блоков кода много. Для понимания я привёл в большом примере один из них. Во всех случаях, кроме последнего, до этого идёт проверка != null. Скорее всего разработчик опечатался при копировании кода.

Заключение

Проделанная работа явно положительно скажется на анализе проектов, использующих ASP.NET Core. Аннотирование методов полезно не только для получения новых хороших срабатываний, но также помогает убрать ложно-позитивные.

Мы покрыли только часть классов, которые посчитали популярными, основываясь на найденных проектах. Кстати, вы можете написать в комментариях о каких-нибудь местах в ASP.NET Core проектах, где мы не выдали предупреждение или сработали некорректно. Особенно про места, с которыми могли бы помочь аннотации.

Данный топ в очередной раз доказывает, что статический анализ действительно позволяет находить интересные ошибки в проектах. И это касается не только ASP-проектов, но и всего остального. А как вы считаете, сможет ли PVS-Studio найти что-нибудь в ваших проектах? Предлагаю заглянуть к нам на сайт и попробовать :).

Теги:

Хабы: