Александр Сеничкин

21 Окт 2019

Теги:

#CSharp

Ищем и анализируем ошибки в коде Orchard CMS. Часть 2

21 Окт 2019

Автор: Александр Сеничкин

Эта статья – результат повторной проверки проекта Orchard с помощью статического анализатора PVS-Studio. Orchard это система управления контентом с открытым исходным кодом, которая является частью галереи ASP.NET-проектов некоммерческого фонда Outercurve Foundation. Проверка интересна тем, что проект и анализатор сильно выросли со времени первого анализа. Новые срабатывания и интересные ошибки ждут вас.

О проекте Orchard CMS

Мы проверяли Orchard с помощью PVS-Studio три года назад. C# анализатор с тех пор сильно развился: мы улучшили data flow анализ, разработали межпроцедурный анализ, добавили новых диагностик и исправили ряд ложных срабатываний. Кроме того, анализ показал, что все замечания из предыдущей статьи были исправлены. Это значит, что цель достигнута - код стал лучше.

Хочется верить, что разработчики уделят внимание этой статье и внесут необходимые правки. Ещё лучше, если введут практику регулярного использования PVS-Studio. Напомню, что для open-source проектов мы предоставляем бесплатный вариант лицензии. Кстати, есть и другие варианты, подходящие и закрытым проектам.

Код проекта Orchard можно загрузить отсюда, как это сделал я. Полное описание проекта можно посмотреть здесь. Если у вас ещё нет нашего анализатора PVS-Studio – можно скачать пробную версию отсюда. Я использовал версию PVS-Studio версии 7.05 Beta. Поделюсь результатами её работы. Надеюсь, вы согласитесь с полезностью использования PVS-Studio. Главное – использовать анализатор регулярно.

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

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

"В прошлой проверке участвовали все файлы (3739 штук) с расширением .cs. В общей сложности они содержали 214 564 строк кода. По итогам проверки было получено 137 предупреждений. На первом (высоком) уровне достоверности было получено 39 предупреждений. На втором уровне (среднем) было получено 60 предупреждений."

На данный момент в проекте 2767 файлов .cs, то есть проект стал меньше на тысячу файлов. Судя по этому уменьшению количества файлов и смене названия репозитория – из проекта выделили ядро (коммит 966). В ядре 108 287 строк кода и анализатор выдал 153 предупреждения, 33 на высоком уровне, 70 на среднем. Предупреждения третьего уровня мы обычно не рассматриваем, я не стал нарушать традицию.

Предупреждение PVS-Studio: V3110 Possible infinite recursion inside 'TryValidateModel' method. PrefixedModuleUpdater.cs 48

public bool TryValidateModel(object model, string prefix)
{
  return TryValidateModel(model, Prefix(prefix));
}

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

public bool TryValidateModel(object model)
{
  return _updateModel.TryValidateModel(model);
}

Я предполагаю, что, как и в случае с перегруженным методом, разработчик хотел вызывать методы через _updateModel. Компилятор не увидел ошибки, _updateModel имеет тип IUpdateModel и текущий класс также реализует этот интерфейс. Поскольку в методе нет ни одного условия для защиты от StackOverflowException, возможно, метод не вызывался ни разу, но я бы на это не рассчитывал. Если предположение верно – исправленный вариант будет выглядеть так:

public bool TryValidateModel(object model, string prefix)
{
  return _updateModel.TryValidateModel(model, Prefix(prefix));
}

Предупреждение PVS-Studio: V3008 The 'content' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 197, 190. DynamicCacheTagHelper.cs 197

public override async Task ProcessAsync(....)
{ 
  ....
  IHtmlContent content;
  ....
  try
  {
    content = await output.GetChildContentAsync();
  }
  finally
  {
    _cacheScopeManager.ExitScope();
  }
  content = await ProcessContentAsync(output, cacheContext);
  ....
}

Анализатор увидел два присваивания в локальную переменную content. GetChildContentAsync – метод из библиотеки, который недостаточно распространён, чтобы мы решили разобраться и проаннотировать его. Так что, к сожалению, ни мы, ни анализатор ничего не знаем о возвращаемом объекте метода и побочных действиях. Но мы точно можем сказать, что присвоение результата в content не имеет смысла без использования. Возможно, это вообще не ошибка, просто лишняя операция. Я не смог прийти к однозначному выводу о способе исправления. Оставлю это решение разработчикам.

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'itemTag'. CoreShapes.cs 92

