ТОП-10 ошибок, найденных в C#-проектах за 2020 год
Наконец-то столь нелёгкий 2020 подходит к концу, а это значит, что пришло время подвести итоги! За этот год командой PVS-Studio было написано немало статей, в которых разбирались разнообразные ошибки, найденные анализатором в open-source проектах. Самые же интересные из них вы можете увидеть прямо здесь, в ТОП-е ошибок, обнаруженных в C#-проектах за 2020 год. Приятного просмотра!
Как формировался топ
В данном списке собраны самые интересные, на мой взгляд, срабатывания, о которых мы с коллегами писали в статьях за 2020 год. Важным критерием отбора была степень уверенности в том, что в соответствующем фрагменте кода действительно допущена ошибка. И, конечно, при отборе, а также расстановке мест, учитывалась, собственно, 'интересность' срабатывания, но это уже моё субъективное мнение — оспорить его вы всегда можете в комментариях.
Я старался сделать топ максимально разнообразным: как в плане сообщений PVS-Studio, так и в плане проектов, на код которых были выданы предупреждения. В список попали срабатывания на исходники 8 проверенных проектов. При этом и диагностические правила почти не повторяются — встретить дважды тут можно только V3022 и V3106 (и нет, их делал не я, но, видимо, это мои любимые). Таким образом, разнообразие вам тут обеспечено :).
Что ж, начнём! Топ 10!
10 место — Новая старая лицензия
Открывает наш топ срабатывание из статьи одного очень хорошего человека про проверку C#-проектов под Linux и macOS, где в качестве примера использовался проект RavenDB:
private static void UpdateEnvironmentVariableLicenseString(....)
{
....
if (ValidateLicense(newLicense, rsaParameters, oldLicense) == false)
return;
....
}
Предупреждение анализатора: V3066 Possible incorrect order of arguments passed to 'ValidateLicense' method: 'newLicense' and 'oldLicense'. LicenseHelper.cs (177) Raven.Server
Казалось бы, что тут не так? Код же вполне компилируется. С чего анализатор решил, будто надо сначала передавать oldLicense, а потом newLicense? Вы уже догадались, не так ли? Глянем-ка на объявление ValidateLicense:
private static bool ValidateLicense(License oldLicense,
RSAParameters rsaParameters,
License newLicense)
Ух ты, и правда — сначала в параметрах идёт старая, а только потом — новая лицензия. А ну-ка скажите, может этот ваш динамический анализ такое отловить? :)
В любом случае, срабатывание интересное. Может, там на самом деле и не важен порядок, но такие фрагменты лучше перепроверять, согласны?
9 место — 'FirstOrDefault' и неожиданный 'null'
На 9 месте оказалось срабатывание из статьи «В «osu!» играй, про ошибки не забывай», написанной в начале ныне уходящего года:
public ScoreInfo CreateScoreInfo(RulesetStore rulesets)
{
var ruleset = rulesets.GetRuleset(OnlineRulesetID);
var mods = Mods != null ? ruleset.CreateInstance()
.GetAllMods().Where(....)
.ToArray() : Array.Empty();
....
}
Видите ошибку? А она есть! Что же говорит анализатор?
Предупреждение анализатора: V3146 [CWE-476] Possible null dereference of 'ruleset'. The 'FirstOrDefault' can return default null value. APILegacyScoreInfo.cs 24
Да-да, я снова не дал всей необходимой информации сразу. На самом деле, в этом коде и правда нельзя увидеть ничего подозрительного. Ведь FirstOrDefault, о котором нам говорит анализатор, находится в определении метода GetRuleset:
public RulesetInfo GetRuleset(int id) =>
AvailableRulesets.FirstOrDefault(....);
Страшное дело! Метод вернёт RulesetInfo, если найдёт подходящий. А если нет? Спокойненько возвращаем null. И выстрелит уже в том месте, где результат вызова будет использован. В данном случае — при вызове ruleset.CreateInstance ().
Может возникнуть вопрос: ну, а вдруг там никогда не бывает null? Вдруг в коллекции всегда найдётся нужный элемент? Ну что ж, если разработчик уверен в этом, то почему бы вместо FirstOrDefault не использовать First?
8 место — Привет из Python
Последнее срабатывание из первой тройки было выдано на код проекта RunUO. Статья, посвящённая его проверке, написана в феврале и доступна по этой ссылке.
Найденный фрагмент крайне подозрителен, хотя сложно сказать наверняка, является ли он ошибкой:
public override void OnCast()
{
if ( Core.AOS )
{
damage = m.Hits / 2;
if ( !m.Player )
damage = Math.Max( Math.Min( damage, 100 ), 15 );
damage += Utility.RandomMinMax( 0, 15 );
}
else { .... }
}
Предупреждение анализатора: V3043 The code’s operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Earthquake.cs 57
Всё верно — дело в отступах! Создаётся впечатление, что строка damage += Utility.RandomMinMax (0, 15) должна была выполняться только в том случае, если m.Player — false. Подобным образом работал бы код на Python, где отступы пишутся не только для красоты, но и для определения логики приложения. Но компилятор C# придерживается другого мнения в данном вопросе! Интересно, а какое мнение было у разработчика?
Вообще в данной ситуации есть 2 варианта. Либо тут действительно пропущены фигурные скобки, и всё работает неправильно, либо всё работает правильно, но со временем точно найдётся человек, который посчитает это ошибкой и «исправит» её.
Возможно, я не прав, и действительно бывают случаи, когда нормально написать что-то такое. Если вам такие известны, то, пожалуйста, напишите комментарий — мне было бы действительно интересно узнать о таких кейсах.
7 место — Perfect или Perfect — вот в чём вопрос!
Распределять предупреждения по местам становится всё труднее, а мы переходим ко второму в этом списке срабатыванию из статьи про osu!.
Как много времени вам понадобится, чтобы увидеть ошибку?
protected override void CheckForResult(....)
{
....
ApplyResult(r =>
{
if ( holdNote.hasBroken
&& (result == HitResult.Perfect || result == HitResult.Perfect))
result = HitResult.Good;
....
});
}
Предупреждение анализатора: V3001 There are identical sub-expressions 'result == HitResult.Perfect' to the left and to the right of the '||' operator. DrawableHoldNote.cs 266
Думаю, немного, ведь достаточно лишь прочитать предупреждение PVS-Studio. Разработчики, использующие статический анализ, так обычно и делают :). Можно было бы спорить про предыдущие места в топе, но здесь ошибка налицо. Сложно сказать, какой именно элемент перечисления HitResult здесь должны были использовать вместо второго Perfect (ну или вместо первого), но в итоге написано явно что-то не то. Что ж, ничего страшного — ошибка найдена, а значит, её можно легко исправить.
6 место — null (не) пройдёт!
На 6 месте оказалось очень крутое срабатывание на код из проекта Open XML SDK. Статью, посвящённую его проверке, можно прочитать здесь.
Разработчик хотел реализовать свойство, которое не вернёт null, даже если его присвоить напрямую. И это действительно здорово, когда можешь быть уверен, что null ни при каких обстоятельствах не будет записан. Жаль, что это совсем не та ситуация:
internal string RawOuterXml
{
get => _rawOuterXml;
set
{
if (string.IsNullOrEmpty(value))
{
_rawOuterXml = string.Empty;
}
_rawOuterXml = value;
}
}
Предупреждение анализатора: V3008 The '_rawOuterXml' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 164, 161. OpenXmlElement.cs 164
Получается, что в _rawOuterXml будет записано value вне зависимости от того null оно или нет. Невнимательно взглянув на этот код, можно посчитать, что в это свойство никогда не будет записан null — ведь проверка-то есть! Что ж, раз так, то под ёлкой можно обнаружить не подарки, а неожиданный NullReferenceException:(
5 место — Выстрел из массива с массивом внутри
Топ-5 срабатывание 2020 года было выдано анализатором на лично мною проверенный проект TensorFlow.NET. Перейдя по ссылке, можно ознакомиться со статьёй, посвящённой проверке этого проекта (ох, и много ж я там повидал всякого).
Кстати, если вы любите смотреть примеры интересных ошибок из реальных C#-проектов, то предлагаю вам подписаться на мой твиттер. Там я планирую выкладывать любопытные срабатывания и просто интересные фрагменты кода, многие из которых, увы, не попадут в статьи. Буду рад вас видеть! :)
Ну, а теперь перейдём-таки к срабатыванию:
public TensorShape(int[][] dims)
{
if(dims.Length == 1)
{
switch (dims[0].Length)
{
case 0: shape = new Shape(new int[0]); break;
case 1: shape = Shape.Vector((int)dims[0][0]); break;
case 2: shape = Shape.Matrix(dims[0][0], dims[1][2]); break; // <=
default: shape = new Shape(dims[0]); break;
}
}
else
{
throw new NotImplementedException("TensorShape int[][] dims");
}
}
Предупреждение анализатора: V3106 Possibly index is out of bound. The '1' index is pointing beyond 'dims' bound. TensorShape.cs 107
На самом деле, было очень сложно выбрать, на какое место поставить это срабатывание, так как оно действительно интересное, но и другие в этом плане не отстают. Итак, давайте разбираться, что же тут происходит.
Если количество массивов в dims не равно 1, то кидается исключение типа NotImplementedException. А что будет, если в dims ровно один массив? Производится проверка количества элементов в этом 'массиве внутри массива'. Обратите внимание на то, что происходит, когда оно равно 2. В качестве одного из аргументов конструктора Shape.Matrix передаётся, неожиданно, dims[1][2]. А теперь давайте-ка вспомним — сколько там было элементов в массиве dims?
Верно, ровно один — мы ведь это проверяли только что! Попытка получения второго элемента из массива, в котором всего один элемент, приведёт к выбрасыванию исключения типа IndexOutOfRangeException. Очевидно, ошибка. А вот очевиден ли способ её исправления?
Первым, что приходит на ум, будет изменение dims[1][2] на dims[0][2]. Ошибка исчезнет? Как бы не так! Опять будет то же самое исключение. Но на этот раз оно будет связано с тем, что в данной case-ветке количество элементов в dims[0] равно 2. Неужели разработчик допустил две ошибки в индексе подряд? Или всё-таки там должна использоваться какая-то другая переменная? Кто знает… Дело анализатора — ошибку найти, а уж исправлять её придётся допустившему её человеку или его коллегам.
4 место — Свойство объекта, которого нет
Ещё одно срабатывание попало в это топ из статьи про проверку OpenRA. Возможно, оно и заслуживает большего, но волею судьбы это срабатывание оказалось на 4 месте. Что ж, это тоже весьма неплохо! Давайте же глянем, какую ошибку смог обнаружить PVS-Studio на этот раз:
public ConnectionSwitchModLogic(....)
{
....
var logo = panel.GetOrNull("MOD_ICON");
if (logo != null)
{
logo.GetSprite = () =>
{
....
};
}
if (logo != null && mod.Icon == null) // <=
{
// Hide the logo and center just the text
if (title != null)
title.Bounds.X = logo.Bounds.Left;
if (version != null)
version.Bounds.X = logo.Bounds.X;
width -= logo.Bounds.Width;
}
else
{
// Add an equal logo margin on the right of the text
width += logo.Bounds.Width; // <=
}
....
}
Предупреждение анализатора: V3125 The 'logo' object was used after it was verified against null. Check lines: 236, 222. ConnectionLogic.cs 236
На что тут стоит обратить внимание? Ну, для начала, в logo наверняка может быть записан null. На это намекают и постоянные проверки, и название метода GetOrNull, который возвращает значение, записываемое в logo. Раз так, давайте подумаем, что же произойдёт, если GetOrNull и правда вернёт null. Поначалу всё в порядке, но затем происходит проверка условия logo!= null && mod.Icon == null. Как вы понимаете, в результате будет произведён переход к else-ветке и… Попытка обращения к свойству Bounds переменной, в которую записан null, а затем — бдыщ! NullReferenceException стучится в дверь.
3 место — Элемент Шрёдингера
Наконец, мы подошли к тройке финалистов. Топ-3 ошибка за 2020 год обнаружилась в проекте Nethermind, о проверке которого была написана статья с интригующим названием «Код в одну строку или проверка Nethermind с помощью PVS-Studio C# для Linux». Ошибка невероятно проста, но при этом незаметна для человеческого глаза, особенно если учесть размеры проекта. Как вы считаете, достойно ли своего места это срабатывание?
public ReceiptsMessage Deserialize(byte[] bytes)
{
if (bytes.Length == 0 && bytes[0] == Rlp.OfEmptySequence[0])
return new ReceiptsMessage(null);
....
}
Предупреждение анализатора: V3106 Possibly index is out of bound. The '0' index is pointing beyond 'bytes' bound. Nethermind.Network ReceiptsMessageSerializer.cs 50
Наверное, иметь возможность взять первую вещь, лежащую в пустой коробке, было бы круто, но здесь такое желание приведёт лишь к выбрасыванию IndexOutOfRangeException. Всего одна мелочь — крошечная ошибка в операторе, а приложение уже работает неправильно, а может и вовсе падает.
Очевидно, вместо '&&' тут стоит использовать '||'. Подобные логические ошибки — не редкость, особенно в сложных конструкциях. Поэтому проверять такие моменты в автоматическом режиме достаточно удобно.
2 место — Меньше 2, но больше 3
На втором место я поставил ещё одно срабатывание на код из проекта RavenDB. Напомню, что о результатах его проверки (и не только) можно почитать в соответствующей статье.
Ну, а теперь встречайте — топ-2 ошибка 2020 года:
private OrderByField ExtractOrderByFromMethod(....)
{
....
if (me.Arguments.Count < 2 && me.Arguments.Count > 3)
throw new InvalidQueryException(....);
....
}
Предупреждение анализатора: V3022 Expression 'me.Arguments.Count 3' is always false. Probably the '||' operator should be used here. QueryMetadata.cs (861) Raven.Server
Ранее мы рассматривали моменты, в которых выбрасывалось неожиданное исключение, а теперь наоборот — ожидаемое исключение не будет выброшено никогда. Ну или всё-таки будет выброшено, если кто-нибудь придумает число, которое будет меньше 2, но при этом больше 3.
Не удивлюсь, если вы не согласитесь, но это срабатывание действительно нравится мне больше всех предыдущих. Да, ошибка невероятно простая, а для её исправления нужно лишь поменять оператор. На это, кстати, намекает и сообщение, передаваемое в конструктор InvalidQueryException: «Invalid ORDER BY 'spatial.distance (from, to, roundFactor)' call, expected 2–3 arguments, got » + me.Arguments.Count.
Соглашусь, это элементарная оплошность, но её никто не заметил и не поправил, как минимум, до тех пор, пока она не была найдена с помощью PVS-Studio. Это напоминает мне о том, что программисты, сколь бы опытны они ни были, всё равно остаются (к сожалению?) людьми. А люди, независимо от квалификации, могут пропустить даже такие глупые ошибки по самым разным причинам. Иногда ошибка выстреливает сразу, а иногда можно долго-долго гадать, почему же пользователь не видит сообщение о некорректном вызове ORDER BY.
1 место — Кавычки на +100% к безопасности кода
Ура, ура, ура! Мы наконец-то добрались до срабатывания, которое я посчитал самым интересным, забавным, классным и так далее. Оно было выдано на код из проекта ONLYOFFICE, с анализом которого связана одна из самых последних статей этого года — «ONLYOFFICE Community Server: как баги способствуют возникновению проблем с безопасностью».
Ну что ж, представляю вам самую грустную историю про ArgumentException, который никогда не будет создан:
public void SetCredentials(string userName, string password, string domain)
{
if (string.IsNullOrEmpty(userName))
{
throw new ArgumentException("Empty user name.", "userName");
}
if (string.IsNullOrEmpty("password"))
{
throw new ArgumentException("Empty password.", "password");
}
CredentialsUserName = userName;
CredentialsUserPassword = password;
CredentialsDomain = domain;
}
Предупреждение анализатора: V3022 Expression 'string.IsNullOrEmpty («password»)' is always false. SmtpSettings.cs 104
Было очень сложно выбрать, какую ошибку на какое место поставить, но это срабатывание для меня с самого начала было лидером среди всех. Простейшая мелкая опечатка — и код уже работает неправильно. Не спасла ни подцветка в IDE, ни ревью, ни старый добрый здравый смысл. Это ведь маленькая, простая, красиво написанная функция. И даже здесь PVS-Studio смог найти то, что было пропущено профессионалами.
Как обычно, дьявол кроется в деталях. Разве бы не было здорово, если бы все такие детали искались автоматически? Конечно, было бы! А разработчик пусть занимается тем, чем статический анализатор заниматься не может — создаёт новые прекрасные и безопасные приложения. Творит, не думая о том, поставил он лишние кавычки при проверке переменной или нет.
Заключение
Найти 10 интересных ошибок в статьях 2020 года было несложно. А вот распределить их по местам оказалось той ещё задачкой. С одной стороны, некоторые срабатывания лучше отражают работу продвинутых механизмов анализатора. С другой — какие-то из ошибок просто кажутся в некоторой мере забавными. Многие из представленных позиций можно было бы поменять местами — например, топ-2 и топ-3.
А может, вы считаете, что в этом списке вообще должны быть какие-то другие срабатывания? На самом деле, у вас есть возможность даже составить собственный топ, перейдя по ссылке к списку статей и отыскав там самые вкусные по вашему мнению срабатывания. В таком случае, обязательно скидывайте свои топы 2020 в комментарии, я с большим удовольствием почитаю. Сможете ли вы составить список интереснее моего?
Конечно, вопрос 'интересности' предупреждений в любом случае субъективен. На мой взгляд, главный критерий оценки срабатывания состоит в том, будет ли программист, увидевший предупреждение от PVS-Studio, менять что-то в соответствующем коде? Данный список как раз и был собран из срабатываний на фрагменты, которые, на мой взгляд, выглядели бы лучше, если бы разработчики использовали статический анализ. К тому же, нет никаких проблем с тем, чтобы попробовать PVS-Studio, проверив собственные или какие-то другие проекты. Достаточно лишь перейти по ссылке, скачать там нужную версию анализатора и запросить триальный ключ, заполнив маленькую форму.
Ну, а на этом у меня всё, большое спасибо за внимание и до новых встреч!
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lipilin. Top-10 Bugs Found in C# Projects in 2020.
