6 лет назад была написана первая статья про проверку проекта с помощью анализатора PVS-Studio для C#. Мы решили оглянуться назад и вновь вернуться туда, откуда всё начиналось – к анализу исходного кода Umbraco CMS.
Как уже понятно из названия, ранее мы уже посвятили проекту Umbraco две статьи:
Предлагаю вам ознакомиться с ними, чтобы понаблюдать, как меняются типы ошибок со временем.
Если вас заинтересовала эта статья, возможно, вы уже знаете, что такое Umbraco, но на всякий случай повторим. Umbraco – система управления контентом с открытым исходным кодом, которая улучшает опыт редактирования содержимого сайта. Сам исходный код можно найти на GitHub.
Напомним и про PVS-Studio. ;)
PVS-Studio – статический анализатор для улучшения качества, защищённости (SAST) и безопасности кода. Работает с языками C, C++, C#, Java под управлением Windows, Linux, macOS.
Для анализа была взята версия проекта от 12.11.2021 и скачана архивом с GitHub. Используемая версия PVS-Studio – 7.15.54288.
Среди срабатываний мы, как обычно, отобрали самые интересные. Какие-то из них указывают на явные ошибки, а какие-то – на код, который выглядит как минимум подозрительно. Но давайте перейдём ближе к делу и посмотрим, что же удалось обнаружить.
Issue 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: V3106 Possibly index is out of bound. The '2' index is pointing beyond 'm.Arguments' bound. ExpressionVisitorBase.cs 632
Думаю, что все когда-либо в жизни сталкивались с такими промахами. Разработчики проверяют, что m.Arguments.Count равно 2, а затем почти сразу же пытаются получить доступ к 3-ему элементу. Очевидно, что это приведёт к исключению IndexOutOfRangeException.
Подобные проблемы обнаруживались и в других проектах, и, как видите, Umbraco не стал исключением.
Issue 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: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless ObjectExtensions.cs 615
Не очень привлекательный фрагмент кода для тщательного вычитывания на code review, не правда ли?
Похоже, что в данном случае повезло и здесь просто избыточное условие. К этому выводу можно прийти при анализе используемых и доступных перегрузок метода XmlConvert.ToString. Однако везёт не всем и не всегда – порой за copy-paste могут скрываться неприметные ошибки.
Issue 3
public bool FlagOutOfDateModels
{
get => _flagOutOfDateModels;
set
{
if (!ModelsMode.IsAuto())
{
_flagOutOfDateModels = false;
}
_flagOutOfDateModels = value;
}
}Предупреждение PVS-Studio: V3008 The '_flagOutOfDateModels' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 54, 51. ModelsBuilderSettings.cs 54
Как мы видим, в set accessor присутствует проверка c присваиванием значения _flagOutOfDateModels, но сразу после неё для этого же поля устанавливается новое значение. Следовательно, блок if не несёт никакой практической пользы.
Issue 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: V3105 The 'routeEndpoints' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. RoutableDocumentFilter.cs 198
Диагностика V3105 предупреждает о возможности возникновения NullReferenceException. _endpointDataSource проверяется на null за счёт использования оператора '?.'. Если переменная _endpointDataSource всё-таки содержит значение null, то и в routeEndpoints будет null.
Странным здесь выглядит то, что обращение к routeEndpoints происходит уже без оператора '?.'. Как следствие, если routeEndpoints – null, при обращении по этой ссылке будет выброшено исключение типа NullReferenceException.
Issue 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: V3066 Possible incorrect order of arguments passed to 'RelationType' constructor. RelateOnCopyNotificationHandler.cs 32
В данном случае происходит вызов конструктора и передача ему аргументов. Взглянем на его сигнатуру:
public RelationType(string name,
string alias,
bool isBidrectional,
Guid? parentObjectType,
Guid? childObjectType)Похоже, что аргументы переданы не в том порядке. Аргумент RelateDocumentOnCopyAlias передаётся в параметр конструктора name, а аргумент RelateDocumentOnCopyName передаётся в параметр alias.
Issue 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: V3004 The 'then' statement is equivalent to the 'else' statement. UrlProviderExtensions.cs 274
Анализатор нашёл конструкцию, в которой ветви then и else совпадают. Получается так, что вне зависимости от значения проверяемого свойства выполняется один и тот же код. Скорее всего, разработчик копировал код и забыл исправить параметры метода.
Issue 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: V3137 The 'username' variable is assigned but is not used by the end of the function. MemberManager.cs 87
Было замечено вот такое срабатывание. Разработчик объявляет переменную username и присваивает ей значение. После этого username больше нигде не используется.
Возможно, разработчики просто не убрали это после рефакторинга кода. Однако есть некоторая (пусть и небольшая) вероятность, что какая-то логика не была реализована или где-то здесь запряталась хитрая ошибка.
Issue 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: V3008 The 'user' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 446, 444. UsersController.cs 446
В блоке else условного выражения происходит присваивание значения user. Сразу после завершения условного выражения опять идёт присваивание user. Следовательно, присвоенное ранее значение не используется и сразу же затирается. Должно ли было использоваться значение userResult.Value, и здесь пропущена какая-то логика, или это просто избыточный код – вопрос открытый. Но в любом случае какие-то подозрения на счёт этого кода сразу закрадываются.
Issue 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: V3022 Expression 'pageNumber > 0' is always true. EntityController.cs 625
Разработчик делает проверку, что pageNumber меньше или равен 0, если это так, то мы выходим. Следуя по коду далее, мы натыкаемся на проверку pageNumber больше 0. Естественно, это условие будет всегда истинным, а следовательно, будет безусловно вызван return. Код, который написан после этого оператора if (а его достаточно много), никогда не будет выполняться.
Здесь же, кстати, было выдано предупреждение про недостижимый код: V3142 Unreachable code detected. It is possible that an error is present. EntityController.cs 630
Issue 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: V3056 Consider reviewing the correctness of 'contentType1' item's usage. ConvertersTests.cs 115
Скорее всего, это copy-paste ошибка: вместо contentType2 при объявлении переменной cnt2 используется contentType1. Довольно странно, не так ли?
Было приятно принять эстафету проверки кода Umbraco. Кстати, судя по комментариям, встречающимся в коде, разработчики проекта взяли на своё вооружение ReSharper. Что, впрочем, не помешало PVS-Studio найти интересные ошибки. Вывод – совместное использование нескольких инструментов может дать больше профита. ;)
Если же у вас появилось желание проверить свой проект, можно взять пробный ключик у нас на сайте.
И не забывайте, что разовые проверки – лучше, чем их отсутствие, но максимальная польза от статического анализа достигается при регулярном использовании и внедрении в процессы.