public async Task<IHtmlContent> List(....string ItemTag....)
{
  ....
  string itemTagName = null;
  if (ItemTag != "-")
  {
    itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag;
  }
  var index = 0;
  foreach (var item in items)
  {
    var itemTag = String.IsNullOrEmpty(itemTagName) ? null : ....;
    ....
    itemTag.InnerHtml.AppendHtml(itemContent);
    listTag.InnerHtml.AppendHtml(itemTag);
    ++index;
  }
  return listTag;
}

В данном коде анализатор обнаружил опасное разыменование itemTag. Хороший пример отличия работы статического анализатора от проверяющего человека. В методе есть параметр с именем ItemTag и локальная переменная с именем itemTag. Как вы понимаете, для компилятора разница огромна! Это две разные, хоть и связанные переменные. Связь идёт через третью переменную, itemTagName. Сценарий выброса исключения такой: аргумент ItemTag равен "-", в itemTagName не присвоится значение, он останется нулевой ссылкой, а если он будет нулевой ссылкой – локальная itemTag тоже станет нулевой ссылкой. Думаю, здесь лучше выбросить исключение, после проверки строки.

public async Task<IHtmlContent> List(....string ItemTag....)
{
  ....
  string itemTagName = null;
  if (ItemTag != "-")
  {
    itemTagName = string.IsNullOrEmpty(ItemTag) ? "li" : ItemTag;
  }
  var index = 0;
  foreach (var item in items)
  {
    var itemTag = ....;
    if(String.IsNullOrEmpty(itemTag))
      throw ....
    ....
    itemTag.InnerHtml.AppendHtml(itemContent);
    listTag.InnerHtml.AppendHtml(itemTag);
    ++index;
  }
  return listTag;
}

Предупреждение PVS-Studio: V3095 The 'remoteClient' object was used before it was verified against null. Check lines: 49, 51. ImportRemoteInstanceController.cs 49

public async Task<IActionResult> Import(ImportViewModel model)
{
  ....
  var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....);
  var apiKey = Encoding.UTF8.GetString(....(remoteClient.ProtectedApiKey));
  if (remoteClient == null || ....)
  {
    ....
  }
  ....
}

Анализатор увидел разыменование remoteClient и проверку на null ниже. Это действительно потенциальный NullReferenceException, ведь метод FirstOrDefault может вернуть значение по умолчанию (null для ссылочных типов). Полагаю, для исправления кода достаточно перенести проверку выше:

public async Task<IActionResult> Import(ImportViewModel model)
{
  ....
  var remoteClient = remoteClientList.RemoteClients.FirstOrDefault(....);
  if (remoteClient != null)
     var apiKey = UTF8.GetString(....remoteClient.ProtectedApiKey);
  else if (....)
  {
    ....
  }
  ....
}

Хотя, возможно, стоит заменить FirstOrDefault на First и убрать проверку совсем.

Из PVS-Studio 7.05 Beta:

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

Не могу не поделиться результатом разрабатываемой диагностики на данном проекте. 27 потенциально опасных мест. Вот некоторые из них:

ContentTypesAdminNodeNavigationBuilder.cs 71:

var treeBuilder = treeNodeBuilders.Where(....).FirstOrDefault();
await treeBuilder.BuildNavigationAsync(childNode, builder, treeNodeBuilders);

ListPartDisplayDriver.cs 217:

var contentTypePartDefinition = ....Parts.FirstOrDefault(....);
return contentTypePartDefinition.Settings....;

ContentTypesAdminNodeNavigationBuilder.cs 113:

var typeEntry = node.ContentTypes.Where(....).FirstOrDefault();
return AddPrefixToClasses(typeEntry.IconClass);

Предупреждение PVS-Studio: V3080 Possible null dereference of method return value. Consider inspecting: CreateScope(). SetupService.cs 136

public async Task<string> SetupInternalAsync(SetupContext context)
{
  ....
  using (var shellContext = await ....)
  {
    await shellContext.CreateScope().UsingAsync(....);
  }
  ....
}

