Миллионы людей используют веб-приложения, созданные на базе ASP.NET Core. Поэтому мы решили улучшить производительность PVS-Studio при анализе подобных проектов.
Чтобы продемонстрировать результаты нашей работы, мы проверим несколько проектов с открытым исходным кодом и представим лучшие из них.
Введение
Мы часто говорим о тех, которые используем технологии статического анализа .Один из них — аннотация.
Что это такое и зачем это нужно? Часто случается, что при парсинге кода невозможно выявить тело метода.
Например, если метод объявлен в библиотеке, исходный код которой недоступен.
Однако даже если код с открытым исходным кодом, есть некоторые высокоуровневые выводы о том, как работает функция, которые анализатор просто не может сделать, и лучше подсказать их вручную.
В таких случаях аннотации являются для анализатора простым и удобным способом получить информацию об особенностях работы этого метода.
И мы, как разработчики PVS-Studio, можем поместить в него необходимую информацию.
Это могут быть данные о значениях, возвращаемых методом, или о том, какие значения следует или не следует передавать в качестве аргументов.
Уже выходил раньше Примечание об аннотировании методов Unity .
В нем мы рассказали о трудностях, с которыми нам пришлось столкнуться и о том, как, пройдя дальше нулевой в аргументе метода оказалось, что это приводило к сбою самого редактора :).
Эта статья будет немного другой.
Сегодня мы не только поговорим о том, как мы улучшили анализ проектов с помощью ASP.NET Core, но и рассмотрим интересные проблемы в коде, которые мы обнаружили в ходе работы.
Некоторые из них анализатор начал находить благодаря аннотациям.
Он уже знал, как искать других.
Мы записали их, потому что эти предупреждения тоже показались заслуживающими внимания.
Кстати, проекты, на которых мы тестировали анализатор, были взяты отсюда .
Основные критерии отбора: на момент проверки проект находится в стадии разработки и ошибок компиляции нет.
Аннотирование основных методов ASP.NET
Как и в случае с 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
- И т. д.
Например, возьмем метод ФизическийФайл(Строка, Строка) сорт КонтроллерБаза .
Если мы посмотрим на документация , то становится ясно, что этот метод принимает абсолютный путь к файлу и тип содержимого файла.
Также не забывайте, что этот метод имеет возвращаемое значение.
Это уже полезно для аннотаций, но есть еще чему поучиться.
Для получения дополнительной информации вы можете использовать 2 метода:
- найти источники на GitHub и просто посмотреть, как работает функция;
- протестируйте функцию вручную, передавая в качестве аргументов разные комбинации значений.
- метод принимает путь к файлу в качестве первого аргумента;
- второй аргумент, указывающий тип содержимого файла, не должен иметь значения нулевой .
В противном случае будет выдано исключение;
- вызов метода не имеет смысла, если его возвращаемое значение не используется.
Теперь, когда парсер встречает аннотированный метод, он уже знает, как его обработать.
Проверка проекта
Места в нашем топе ранжируются субъективно — вы всегда можете высказать свое мнение об ошибке.Возможно, вы могли бы переместить некоторые предупреждения в другое место вверху.
Расскажите нам об этом в комментариях.
Ну, хватит предисловий! Перейдем к самому интересному — проверке проектов.
10 место
Откройте этот топ предупреждений, сгенерированных анализатором на проекте.Да-да, там не один триггер, а сразу два.
Так что технически сегодня мы имеем дело с одиннадцатью предупреждениями, а не с десятью.
:)
Предупреждение анализатора: В3080 Возможное нулевое разыменование.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; }
Рассмотрите возможность проверки TargetValue. RequiredWhenAttribute.cs 78 Анализатор обратил внимание на то, что можно разыменовать нулевую ссылку.
Если переменная зависимое значение не равный нулевой , А Целевое значение - равно, то будет выброшено всеми любимое исключение 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)
{
.
} .
}
Предупреждение анализатора: В3027 Переменная «модель» использовалась в логическом выражении до того, как она была проверена на соответствие нулю в том же логическом выражении.
Согласиеконтроллер.
cs 87 В этом случае переменная модель сначала используется, а затем его значение проверяется на равенство нулевой , хотя должно быть наоборот. Стоит отметить, что предупреждения, связанные с 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;
}
Предупреждение анализатора: В3001 Слева и справа от оператора «==» находятся идентичные подвыражения firstUrl.Host. GrantUrlTesterService.cs 48 Эту ошибку довольно легко обнаружить глазами, хотя для этого, вероятно, необходимо знать, что в методе есть ошибка.
Анализатор обнаружил фрагмент кода, в котором есть серия сравнений и последнее из них аномальное.
Свойство Хозяин объект первый URL сравнивает сам с собой.
Сложно определить, насколько критична эта оплошность, но скорее всего где-то есть нарушения в логике из-за неправильного возвращаемого значения.
Интересные случаи, когда одновременно проявлялись две типичные закономерности ошибок: эффект последней строки И ошибка в функциях сравнения .
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;
}
Предупреждение анализатора: В3156 Ожидается, что первый аргумент метода AddLoginAsync не будет иметь значение NULL. Потенциальное нулевое значение: пользователь как TUser. SiteUserManager.cs 257 Давайте внимательнее посмотрим на это предупреждение.
Метод можно передать нулевой при звонке Аддлогин асинк .
Оператор как если преобразование не удалось, отобразится нулевой .
Кстати, именно благодаря тому, что мы аннотировали этот метод, у анализатора есть информация о запрете передачи нулевой первый параметр.
Также интересный момент с приведением объектов пользователь сорт Пользователь сайта К TUser , который является общим параметром.
Давайте разберемся, что такое универсальный параметр: public class SiteUserManager<TUser> : UserManager<TUser> where TUser : SiteUser
Идея в том, что на месте TUser можно заменить Пользователь сайта или любой тип, который его наследует. Давайте еще раз посмотрим на код: 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 И Аддлогин асинк отправляется нулевой вообще во всех случаях когда на месте TUser Нет Пользователь сайта и некоторые из его наследников.
В этом случае возникает вопрос.
Зачем использовать общий параметр, если код работает только с одним конкретным типом? Возможно, это особенность только этой функции, но она не очень очевидна.
7 место
На 7 строчке нашего рейтинга находится ошибка проекта.Пиранья .
Давайте сыграем в небольшую игру для самых внимательных.
Попробуйте найти ошибку в следующем фрагменте.
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 = "Комментировать сайт",
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.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 = "Новости";
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 = "Добро пожаловать в блог";
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 = "Добро пожаловать в новости";
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.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 = "Новости"; blog.EnableComments = true; // <= news.Published = DateTime.Now; await api.Pages.SaveAsync(news); .
}
}
Предупреждение анализатора: В3127 Были найдены два похожих фрагмента кода.
Возможно, это опечатка и вместо переменной «blog» следует использовать переменную «news».
CommentTests.cs 94 Есть два блока кода, схожих по структуре.
Анализатор сигнализирует о возможной опечатке во втором блоке строки блог.
EnableComments = правда .
Скорее всего разработчик допустил эту ошибку из-за копирования-вставки аналогичного блока кода, пришедшего ранее, и просто забыл изменить его в одном месте блог на Новости .
Очень интересно, что подобные ошибки допускают все программисты, независимо от опыта.
6 место
В проекте обнаружена следующая ошибка ОрчардЯдро .
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); .
}
Предупреждение анализатора: В3127 Были найдены два похожих фрагмента кода.
Возможно, это опечатка и вместо ConsumerSecret следует использовать переменную AccessTokenSecret. TwitterClientMessageHandler.cs 51 Анализатор предупреждает о двух одинаковых проверках.
На объекте настройки собственность называется ConsumerSecret , хотя, видимо, его следует использовать AccessTokenSecret , который существует на самом деле.
Из-за ошибки программиста меняется логика работы некоторых защит. Оповещения, указывающие на потенциальные дыры в безопасности, более ценны.
5 место
Вот мы и прошли первую половину вершины.Дальше будет еще интереснее.
На пятом месте снова предупреждение от Сквидекс .
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;
}
}
Предупреждение анализатора: В3001 Слева и справа от оператора «&&» находятся идентичные подвыражения «pw > 0».
FileTagAssetMetadataSource.cs 80 Анализатор подсказывает, что слева и справа от оператора есть два одинаковых подвыражения.
Скорее всего в если Должна быть проверка того, что ширина и высота больше 0. Вместо этого ширина проверяется дважды.
Следовательно, работа программы искажается, так как изображение не проверяется на правильность размеров.
4 место
Но это предупреждение по проекту Сервер BTCPay был выпущен благодаря добавлению новых аннотаций методов.
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
});
}
}
Предупреждение анализатора: В3010 Необходимо использовать возвращаемое значение функции BadRequest. ChangellyController.cs 72 PVS-Studio сообщает, что без использования возвращаемого значения вызов бессмысленен.
Анализатор не может расширить тело метода.
Плохой запрос , но благодаря аннотации теперь есть информация о том, что необходимо использовать возвращаемое значение.
Похоже, здесь это пропустили возвращаться .
Возможно, из-за этой оплошности ломается логика метода.
Рассчитать сумму .
Пропущенный возвращаться в Плохой запрос приводит как минимум к большему количеству итераций, а как максимум к зависанию программы.
3 место
Здесь мы находимся в тройке лучших предупреждений.На 3-е место ставим предупреждение, выданное анализатором для кода проекта.
Сквидекс .
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;
}
Предупреждение анализатора: В3029 Условные выражения операторов if, расположенных рядом друг с другом, идентичны.
Проверьте строки: 50, 57. AssetFolderDto.cs 50 Анализатор обнаружил два соседних если с теми же условиями.
Такое письмо явно вызывает подозрения.
Я думаю, все согласятся, что во втором условии вы ожидаете если видеть ресурсы.
CanDeleteAsset .
Такое свойство действительно имеется, и оно используется в аналогичном методе.
private static AssetDto CreateLinks(AssetDto response,
Resources resources)
{
.
if (resources.CanUpdateAsset) .
if (resources.CanUploadAsset) .
if (resources.CanDeleteAsset) .
.
}
2 место
В Сильвер на этот раз попадает ошибка, обнаруженная в проекте.Сквидекс .
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>();
}
Предупреждение анализатора: В3022 Выражение 'версия < 7' is always false. MigrationPath.cs 55 Скажу сразу, что вместо «.
» есть еще несколько галочек — я решил их сократить для улучшения читабельности.
Полный код метода можно найти здесь .
Анализатор показывает, что условие версия < 7 всегда ложь.
Ветвь еще никогда не будет выполнено, поскольку условие версия < 22 включает в себя то, что версия < 7 .
Подобные ошибки бывает трудно обнаружить при написании кода, особенно с таким большим количеством условных операторов, но они сразу становятся видимыми, когда вам на них указывают.
1-е место
При небольшом отставании от предыдущего места произошла интересная ошибка со стороны ОрчардЯдро .
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); } } .
}
Предупреждение анализатора: В3080 Возможное нулевое разыменование.
Рассмотрите возможность проверки «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); } } .
} .
}
Происходит проверка: если имущество метаданные.
CreateRouteValues равно нулевой , то для него вызывается метод Добавлять .
Естественно, это ошибка.
Примечательно, что подобных блоков кода немало.
Для понимания я привел один из них в качестве большого примера.
Во всех случаях, кроме последнего, перед этим идет проверка != ноль .
Скорее всего разработчик допустил опечатку при копировании кода.
Заключение
Проделанная работа явно окажет положительное влияние на анализ проектов с использованием ASP.NET Core. Аннотирование методов не только полезно для получения новых положительных результатов, но также помогает удалить ложные срабатывания.Мы рассмотрели лишь часть классов, которые сочли популярными на основании найденных нами проектов.
Кстати, в комментариях вы можете написать о некоторых местах в проектах ASP.NET Core, где мы не выдавали предупреждение или работали некорректно.
Особенно о местах, где аннотации могли бы помочь.
Этот топ еще раз доказывает, что статический анализ действительно позволяет находить интересные ошибки в проектах.
И это касается не только ASP-проектов, но и всего остального.
Как вы думаете, сможет ли PVS-Studio что-то найти в ваших проектах? Предлагаю вам посетить нас по адресу Веб-сайт и попробуйте :).
Теги: #C++ #.
NET #asp.net ядро
-
Карты Проксимити: Нужны Ли Они Нам?
19 Oct, 24 -
Смарт-Карты И Их Различные Типы
19 Oct, 24 -
Удаленный Работник – Мечта Работодателя
19 Oct, 24 -
Мультимедийный Центр На Arduino Uno
19 Oct, 24 -
Новая Жизнь Без Ie6
19 Oct, 24
