Хабрахабр

Ищем и анализируем ошибки в коде Orchard CMS

Picture 6

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

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

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

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

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

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

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

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

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

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(....)
finally { _cacheScopeManager.ExitScope(); } content = await ProcessContentAsync(output, cacheContext); ....
}

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

Consider inspecting 'itemTag'. Предупреждение PVS-Studio: V3080 Possible null dereference. 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 и убрать проверку совсем.

05 Beta: Из PVS-Studio 7.

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

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 как можно лучше.

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

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

Perhaps, this is a typo and 'AccessTokenSecret' variable should be used instead of 'ConsumerSecret' TwitterClientMessageHandler.cs 52
Предупреждение PVS-Studio: V3127 Two similar code fragments were found.

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(...). ContentPartHandlerCoordinator.cs 190

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

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

Picture 14

Заключение

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

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

Scanning the code of Orchard CMS for Bugs. Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Alexander Senichkin.

Теги
Показать больше

Похожие статьи

Добавить комментарий

Ваш e-mail не будет опубликован. Обязательные поля помечены *

Кнопка «Наверх»
Закрыть