Итак, анализатор отметил разыменование результата вызова метода CreateScope. Метод CreateScope совсем небольшой, приведу его целиком:

public ShellScope CreateScope()
{
  if (_placeHolder)
  {
    return null;
  }
  var scope = new ShellScope(this);
  // A new scope can be only used on a non released shell.
  if (!released)
  {
    return scope;
  }
  scope.Dispose();
  return null;
}

Как видите, он может вернуть null в двух случаях. Анализатор не может сказать, по какой из веток пойдёт выполнение программы, поэтому отмечает код как подозрительный. В своём коде я бы сразу добавил проверку на null.

Возможно, я сужу предвзято, но все асинхронные методы надо страховать от NullReferenceException как можно лучше. Отлаживать такие вещи – очень сомнительное удовольствие.

В данном случае у метода CreateScope четыре вызова и в двух проверка есть, а в двух других отсутствует. Причем пара без проверки похожа на копипасту (один класс, один метод, разыменовывается для вызова UsingAsync). Первый вызов из этой пары я уже привёл и, разумеется, на второй вызов тоже есть аналогичное предупреждение анализатора:

V3080 Possible null dereference of method return value. Consider inspecting: CreateScope(). SetupService.cs 192

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

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

Классика копипасты. ConsumerSecret проверили дважды, а AccessTokenSecret – ни разу. Очевидно, надо поправить:

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: V3139 Two or more case-branches perform the same actions. SerialDocumentExecuter.cs 23

Ещё одна ошибка копипасты. Чтобы было понятнее – приведу класс целиком, так как он небольшой.

public class SerialDocumentExecuter : DocumentExecuter
{
  private static IExecutionStrategy ParallelExecutionStrategy 
    = new ParallelExecutionStrategy();
  private static IExecutionStrategy SerialExecutionStrategy
    = new SerialExecutionStrategy();
  private static IExecutionStrategy SubscriptionExecutionStrategy
    = new SubscriptionExecutionStrategy();

  protected override IExecutionStrategy SelectExecutionStrategy(....)
  {
    switch (context.Operation.OperationType)
    {
      case OperationType.Query:
        return SerialExecutionStrategy;

      case OperationType.Mutation:
        return SerialExecutionStrategy;

      case OperationType.Subscription:
        return SubscriptionExecutionStrategy;

      default:
        throw ....;
    }
  }
}

Анализатор посчитал подозрительным две одинаковые ветки case. И действительно, в классе три сущности, две возвращаются при обходе, одна - нет. Если так задумано и от третьего варианта отказались, то можно удалить его, объединив две ветки так:

switch (context.Operation.OperationType)
{
  case OperationType.Query:
  case OperationType.Mutation:
    return SerialExecutionStrategy;

  case OperationType.Subscription:
    return SubscriptionExecutionStrategy;

  default:
    throw ....;
}

Если же это ошибка копипасты – надо поправить возвращаемое поле так:

switch (context.Operation.OperationType)
{
  case OperationType.Query:
    return ParallelExecutionStrategy;

  case OperationType.Mutation:
    return SerialExecutionStrategy;

  case OperationType.Subscription:
    return SubscriptionExecutionStrategy;

  default:
    throw ....;
}

Или наоборот. Я не знаком с проектом и не могу соотнести названия типов операций и стратегий.

