6 лет назад была написана первая статья о проверке проекта с помощью анализатора PVS-Studio для C#.
Мы решили оглянуться назад и снова вернуться к тому, с чего всё началось — к анализу исходного кода Umbraco CMS. 
Введение
Как следует из названия, ранее мы посвятили проекту Umbraco две статьи: Предлагаю вам взглянуть на них, чтобы увидеть, как со временем меняются типы ошибок.Если вам интересна эта статья, возможно, вы уже знаете, что такое Умбрако, но на всякий случай повторим.
Umbraco — это система управления контентом с открытым исходным кодом, которая упрощает редактирование контента веб-сайта.
Сам исходный код можно найти по адресу GitHub .
Напомним также о PVS-Studio. ;) PVS-Студия — статический анализатор для повышения качества, защищенности (SAST) и защищенности кода.
Работает с языками C, C++, C#, Java под управлением Windows, Linux, macOS. Для анализа была взята версия проекта от 12 ноября 2021 года и скачана в виде архива с сайта GitHub .
Используемая версия PVS-Studio — 7.15.54288. Среди плюсов мы, как обычно, выбрали самые интересные.
Некоторые из них указывают на явные ошибки, а некоторые указывают на код, который выглядит как минимум подозрительно.
Но давайте перейдем к делу и посмотрим, что мы нашли.
Что с предупреждениями?
Выпуск 1 Сможете ли вы найти ошибку в этом примере?Хорошо-хорошо, давайте посмотрим на сокращенную версию кода.protected virtual string VisitMethodCall(MethodCallExpression m) { .case "SqlText": if (m.Method.DeclaringType != typeof(SqlExtensionsStatics)) goto default; if (m.Arguments.Count == 2) { var n1 = Visit(m.Arguments[0]); var f = m.Arguments[2]; if (!(f is Expression<Func<string, string>> fl)) throw new NotSupportedException("Expression is not a proper lambda."); var ff = fl.Compile(); return ff(n1); } else if (m.Arguments.Count == 3) { var n1 = Visit(m.Arguments[0]); var n2 = Visit(m.Arguments[1]); var f = m.Arguments[2]; if (!(f is Expression<Func<string, string, string>> fl)) throw new NotSupportedException("Expression is not a proper lambda."); var ff = fl.Compile(); return ff(n1, n2); } else if (m.Arguments.Count == 4) { var n1 = Visit(m.Arguments[0]); var n2 = Visit(m.Arguments[1]); var n3 = Visit(m.Arguments[3]); var f = m.Arguments[3]; if (!(f is Expression<Func<string, string, string, string>> fl)) throw new NotSupportedException("Expression is not a proper lambda."); var ff = fl.Compile(); return ff(n1, n2, n3); } else throw new NotSupportedException("Expression is not a proper lambda."); .
}
protected virtual string VisitMethodCall(MethodCallExpression m)
{
.
case "SqlText": .
if (m.Arguments.Count == 2) { var n1 = Visit(m.Arguments[0]); var f = m.Arguments[2]; .
}
}
Предупреждение PVS-Studio: В3106 Возможно, индекс вышел за пределы.
Индекс «2» указывает за границу «m.Arguments».
ExpressionVisitorBase.cs 632 Я думаю, каждый хоть раз в жизни сталкивался с такими ошибками.
Разработчики это проверяют m.Arguments.Count равно 2, а затем почти сразу пытается получить доступ к третьему элементу.
Очевидно, это вызовет исключение IndexOutOfRangeException .
Подобные проблемы встречались и в других проектах, и, как видите, Umbraco не стал исключением.
Выпуск 2 Предлагаю вам проверить свою внимательность и найти проблему самостоятельно.
Сразу после кода будет картинка, а после нее – правильный ответ. public static string ToXmlString(this object value, Type type)
{
if (value == null) return string.Empty;
if (type == typeof(string))
return (value.ToString().
IsNullOrWhiteSpace() ? "" : value.ToString()); if (type == typeof(bool)) return XmlConvert.ToString((bool)value); if (type == typeof(byte)) return XmlConvert.ToString((byte)value); if (type == typeof(char)) return XmlConvert.ToString((char)value); if (type == typeof(DateTime)) return XmlConvert.ToString((DateTime)value, XmlDateTimeSerializationMode.Unspecified); if (type == typeof(DateTimeOffset)) return XmlConvert.ToString((DateTimeOffset)value); if (type == typeof(decimal)) return XmlConvert.ToString((decimal)value); if (type == typeof(double)) return XmlConvert.ToString((double)value); if (type == typeof(float)) return XmlConvert.ToString((float)value); if (type == typeof(Guid)) return XmlConvert.ToString((Guid)value); if (type == typeof(int)) return XmlConvert.ToString((int)value); if (type == typeof(long)) return XmlConvert.ToString((long)value); if (type == typeof(sbyte)) return XmlConvert.ToString((sbyte)value); if (type == typeof(short)) return XmlConvert.ToString((short)value); if (type == typeof(TimeSpan)) return XmlConvert.ToString((TimeSpan)value); if (type == typeof(bool)) return XmlConvert.ToString((bool)value); if (type == typeof(uint)) return XmlConvert.ToString((uint)value); if (type == typeof(ulong)) return XmlConvert.ToString((ulong)value); if (type == typeof(ushort)) return XmlConvert.ToString((ushort)value); .
}
Если вы быстро справились с этим заданием, то у вас орлиное зрение! Давайте посмотрим на сокращенную версию метода: public static string ToXmlString(this object value, Type type)
{
.
if (type == typeof(bool)) return XmlConvert.ToString((bool)value); .
if (type == typeof(bool)) return XmlConvert.ToString((bool)value); .
}
Предупреждение PVS-Studio: В3021 Есть два оператора if с одинаковыми условными выражениями.
Первый оператор if содержит возврат метода.
Это означает, что второй оператор if бессмыслен.
ObjectExtensions.cs 615 Не очень привлекательный фрагмент кода, который стоит внимательно читать при обзоре кода, не так ли? Похоже, в данном случае нам повезло и это просто избыточное условие.
К такому выводу можно прийти, проанализировав используемые и доступные перегрузки методов.
XmlConvert.ToString .
Однако не всем и не всегда везет – иногда удается скрытие незаметных ошибок .
Выпуск 3 public bool FlagOutOfDateModels
{
get => _flagOutOfDateModels;
set
{
if (!ModelsMode.IsAuto())
{
_flagOutOfDateModels = false;
}
_flagOutOfDateModels = value;
}
}
Предупреждение PVS-Studio: В3008 Переменной '_flagOutOfDateModels' присваиваются значения дважды подряд. Возможно, это ошибка.
Проверьте строки: 54, 51. ModelsBuilderSettings.cs 54 Как мы видим, аксессор set содержит проверку с присвоением значения.
_flagOutOfDateModels , но сразу после него для того же поля устанавливается новое значение.
Поэтому блок если не имеет никакой практической пользы.
Выпуск 4 private bool MatchesEndpoint(string absPath)
{
IEnumerable<RouteEndpoint> routeEndpoints = _endpointDataSource
?.
Endpoints .
OfType<RouteEndpoint>() .
Where(x => { .
}); var routeValues = new RouteValueDictionary(); RouteEndpoint matchedEndpoint = routeEndpoints .
Where(e => new TemplateMatcher( TemplateParser.Parse(e.RoutePattern.RawText), new RouteValueDictionary()) .
TryMatch(absPath, routeValues)) .
OrderBy(c => c.Order) .
FirstOrDefault();
return matchedEndpoint != null;
}
Предупреждение PVS-Studio: В3105 ПеременнаяrouteEndpoints использовалась после того, как она была назначена с помощью условного оператора с нулевым значением.
Возможно исключение NullReferenceException. МаршрутаблеДокументФилтер.
cs 198 Диагностика В3105 предупреждает о возможности NullReferenceException .
_endpointDataSource проверено на нулевой с помощью знака '?.
' оператор.
Если переменная _endpointDataSource все еще содержит смысл нулевой , затем в Конечные точки маршрута воля нулевой.
Странным здесь кажется то, что обращение к Конечные точки маршрута встречается без знака '?.
' оператор.
Как следствие, если Конечные точки маршрута – нулевой , при доступе к этой ссылке возникает исключение типа NullReferenceException .
Выпуск 5 public void Handle(ContentCopiedNotification notification)
{
.
if (relationType == null) { relationType = new RelationType( Constants.Conventions.RelationTypes.RelateDocumentOnCopyAlias, Constants.Conventions.RelationTypes.RelateDocumentOnCopyName, true, Constants.ObjectTypes.Document, Constants.ObjectTypes.Document); _relationService.Save(relationType); } .
}
Предупреждение PVS-Studio: В3066 Возможен неверный порядок аргументов, передаваемых в конструктор RelationType. RelateOnCopyNotificationHandler.cs 32 В этом случае вызывается конструктор и ему передаются аргументы.
Давайте посмотрим на его подпись: public RelationType(string name,
string alias,
bool isBidrectional,
Guid? parentObjectType,
Guid? childObjectType)
Похоже, аргументы были переданы в неправильном порядке.
Аргумент СвязатьДокументОнкопировать Псевдоним передается в параметр конструктора имя и аргумент СвязатьДокументОнкопировать Имя передано в параметр псевдоним .
Выпуск 6 private static async Task<Attempt<UrlInfo>> DetectCollisionAsync(.
) { .
if (pcr.IgnorePublishedContentCollisions)
{
logger.LogDebug(logMsg, url, uri, culture);
}
else
{
logger.LogDebug(logMsg, url, uri, culture);
}
}
Предупреждение PVS-Studio: В3004 Оператор then эквивалентен оператору else. UrlProviderExtensions.cs 274 Анализатор нашел структуру, в которой ветки затем И еще совпадают. Получается, что независимо от значения проверяемого свойства выполняется один и тот же код. Скорее всего, разработчик скопировал код и забыл исправить параметры метода.
Выпуск 7 public async Task<bool> IsMemberAuthorizedAsync(.
) { .
if (IsLoggedIn() == false) { allowAction = false; } else { string username; .
username = currentMember.UserName; IList<string> allowTypesList = allowTypes as IList<string> ?? allowTypes.ToList(); if (allowTypesList.Any(allowType => allowType != string.Empty)) { allowAction = allowTypesList.Select(x => x.ToLowerInvariant()) .
Contains(currentMember .
MemberTypeAlias .
ToLowerInvariant()); } if (allowAction && allowMembers.Any()) { allowAction = allowMembers.Contains(memberId); } .
}
return allowAction;
}
Предупреждение PVS-Studio: В3137 Переменная username назначается, но не используется в конце функции.
MemberManager.cs 87 Вот что было замечено.
Разработчик объявляет переменную имя пользователя и присваивает ему значение.
После этого имя пользователя больше нигде не используется.
Возможно, разработчики просто не убрали это после рефакторинга кода.
Однако существует некоторая (хотя и небольшая) вероятность того, что какая-то логика не была реализована или где-то спрятана хитрая ошибка.
Выпуск 8 public async Task<ActionResult<UserDisplay>> PostInviteUser(UserInvite userSave)
{
if (_securitySettings.UsernameIsEmail)
{
userSave.Username = userSave.Email;
}
else
{
var userResult = CheckUniqueUsername(userSave.Username, u =>
u.LastLoginDate != default
|| u.EmailConfirmedDate.HasValue);
if (!(userResult.Result is null))
{
return userResult.Result;
}
user = userResult.Value;
}
user = CheckUniqueEmail(userSave.Email, u => u.LastLoginDate != default ||
u.EmailConfirmedDate.HasValue);
.
}
Предупреждение PVS-Studio: В3008 Переменной user присваиваются значения дважды подряд. Возможно, это ошибка.
Проверить строки: 446, 444. UsersController.cs 446 В блоке еще условному выражению присваивается значение пользователь.
Сразу после завершения условного выражения присваивание происходит снова.
пользователь .
Следовательно, ранее присвоенное значение не используется и немедленно перезаписывается.
Следует ли использовать это значение? userResult.Значение, и не хватает ли здесь какой-то логики или это просто лишний код — вопрос открытый.
Но в любом случае сразу закрадываются некоторые подозрения по поводу этого кода.
Выпуск 9 public ActionResult<PagedResult<EntityBasic>> GetPagedChildren(.
int pageNumber, .
) { if (pageNumber <= 0) { return NotFound(); } .
if (objectType.HasValue) { if (id == Constants.System.Root && startNodes.Length > 0 && startNodes.Contains(Constants.System.Root) == false && !ignoreUserStartNodes) { if (pageNumber > 0) // <= { return new PagedResult<EntityBasic>(0, 0, 0); } IEntitySlim[] nodes = _entityService.GetAll(objectType.Value, startNodes).
ToArray();
if (nodes.Length == 0)
{
return new PagedResult<EntityBasic>(0, 0, 0);
}
if (pageSize < nodes.Length)
{
pageSize = nodes.Length; // bah
}
var pr = new PagedResult<EntityBasic>(nodes.Length, pageNumber, pageSize)
{
Items = nodes.Select(_umbracoMapper.Map<EntityBasic>)
};
return pr;
}
}
}
Предупреждение PVS-Studio: В3022 Выражение «pageNumber > 0» всегда истинно.
EntityController.cs 625 Разработчик это проверяет номер страницы меньше или равно 0, если да, то выходим.
Следуя по коду дальше, натыкаемся на проверку номер страницы более 0 .
Естественно, что это условие всегда будет истинным, а потому будет безусловно вызвано возвращаться .
Код, написанный после этого оператора если (и его довольно много) , никогда не будет выполнено.
Здесь, кстати, вышло предупреждение о недостижимом коде: В3142 Обнаружен недоступный код. Вполне возможно, что ошибка присутствует. EntityController.cs 630 Выпуск 10 И тут у нас ошибка в тесте.
Вы можете подумать, что это не так уж и важно, но именно тесты дают вам некоторую уверенность в том, что ваш код работает правильно.
Если в них есть ошибка, можете ли вы быть уверены, что программа работает корректно? И в такие моменты приходит на помощь статический анализ.
Public void SimpleConverter3Test()
{
.
IpublishedContentType contentType1 = contentTypeFactory.CreateContentType(Guid.NewGuid(), 1002, "content1", t => CreatePropertyTypes(t, 1)); IpublishedContentType contentType2 = contentTypeFactory.CreateContentType(Guid.NewGuid(), 1003, "content2", t => CreatePropertyTypes(t, 2)); .
var cnt1 = new InternalPublishedContent(contentType1) // <=
{
Id = 1003,
Properties = new[]
{
new InternalPublishedProperty {Alias = "prop1",
SolidHasValue = true, SolidValue = "val1"}
}
};
var cnt2 = new InternalPublishedContent(contentType1) // <=
{
Id = 1004,
Properties = new[]
{
new InternalPublishedProperty {Alias = "prop2",
SolidHasValue = true, SolidValue = "1003"}
}
};
}
Предупреждение PVS-Studio: В3056 Рассмотрите возможность проверки правильности использования элемента «contentType1».
КонвертерыTests.cs 115 Скорее всего это ошибка копирования-вставки: вместо контенттип2 при объявлении переменной cnt2 использовал тип контента1. Довольно странно, не так ли?
Заключение
Было очень приятно принять эстафету проверки кода Umbraco. Кстати, судя по найденным в коде комментариям, разработчики проекта взяли на вооружение ReSharper. Это, однако, не помешало PVS-Studio найти интересные ошибки.Вывод – совместное использование нескольких инструментов может дать вам больше прибыли.
;) Если вы хотите проверить свой проект, вы можете получить у нас пробный ключ.
Веб-сайт .
И не забывайте, что разовые проверки лучше, чем отсутствие проверок, но максимальная польза от статического анализа достигается за счет регулярного использования и внедрения в процессы.
Если вы хотите поделиться этой статьей с англоязычной аудиторией, воспользуйтесь ссылкой для перевода: Артем Ровенский.
PVS-Studio в третий раз проверяет код Umbraco .
Теги: #C++ #.
NET #CMS #umbraco
-
«Нас Учили Говорить «Спасибо»…»
19 Oct, 24 -
Мой Любимый Редактор Высшего Уровня
19 Oct, 24 -
Ex.ua Остался Один!
19 Oct, 24 -
Битрикс24 — Crm Или Больше?
19 Oct, 24 -
Конкурент Yahoo Answers Готов К Бою
19 Oct, 24
