За 2021 год разработчики PVS-Studio написали ряд статей, в которых разбирали странности, найденные анализатором в Open Source проектах. Год подходит к концу, а значит, пришло время представить традиционный разбор 10 самых интересных ошибок. Приятного просмотра!
Как и в статье 2020 года, срабатывания отбирались и распределялись по местам по следующим принципам:
Стоит признать, что в этом году статей о проверках C# проектов было немного и срабатывания в получившемся списке часто связаны с одними и теми же проектами. Как-то так вышло, что больше всего предупреждений я взял из статей про DNN и PeachPie.
С другой стороны, найденные в этом году ошибки совсем не похожи друг на друга – все предупреждения из списка выданы разными диагностиками!
С тяжёлым сердцем я вычёркивал некоторые срабатывания, которые были очень даже хороши, но менее интересны, чем другие. Иной раз приходилось убирать что-то исключительно для разнообразия топа. Поэтому, если вам нравятся обзоры предупреждений анализатора, обязательно посмотрите другие статьи. Кто знает, может, вас впечатлит что-то такое, о чём я не написал. Скидывайте свои топы в комментарии – мне будет очень интересно почитать :).
Открывает сегодняшний список срабатывание из статьи о проверке PeachPie:
using System_DateTime = System.DateTime;
internal static System_DateTime MakeDateTime(....) { .... }
public static long mktime(....)
{
var zone = PhpTimeZone.GetCurrentTimeZone(ctx);
var local = MakeDateTime(hour, minute, second, month, day, year);
switch (daylightSaving)
{
case -1:
if (zone.IsDaylightSavingTime(local))
local.AddHours(-1); // <=
break;
case 0:
break;
case 1:
local.AddHours(-1); // <=
break;
default:
PhpException.ArgumentValueNotSupported("daylightSaving", daylightSaving);
break;
}
return DateTimeUtils.UtcToUnixTimeStamp(TimeZoneInfo.ConvertTime(local,
....));
}Предупреждения PVS-Studio:
Эти срабатывания, по сути, говорят об одном и том же, поэтому я и решил их объединить.
Анализатор сообщает, что результаты вызовов должны куда-нибудь записываться – в противном случае они попросту не имеют никакого смысла. Дело в том, что методы вроде AddHours не меняют исходный объект – вместо этого возвращается новый, отличающийся от исходного соответствующим образом. Сложно сказать, насколько критична эта оплошность, но очевидно, что код работает некорректно.
Подобные ошибки чаще связаны со строками, но порой их можно встретить и при работе с другими типами. Все они происходят из-за неверного понимания особенностей работы "изменяющих" методов.
На 9-ое место попало срабатывание из статьи про проверку Ryujinx:
public uint this[int index]
{
get
{
if (index == 0)
{
return element0;
}
else if (index == 1)
{
return element1;
}
else if (index == 2)
{
return element2;
}
else if (index == 2) // <=
{
return element3;
}
throw new IndexOutOfRangeException();
}
}Предупреждение анализатора: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 26, 30. ZbcSetTableArguments.cs 26
Очевидно, тут всё будет нормально лишь до тех пор, пока кто-нибудь не захочет получить элемент с индексом '3'. Ну а если захочет, то будет выброшено исключение. Всё бы ничего, но зачем же тогда нужен никогда не выполняющийся блок с element3?
Удивительно, но ситуации, вызванные опечатками с использованием чисел 0, 1, 2 – частые гости в программировании. По этому поводу даже была написана целая статья – всем рекомендую к прочтению. Ну а мы идём дальше.
Следующее предупреждение я взял из ранее упомянутой статьи про PeachPie. Примечательно оно тем, что код, на мой взгляд, выглядит совершенно нормально и вообще не вызывает никаких подозрений:
public static bool mail(....)
{
// to and subject cannot contain newlines, replace with spaces
to = (to != null) ? to.Replace("\r\n", " ").Replace('\n', ' ') : "";
subject = (subject != null) ? subject.Replace("\r\n", " ").Replace('\n', ' ')
: "";
Debug.WriteLine("MAILER",
"mail('{0}','{1}','{2}','{3}')",
to,
subject,
message,
additional_headers);
var config = ctx.Configuration.Core;
....
}Казалось бы, а что же тут не так? Вроде бы всё выглядит неплохо. Производятся всякие присваивания, а затем вызывается перегрузка Debug.WriteLine, которая в качестве первого аргумента принимает... Минуточку! А каким по порядку аргументом нужно передавать формат?
Что ж, давайте взглянем на объявление Debug.WriteLine:
public static void WriteLine(string format, params object[] args);В соответствии с сигнатурой строка формата должна передаваться первым аргументом. В коде же первым аргументом является "MAILER", а фактический формат попадает уже в массив args, который в этом случае вообще ни на что не повлияет.
PVS-Studio в свою очередь сообщает, что все аргументы форматирования будут проигнорированы: V3025: Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: 1st, 2nd, 3rd, 4th, 5th. Mail.cs 25
Получается, что выводиться будет просто "MAILER" без какой-либо другой полезной информации, которую явно хотелось бы видеть :(.
На 7-ом месте перед нами вновь срабатывание из PeachPie.
Довольно часто разработчики упускают проверки на null. Особенно интересна ситуация, когда в одном месте функции переменную проверили, а в другом (где она всё также может быть null) – забыли или не посчитали нужным. И тут остаётся лишь гадать, была ли проверка лишней или напротив – кое-где её не хватает. Проверки на null не всегда предполагают использование операторов сравнения: например, в коде ниже разработчик использовал null-conditional оператор:
public static string get_parent_class(....)
{
if (caller.Equals(default))
{
return null;
}
var tinfo = Type.GetTypeFromHandle(caller)?.GetPhpTypeInfo();
return tinfo.BaseType?.Name;
}Предупреждение V3105: The 'tinfo' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. Objects.cs 189
По мнению разработчика, вызов Type.GetTypeFromHandle(caller) может вернуть null – оттого он и использовал "?." для вызова GetPhpTypeInfo. Судя по документации, это действительно возможно.
Да, "?." спасает от одного исключения. Если вызов GetTypeFromHandle действительно вернёт null, то в переменную tinfo также будет записан null. Однако при попытке обращения к свойству BaseType будет выброшено другое исключение. Просматривая код, я решил, что в последней строке не хватает ещё одного "?": return tinfo?.BaseType?.Name;
Тем не менее, лучше всего исправить ошибку могут только сами разработчики проекта. Именно это они и сделали после того, как мы отписали им о найденных проблемах. Вместо дополнительной проверки на null они решили явно выбрасывать исключение в тех случаях, когда GetTypeFromHandle возвращает null:
public static string get_parent_class(....)
{
if (caller.Equals(default))
{
return null;
}
// cannot be null; caller is either default or an invalid handle
var t = Type.GetTypeFromHandle(caller)
?? throw new ArgumentException("", nameof(caller));
var tinfo = t.GetPhpTypeInfo();
return tinfo.BaseType?.Name;
}Для статьи код пришлось отформатировать. При необходимости вы можете найти данный метод по ссылке.
Порой кажется, что время будто замедляется. Иной раз чувствуешь, что прошла целая неделя, а на самом деле – лишь один день. Что ж, на 6-ом месте у нас срабатывание из статьи про DotNetNuke, выданное на код, в котором за неделю проходит всего один день:
private static DateTime CalculateTime(int lapse, string measurement)
{
var nextTime = new DateTime();
switch (measurement)
{
case "s":
nextTime = DateTime.Now.AddSeconds(lapse);
break;
case "m":
nextTime = DateTime.Now.AddMinutes(lapse);
break;
case "h":
nextTime = DateTime.Now.AddHours(lapse);
break;
case "d":
nextTime = DateTime.Now.AddDays(lapse); // <=
break;
case "w":
nextTime = DateTime.Now.AddDays(lapse); // <=
break;
case "mo":
nextTime = DateTime.Now.AddMonths(lapse);
break;
case "y":
nextTime = DateTime.Now.AddYears(lapse);
break;
}
return nextTime;
}Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. DotNetNuke.Tests.Core PropertyAccessTests.cs 118
Очевидно, данная функция должна возвращать DateTime, соответствующий некоторому моменту времени после текущего. Но почему-то буква 'w', которая как бы намекает на слово 'week', обрабатывается так же, как и 'd'. Пытаясь получить дату через неделю от текущего момента, мы получим завтрашний день!
Стоит отметить, что здесь, во всяком случае, нет ошибки с изменением иммутабельных объектов. И всё же очень странно, что код ветвей для "d" и "w" повторяется. Конечно, стандартного метода AddWeeks в типе DateTime нет, но никто не мешает добавлять 7 дней :).
На 5-ое место я поставил ещё одно из моих любимых срабатываний из статьи про проверку PeachPie. Предлагаю вам для начала внимательно взглянуть на этот фрагмент и продумать, есть ли тут ошибка.
public static bool IsAutoloadDeprecated(Version langVersion)
{
// >= 7.2
return langVersion != null
&& langVersion.Major > 7
|| (langVersion.Major == 7 && langVersion.Minor >= 2);
}В чём же тут проблема?
Наверняка вам не составило труда найти в том фрагменте ошибку. Это действительно легко сделать, если знать, что она там есть :). Должен признаться, что я постарался вас запутать, немного изменив исходное форматирование. На самом деле логическая конструкция была написана в одну строку.
Давайте теперь взглянем на версию, отформатированную в соответствии с приоритетами операторов:
public static bool IsAutoloadDeprecated(Version langVersion)
{
// >= 7.2
return langVersion != null && langVersion.Major > 7
|| (langVersion.Major == 7 && langVersion.Minor >= 2);
}Предупреждение V3080: Possible null dereference. Consider inspecting 'langVersion'. AnalysisFacts.cs 20
Код проверяет, что переданный параметр langVersion не равен null. Стало быть, разработчик предполагал, что null действительно может быть передан при вызове. Спасает ли проверка?
Увы, если переменная langVersion будет равна null, то значение первой части выражения будет равно false. При вычислении же второй части будет выброшено исключение.
Учитывая комментарий, можно легко понять, что здесь либо перепутали приоритеты операторов, либо попросту поставили скобку неправильно. Кстати, этой ошибки, как и других (хочется верить), в проекте больше нет – мы написали разработчикам о найденных проблемах, и они оперативно внесли исправления. Новую версию функции IsAutoloadDeprecated можно увидеть здесь.
Мы почти подобрались к финалистам, но перед ними – 4-ое место, на которое я поставил срабатывание из последней статьи о проекте Umbraco. Что же тут у нас?
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.
Возникает логичный вопрос – а какое значение можно передать в pageNumber, чтобы оба условия pageNumber <= 0 и pageNumber > 0 оказались ложными?
Очевидно, нет такого значения. Если проверка pageNumber <= 0 выдала false, то условие pageNumber > 0 всегда истинно. Страшно ли это? Давайте отдельно взглянем на код, который идёт после always-true проверки:
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;Так как представленная в начале фрагмента проверка всегда выдаёт true, то в любом случае будет произведён выход из метода. А что же с кодом ниже? Вроде как он содержит кучу осмысленных операций, но ни одна из них никогда не будет выполнена!
Всё это выглядит очень подозрительно. Логично, что если pageNumber меньше или равен 0, то возвращается некоторый результат по умолчанию – NotFound(). Но при этом если параметр больше 0, то возвращается... Да на самом деле тоже что-то такое, что похоже на результат по умолчанию – new PagedResult<EntityBasic>(0, 0, 0). А как же тогда выдать какой-нибудь нормальный результат? Неясно :(.
Итак, мы подобрались к финалистам. На 3-ем месте находится срабатывание диагностики V3122, которая долгое время не обнаруживала ошибок в Open Source проектах. И вот, наконец, в 2021 году мы проверили DotNetNuke и нашли целых 2 срабатывания V3122!
Итак, представляю вашему вниманию 3-е место:
public static string LocalResourceDirectory
{
get
{
return "App_LocalResources";
}
}
private static bool HasLocalResources(string path)
{
var folderInfo = new DirectoryInfo(path);
if (path.ToLowerInvariant().EndsWith(Localization.LocalResourceDirectory))
{
return true;
}
....
}Предупреждение PVS-Studio: V3122 The 'path.ToLowerInvariant()' lowercase string is compared with the 'Localization.LocalResourceDirectory' mixed case string. Dnn.PersonaBar.Extensions LanguagesController.cs 644
Сначала производится получение значения path в нижнем регистре, а затем проверяется, что оно оканчивается на строку, заведомо содержащую символы в верхнем регистре – "App_LocalResources" (литерал, возвращаемый из свойства LocalResourceDirectory). Очевидно, такая проверка всегда возвращает false, и выглядит всё это очень подозрительно.
Это срабатывание в очередной раз напоминает мне, что, сколько бы ошибок мы ни смотрели, всегда может найтись что-то, способное удивить. Идём дальше :).
На втором месте находится отличное срабатывание из статьи о проекте ILSpy, написанной в начале года:
private static void WriteSimpleValue(ITextOutput output,
object value, string typeName)
{
switch (typeName)
{
case "string":
output.Write( "'"
+ DisassemblerHelpers
.EscapeString(value.ToString())
.Replace("'", "\'") // <=
+ "'");
break;
case "type":
....
}
....
}V3038 The '"'"' argument was passed to 'Replace' method several times. It is possible that other argument should be passed instead. ICSharpCode.Decompiler ReflectionDisassembler.cs 772
Кажется, что автор хотел осуществить замену всех вхождений символа одинарной кавычки на строку, состоящую из двух символов: символа обратной косой черты и символа одинарной кавычки. Увы, но из-за особенностей работы с escape-последовательностями второй аргумент фактически окажется просто одинарной кавычкой, а значит, никакой замены тут не произойдёт.
На ум приходят 2 идеи:
Итак, вот мы и добрались до срабатывания, которое, на мой взгляд, является самым необычным и интересным за весь 2021 год. Оно из упомянутой уже много раз статьи про проверку DotNetNuke.
Самое интересное – срабатывание крайне простое, но человеческий глаз попросту не улавливает такие ошибки без подсказок инструментов, производящих статический анализ. Громкие слова? Ну что же, попробуйте отыскать тут проблему (если она, конечно, вообще есть):
private void ParseTemplateInternal(...., string templatePath, ....)
{
....
string path = Path.Combine(templatePath, "admin.template");
if (!File.Exists(path))
{
// if the template is a merged copy of a localized templte the
// admin.template may be one director up
path = Path.Combine(templatePath, "..\admin.template");
}
....
}Ну, как успехи? Не удивлюсь, если вы найдёте ошибку, но признайтесь – если не знать, что она там есть, то никогда её там не увидишь. А если не нашли – что ж, тоже ничего удивительного. Не так-то просто заметить, что в комментарии к коду вместо 'template' написано 'templte' :).
Да шучу я, конечно же, там есть и настоящая ошибка, влияющая на работу программы. Давайте взглянем на код ещё раз:
private void ParseTemplateInternal(...., string templatePath, ....)
{
....
string path = Path.Combine(templatePath, "admin.template");
if (!File.Exists(path))
{
// if the template is a merged copy of a localized templte the
// admin.template may be one director up
path = Path.Combine(templatePath, "..\admin.template");
}
....
}Предупреждение PVS-Studio: V3057 The 'Combine' function is expected to receive a valid path string. Inspect the second argument. DotNetNuke.Library PortalController.cs 3538
Здесь мы имеем две операции конструирования пути (вызов Path.Combine). С первой всё нормально, а вот вторая интереснее. Видно, что во втором случае хотели брать файл 'admin.template' не из директории templatePath, а из родительской. Но вот незадача! После того как добавили '..\', путь перестал быть валидным, так как образовалась escape-последовательность: ..\admin.template.
Напоминает предыдущее срабатывание, не правда ли? И всё же, оно совсем другое. Тем не менее, решение тут то же самое – добавляем перед строкой '@' или ставим дополнительный '\'.
Ну, раз первый элемент в коллекции имеет индекс 0, то и в топе должно быть нулевое место!
Конечно, ошибка на этом месте особенная, выходящая за рамки привычного топа. И всё же она стоит упоминания, ведь она обнаружилась в известном всем нам приложении – Visual Studio 2022. А причём тут тогда статический анализ? Что ж, давайте разбираться по порядку.
Проблему обнаружил мой коллега Сергей Васильев и рассказал о ней в статье "Как Visual Studio 2022 съела 100 Гб памяти и при чём здесь XML бомбы?". Здесь же я лишь коротко пройду по основным моментам.
Оказалось, что в Visual Studio 2022 Preview 3.1 можно добавить в проект определённый XML-файл и IDE начнёт страдать, а вместе с ней страдать будет и всё остальное. Пример подобного файла представлен ниже:
<?xml version="1.0"?>
<!DOCTYPE lolz [
<!ENTITY lol "lol">
<!ELEMENT lolz (#PCDATA)>
<!ENTITY lol1 "&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;&lol;">
<!ENTITY lol2 "&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;&lol1;">
<!ENTITY lol3 "&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;&lol2;">
<!ENTITY lol4 "&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;&lol3;">
<!ENTITY lol5 "&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;&lol4;">
<!ENTITY lol6 "&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;&lol5;">
<!ENTITY lol7 "&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;&lol6;">
<!ENTITY lol8 "&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;&lol7;">
<!ENTITY lol9 "&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;&lol8;">
<!ENTITY lol10 "&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;&lol9;">
<!ENTITY lol11
"&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;&lol10;">
<!ENTITY lol12
"&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;&lol11;">
<!ENTITY lol13
"&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;&lol12;">
<!ENTITY lol14
"&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;&lol13;">
<!ENTITY lol15
"&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;&lol14;">
]>
<lolz>&lol15;</lolz>Visual Studio, по сути, оказалась уязвима к XEE. Пытаясь раскрывать все эти lol-сущности, IDE зависала и начинала пожирать просто гигантские объёмы оперативной памяти. Очевидно, со временем она попросту забирала вообще всё, что только возможно :(.
Проблемы подобного рода возникают из-за использования небезопасно сконфигурированного парсера XML. В частности, в таком парсере включена обработка DTD и отсутствуют какие-либо лимиты для сущностей. Таким образом, не стоит считывать таким парсером файлы, пришедшие непонятно откуда, так как это может привести к DoS.
Отлавливать же такие проблемы в коде позволяет статический анализ. Кстати, в PVS-Studio недавно появилась диагностика для обнаружения потенциальных XEE – V5615.
Про обнаруженную в Visual Studio ошибку мы сделали баг-репорт, и в новых версиях проблема уже не возникает. Отличная работа, Microsoft! :)
В 2021 году мы, к сожалению, написали не так уж много обзоров предупреждений на реальных проектах. С другой стороны, было написано большое количество других статей, так или иначе связанных с C#. Ссылки на некоторые из них можно найти после заключения.
Выбрать же интересные срабатывания для этого топа оказалось крайне просто. Куда сложнее было выбрать 10 лучших, так как классных срабатываний оказалось гораздо больше.
Распределение мест тоже является той ещё задачкой – здесь всё очень субъективно, поэтому не относитесь к номерам позиций в топе слишком серьёзно :). Так или иначе, все отобранные предупреждения выглядят довольно серьёзно и ещё раз напоминают мне о том, что мы не зря делаем своё дело.
А уверены ли вы, что в коде вашего проекта нет подобных проблем? Что никакие скрытые угрозы не притаились между строк? Пожалуй, в большом проекте никогда не будет такой уверенности. Но даже из этой статьи очевидно, что многие мелкие (и не очень) ошибки могут быть обнаружены с помощью анализатора. Поэтому предлагаю вам попробовать PVS-Studio на своих проектах.
Ну а у меня на этом всё, счастливого вам Нового Года и до новых встреч!
Ниже я собрал несколько статей, которые можно почитать длинными зимними вечерами, если вдруг вы что-то из этого пропустили :).