switch (context.Operation.OperationType)
{
  case OperationType.Query:
    return SerialExecutionStrategy; 

  case OperationType.Mutation:
    return ParallelExecutionStrategy;

  case OperationType.Subscription:
    return SubscriptionExecutionStrategy;

  default:
    throw ....;
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'request'. GraphQLMiddleware.cs 148

private async Task ExecuteAsync(HttpContext context....)
{
  ....
  GraphQLRequest request = null;
  ....
  if (HttpMethods.IsPost(context.Request.Method))
  {
    ....
  }
  else if (HttpMethods.IsGet(context.Request.Method))
  {
    ....
    request = new GraphQLRequest();
    ....
  }
  var queryToExecute = request.Query;
  ....
}

В первом if блоке переменная request получает значение отличное от null в нескольких местах, но везде с вложенными условиями. Я не стал приводить все эти условия, так как пример вышел бы слишком громоздким. Достаточно первых условий, проверяющих тип http метода IsGet или IsPost. В классе Microsoft.AspNetCore.Http.HttpMethods есть девять статических методов для проверки типа запроса. Значит если в метод ExecuteAsync попадает, например, Delete или Set запрос – будет выброшен NullReferenceException. Даже если на данный момент такие методы не поддерживаются вообще – лучше сделать проверку с исключением. Ведь требования к системе могут поменяться. Пример проверки ниже:

private async Task ExecuteAsync(HttpContext context....)
{
  ....
  if (request == null)
    throw ....;
  var queryToExecute = request.Query;
  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference of method return value. Consider inspecting: Get<ContentPart>(...). ContentPartHandlerCoordinator.cs 190

Большую часть срабатываний диагностики V3080 проще посмотреть в среде разработки. Нужна навигация по методам, подсветка типов, ободряющая атмосфера IDE. Я стараюсь максимально сократить примеры, чтобы вам было удобнее читать. Но если у меня получается неважно, или если вы хотите проверить свой уровень программирования и разобраться самостоятельно – советую посмотреть срабатывания этой диагностики на любом открытом или собственном проекте.

public override async Task LoadingAsync(LoadContentContext context)
{
  ....
  context.ContentItem.Get<ContentPart>(typePartDefinition.Name)
                     .Weld(fieldName, fieldActivator.CreateInstance());
  ....
}

На эту строку ругается анализатор. Посмотрим на метод Get:

public static TElement Get<TElement>(this ContentElement contentElement....)
        where TElement : ContentElement
{
    return (TElement)contentElement.Get(typeof(TElement), name);
}

Он вызывает свою перегрузку. Посмотрим и на неё:

public static ContentElement Get(this ContentElement contentElement....)
{
  ....
  var elementData = contentElement.Data[name] as JObject;
  if (elementData == null)
  {
    return null;
  }
  ....
}

Получается, если при получении элемента из Data по индексатору name мы получим сущность не совместимого с JObject типа, то метод Get вернёт null. Не возьмусь судить о вероятности этого, так как эти типы из библиотеки Newtonsoft.Json, а у меня немного опыта работы с ней. Но разработчик подозревал, что нужного элемента может не быть. Значит, не стоит забывать о такой возможности и при обращении к результатам чтения. Я бы добавил выброс исключения в самый первый Get, если мы считаем, что узел должен быть, или проверку перед разыменованием, если отсутствие узла не поменяет общую логику (берётся значение по умолчанию, например).

Вариант один:

public static ContentElement Get(this ContentElement contentElement....)
{
  ....
  var elementData = contentElement.Data[name] as JObject;
  if (elementData == null)
  {
    throw....
  }
  ....
}

Вариант два:

public override async Task LoadingAsync(LoadContentContext context)
{
  ....
  context.ContentItem.Get<ContentPart>(typePartDefinition.Name)
                     ?.Weld(fieldName, fieldActivator.CreateInstance());
  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'results'. ContentQueryOrchardRazorHelperExtensions.cs 19

public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....)
{
  var results = await orchardHelper.QueryAsync(queryName, parameters);
  ....
  foreach (var result in results)
  {
    ....
  }
 ....
}

Довольно простой пример относительно предыдущего. Анализатор считает, что результат вызова QueryAsync может быть нулевой ссылкой. Вот этот метод:

public static async Task<IEnumerable> QueryAsync(....)
{
  ....
  var query = await queryManager.GetQueryAsync(queryName);
  if (query == null)
  {
    return null;
  }
  ....
}

Здесь GetQueryAsync – интерфейсный метод, нельзя быть уверенным в каждой реализации. Тем более, что в этом же проекте есть такой вариант:

public async Task<Query> GetQueryAsync(string name)
{
  var document = await GetDocumentAsync();
  if(document.Queries.TryGetValue(name, out var query))
  {
    return query;
  }
  return null;
}

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

public static async Task<IEnumerable<ContentItem>> ContentQueryAsync(....)
{
  var results = await orchardHelper.QueryAsync(queryName, parameters);
  if(results == null)
    throw ....;
  ....
  foreach (var result in results)
  {
    ....
  }
 ....
}

Заключение

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

И хотя разовые проверки не являются максимально эффективными, призываю вас загрузить и попробовать PVS-Studio на своём проекте - вдруг удастся найти что-нибудь интересное?

#CSharp