Для оценки качества работы нашего анализатора, а также с целью популяризации методологии статического анализа, мы регулярно проверяем на наличие ошибок проекты с открытым исходным кодом и пишем про это статьи. Не стал исключением и прошедший 2016 год, который примечателен ещё и тем, что это было время своеобразного «взросления» C# анализатора. PVS-Studio получил значительное количество новых C# диагностик, улучшенный механизм работы с виртуальными значениями (symbolic execution) и многое другое. По результатам проделанной нашей командой работы, я составил своеобразный хит-парад наиболее интересных ошибок, обнаруженных в проектах С# в 2016 году.Десятое место: когда в минуте не всегда 60 секунд
Начну хит-парад с ошибки, обнаруженной при проверке проекта Orchard CMS. Описание ошибки можно найти в статье. Вообще же, со всеми статьями про проверку проектов можно ознакомиться здесь.
V3118 Seconds component of TimeSpan is used, which does not represent full time interval. Possibly 'TotalSeconds' value was intended instead. AssetUploader.cs 182
void IBackgroundTask.Sweep()
{
....
// Don't flood the database with progress updates;
// Limit it to every 5 seconds.
if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5)
{
....
}
}Вместо TotalSeconds в данном случае разработчик ошибочно использовал Seconds. Таким образом, будет получено не полное число секунд, содержащееся между датами _clock.UtcNow и lastUpdateUtc, как рассчитывал программист, а только остаточное значение диапазона. Например, для значения диапазона 1 минута 4 секунды результатом будет не 64, а 4 секунды. Невероятно, но даже опытные разработчики допускают подобные ошибки.
Девятое место: выражение всегда истинно
Следующая ошибка приведена в статье о проверке проекта GitExtensions.
V3022 Expression 'string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)' is always true. GitUI FormFormatPatch.cs 155
string rev1 = "";
string rev2 = "";
var revisions = RevisionGrid.GetSelectedRevisions();
if (revisions.Count > 0)
{
rev1 = ....;
rev2 = ....;
....
}
else
if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <=
{
MessageBox.Show(....);
return;
}Обратите внимание на ключевое слово else. Вероятно, ему вовсе тут не место. Невнимательность при рефакторинге или банальная усталость программиста, и вот мы получаем кардинальное изменение логики работы программы, что приводит к непредсказуемому поведению. Хорошо, что статический анализатор никогда не устаёт.
Восьмое место: возможная опечатка
В статье о проверке исходного кода FlashDevelop приведена интересная ошибка, связанная с опечаткой.
V3056 Consider reviewing the correctness of 'a1' item's usage. LzmaEncoder.cs 225
public void SetPrices(....)
{
UInt32 a0 = _choice.GetPrice0();
UInt32 a1 = _choice.GetPrice1();
UInt32 b0 = a1 + _choice2.GetPrice0(); // <=
UInt32 b1 = a1 + _choice2.GetPrice1();
....
}Я согласен с анализатором, а также с автором статьи. Вместо переменной a1 в отмеченной строке напрашивается использование а0. В любом случае, не мешало бы дать переменным более понятные имена.
Седьмое место: логическая ошибка
По мотивам повторной проверки проекта Umbraco также была написана статья. Пример интересной, на мой взгляд, ошибки из этой статьи.
V3022 Expression 'name != «Min» || name != «Max»' is always true. Probably the '&&' operator should be used here. DynamicPublishedContentList.cs 415
private object Aggregate(....)
{
....
if (name != "Min" || name != "Max")
{
throw new ArgumentException(
"Can only use aggregate min or max methods on properties
which are datetime");
}
....
}Исключение типа ArgumentException будет выброшено при любом значении переменной name. И всё из-за ошибочного использования в условии оператора || вместо &&.
Шестое место: ошибочное условие выхода из цикла
Статья о проверке проекта Accord.Net содержит описание нескольких интересных ошибок. Я выбрал две, одна из которых вновь связана с опечаткой.
V3015 It is likely that a wrong variable is being compared inside the 'for' operator. Consider reviewing 'i' Accord.Audio SampleConverter.cs 611
public static void Convert(float[][] from, short[][] to)
{
for (int i = 0; i < from.Length; i++)
for (int j = 0; i < from[0].Length; j++)
to[i][j] = (short)(from[i][j] * (32767f));
}Ошибка содержится в условии второго цикла for, счётчиком которого является переменная j. Использование имен переменных вида i, j для счётчиков — это, своего рода, классика жанра. К сожалению, эти переменные очень схожи по написанию и разработчики часто допускают опечатки в подобном коде. Не думаю, что в данном случае стоит рекомендовать использование более осмысленных имен. Все равно так делать никто не будет :). Поэтому дам другую рекомендацию: используйте статические анализаторы!
Пятое место: использование битового оператора вместо логического
Еще одна интересная и достаточно распространенная ошибка из статьи о проверке проекта Accord.Net.
V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&&' operator should be used instead. Accord.Math JaggedSingularValueDecompositionF.cs 461
public JaggedSingularValueDecompositionF(....)
{
....
if ((k < nct) & (s[k] != 0.0))
....
}Очевидно, что даже если не будет выполнено первое условие, ошибочное использование оператора & вместо && приведет к проверке второго условия, что, в свою очередь, повлечёт выход за пределы массива.
Четвертое место: раз кавычка, два кавычка
На четвертом месте — ошибка из статьи о проверке проекта Xamarin.Forms.
V3038 The first argument of 'Replace' function is equal to the second argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349
void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na)
{
....
output.Write("string('{0}')",
NRefactory.CSharp
.TextWriterTokenWriter
.ConvertString(
(string)na.Argument.Value).Replace("'", "\'"));
....
}В данном случае будет произведена замена кавычки на… кавычку. Не думаю, что это именно то, чего добивался разработчик.
А анализатор — молодец!
Третье место: ThreadStatic
Проект Mono, о проверке которого написана статья, оказался богат на интересные ошибки. А одна из них — действительно редкий гость.
V3089 Initializer of a field marked by [ThreadStatic] attribute will be called once on the first accessing thread. The field will have default value on different threads. System.Data.Linq-net_4_x Profiler.cs 16
static class Profiler
{
[ThreadStatic]
private static Stopwatch timer = new Stopwatch();
....
}Если кратко: выполняется некорректная инициализация поля, отмеченного атрибутом ThreadStatic. В документации к диагностике приведено подробное описание ситуации, а также даны советы, как можно избежать подобных ошибок. Великолепный пример ошибки, которую не так просто найти и исправить обычными средствами.
Второе место: Copy-Paste, эталонно!
Один из эталонных, на мой взгляд, примеров ошибки типа Copy-Paste содержится в уже упомянутой ранее статье о проверке проекта Mono.
V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258
Вот краткий фрагмент кода, в котором была найдена ошибка:
button_pressed_highlight = use_system_colors ?
Color.FromArgb (150, 179, 225) :
Color.FromArgb (150, 179, 225);Вы спросите: «А что же тут такого?» Стоило ли помещать такую очевидную ошибку на второе место хит-парада? Дело в том, что приведенный фрагмент кода был дополнительно отформатирован для наглядности. А теперь подготовьтесь и представьте, что перед вами стоит задача поиска подобной ошибки в коде без использования инструментальных средств. Итак, слабонервных просьба удалиться, ниже следует скриншот, на котором содержится полный фрагмент кода с ошибкой:
Очевидно, что подобную ошибку может допустить любой программист вне зависимости от его квалификации. А успешный поиск таких ошибок демонстрирует всю мощь статического анализа.
Первое место: PVS-Studio
Да, вам не показалось. Здесь действительно написано «PVS-Studio». И он на первом месте нашего хит-парада. Но не только потому, что это хороший статический анализатор. А ещё и потому, что в нашей команде работают обычные люди, которые допускают обычные человеческие ошибки в коде. Именно про это и была в своё время написана статья. Ошибка, а точнее сразу две, которые были обнаружены в коде PVS-Studio при помощи PVS-Studio.
V3022 Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559
V3022 Expression 'RowsCount > 200000' is always false. ProcessingEngine.cs 561
public void ProcessFiles(....)
{
....
int RowsCount =
DynamicErrorListControl.Instance.Plog.NumberOfRows;
if (RowsCount > 20000)
DatatableUpdateInterval = 30000; //30s
else if (RowsCount > 100000)
DatatableUpdateInterval = 60000; //1min
else if (RowsCount > 200000)
DatatableUpdateInterval = 120000; //2min
....
}Результатом работы данного фрагмента кода (при условии, что RowsCount > 20000) всегда будет значение DatatableUpdateInterval равное 30000.
К счастью, мы уже проделали определенную работу в этом направлении.
Благодаря повсеместному использованию инкрементального анализа в нашей команде, появление статей о поиске ошибок в PVS-Studio при помощи PVS-Studio в будущем будет очень маловероятно.
Заключение
Хочу заметить, что теперь любой из вас может составить свой хит-парад найденных ошибок, воспользовавшись возможностью бесплатного использования статического анализатора PVS-Studio для проверки собственных проектов.
Скачать и попробовать PVS-Studio: http://www.viva64.com/ru/pvs-studio/
По вопросам приобретения коммерческой лицензии просим Вас связаться с нами в почте. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS-Studio, если хотите снять ограничения демонстрационной версии.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Top 10 C# projects errors found in 2016
Комментарии (37)
Mak_71_rus
27.02.2017 10:59+1Ребят, ну эта статья уже на откровенно рекламную тянет.
sutarmin
27.02.2017 15:35+7Тогда что же такое у них все остальные статьи? Обычно в блог PVS-Studio все идут, чтобы поглазеть на занимательные баги, а тут подборка из ТОП-10 багов за всё время. Мне кажется, что это одна из лучших статей в их блоге.
Espleth
27.02.2017 11:06+4По поводу третьего места, вас не смущает, что в вашем решении если пользователь инициализирует переменную нулем а потом попробует ее получить, то он получит 42?)
get { if (field == default(Int32)) field = 42; return field; }
n0mo
27.02.2017 12:28В документации приведен синтетический пример. Конечно, он не идеален. Но смысл — указать на возможный путь обхода проблемы, а конечную логику в каждом конкретном случае придумает сам разработчик. В данном примере можно было бы ввести дополнительный флаг, сигнализирующий об установке переменной значения по-умолчанию (0) в сеттере, и использовать его в геттере.
Espleth
27.02.2017 18:29+1Как вариант, или сделать int nullable
т.е. вот так в примере
private static Int32? field = 42;
и проверку if (field == null) или же просто return field ?? 42;
Но суть в том, что не очень то хорошо такие примеры приводить, хотя конечно кто будет слепо копировать сам себе злобный буратино =)
matabili1973
27.02.2017 12:08По поводу 4 места: это же стандартная процедура экранирования служебного символа, нет? Я думаю, автор пытается сделать именно то, что получит — кавычку как часть строки.
sabio
27.02.2017 12:30Чтобы получить кавычку "как часть строки", нужно заменить её на "слэш-кавычку". Т.е. должно быть двойное экранирование:
Replace("'", "\\'")n0mo
27.02.2017 12:34или так:
Replace("'", @"\'")matabili1973
27.02.2017 13:32Ок, я не знаю C#, но ясно, что ошибка не в задумке, а в реализации.
Stroce
27.02.2017 15:35+3Тут везде ошибка в реализации. Вряд ли статический анализатор укажет на ошибку в задумке. Под «разработчик задумывал не это» имеется в виду «он хотел правильно, но получилось неправильно», кмк
ertaquo
27.02.2017 12:24+2Оффтоп: а вы не проверяли исходный код игры «ЩИ!!! Симулятор жестокости»?
http://www.gamedev.ru/projects/forum/?id=160897
https://gist.github.com/ForNeVeR/9001938
ForNeVeR
27.02.2017 19:32+1Друзья, пожалуйста, учтите, что код по ссылке на gist не мой, а я просто его вовремя стянул с форума и выложил на всеобщее обозрение (поскольку ссылки с форума на gamedev давно протухли, это вполне полезно). Свой код я так не пишу!
ertaquo
27.02.2017 20:23Забыл упомянуть, пардон, не со зла.
Кстати, вот статья про разработку этого шедевра: https://nooby-games.ru/щи-симулятор-жестокости/
k12th
27.02.2017 12:52+2Оффтоп: а неужели нельзя это место на C# как-то поизящнее переписать? Я не настоящий сварщик, но должен же быть способ без адской копипасты.
x893
27.02.2017 14:23+1ты обычно полные профессионалы из индии пишут.
Durimar123
27.02.2017 18:07Есть небольшая вероятность, что это «auto generation code».
Когда то, я писал создание юнита констант из базы данных, получался примерно такой же код.
Deosis
28.02.2017 06:38А у вас есть другой способ проинициализировать 50 полей?
k12th
28.02.2017 11:13Вроде в шарпе были литералы для структур? Но, как я говорил, я не настоящий сварщик.
mayorovp
28.02.2017 22:19Для начала, все эти _begin/_end/_middle нужно группировать в объекты.
Загрузка из конфига неплохо работает. Или из ресурсов. Или автогенерация подобного же кода — но уже на основе другого файла. Или хотя бы составление всего вот этого в визуальном редакторе...
sabio
27.02.2017 13:03+4Ещё один оффтоп :-)
Недавно прочитал статью The Day I Parsed A Monster. Статья об одной из трудностей, с которой столкнулись разработчики X-Ray при проверке .NET Core runtime.
Сама статья несколько раздута (а уж "решение" и вовсе не интересно — замена полноценного парсера десятком микро-грамматик для распознавания структурных единиц: условий, циклов и пр.). Но несколько моментов мне всё же показались интересными:
1) В ходе работы над X-Ray была найдена синтаксическая ошибка в
gc.cpp. Я вижу, вы проверяли проекты .NET Core в декабре 2015. Но не смог найти, была ли в ваших отчётах эта ошибка? (я, правда, не очень силён в "топонимике" .NET Core: runtime — это то же самое, что CoreCLR? и если нет, не хотите ли проверить .NET Core runtime?)
2) Автор статьи сокрушается о "неподъёмной" сложности
gc.cppс его 37 тысячами строк кода. Как обстоят дела у PVS-Studio с обработкой файлов таких размеров?
3) Также интересной мне показалась идея отслеживать изменения в коде по логам системы контроля версий с тем, чтобы давать больший приоритет определённым участкам. Правда, это более актуально для анализа сложности, чем для поиска ошибок. Но, быть может, и вам будет интересно реализовать что-то подобное. Скажем, для запуска наиболее ресурсоёмких проверок (в первую очередь) на новых участках кода.
4) А ещё понравилась их диаграмма. Если сделать такую с диагностиками, можно будет одним взглядом оценить, насколько плохи дела в том или ином модуле.
5) Не думали ли вы сделать SaaS версию PVS-Sudio? Чтобы можно было указать ей URL открытого проекта на GitHub или BitBucket и (через какое-то время) получить отчёт.
jetcar
27.02.2017 16:17-8первое место это тупая попытка рекламы, кто же пишет код, а потом его совсем не тестирует, а попытка втюхать анализатор кода вместо написания тестов как-то сомнительно.
Andrey2008
27.02.2017 17:26+7Ох уж мне эти программисты-идеалисты. :) Напоминаю, что мы находим ошибки, в таких проектах, как GCC, GDB, Clang, Qt, Chromium, Boost и так далее. Вы думаете, они не тестирую проекты? :)
jetcar
27.02.2017 19:01-4а я и не говорил что анализатор плох, а вот писать статьи где основная цель реклама это очень плохо
Andrey2008
27.02.2017 17:52Статический анализ не отменяет необходимость тестов. Статический анализ, это ответ на вопрос: «А что ещё мы можем сделать, чтобы улучшить качество и надёжность?». Статический анализатор не конкурирует с другими методологиями, а дополняет их. В ровно в той же степени, в какой предупреждения компилятора не заменяют и не конкурирует с методологией TDD, или скажем с отладчиком.
Использование статического анализа свидетельствует о зрелости процессов разработки, используемых в компаниях.
Andrey2008
06.03.2017 14:02Решил послать весточку интересующимся анализатором PVS-Studio. Сегодня 06.03.2013 в 15.00 состоится пробный стрим, где можно будет вживую посмотреть на процесс работы PVS-Studio и интерактивно пообщаться. Присоединяйтесь к нашему стриму. Подробности (см. раздел Важно).
Siemargl
Непонятно, почему RowsCount > 100000 всегда false?
До этого условия просто не дойдет очередь, т.к отработает ветка RowsCount > 20000, но оно может быть и истинным.
qark
Если RowsCount <= 20000, то очередь дойдёт, и условие RowsCount > 100000 всегда будет ложным.
Если же RowsCount > 20000, то очередь действительно не дойдёт, но в таком случае значение RowsCount > 100000 не представляет никакого интереса.
То есть диагностика «условие всегда ложно» неявно подразумевает «когда его будут проверять».
Siemargl
Ну конечно, случай <=20000 я просто не рассматривал по логике дальнейших действий.
Кстати, а в этом случае не обнаружилось, что DatatableUpdateInterval не инициализирована или равна 0, что по логике тоже ошибка? Или же она инициализирована ранее в коде, но не показана?
n0mo
Ранее она инициализирована значением по умолчанию = 15000
klisitsyn
Согласен с Siemargl. Почему столько несогласных?
«V3022 Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559»
Это неверное утверждение. Выражение само по себе будет истинным при RowsCount свыше 100000, нельзя утвеждать что это «always false». Другое дело, что ветка условия действительно никогда не будет выполнена.
Возможно, было бы лучше перефразировать предупреждение как-нибудь так: «V3022 Unreachable conditional operator branch identified by expression 'Expression 'RowsCount > 100000' (overridden by prior expression 'RowsCount > 20000')».
Andrey2008
В ТОТ МОМЕНТ когда будет вычисляться условие RowsCount > 100000, оно ВСЕГДА БУДЕТ ЛОЖНО. Так что всё нормально. Возможно логика сообщений может казаться странной, но на всех всё равно не угодишь. :)
Deosis
Тогда уж лучше написать, что «код недостижим». Иначе человек попадет в ступор. Например он вводит миллион, а программа говорит, что введенное число никогда не будет больше ста.