- PVSM.RU - https://www.pvsm.ru -
Для оценки качества работы нашего анализатора, а также с целью популяризации методологии статического анализа, мы регулярно проверяем на наличие ошибок проекты с открытым исходным кодом и пишем про это статьи. Не стал исключением и прошедший 2016 год, который примечателен ещё и тем, что это было время своеобразного «взросления» C# анализатора. PVS-Studio получил значительное количество новых C# диагностик, улучшенный механизм работы с виртуальными значениями (symbolic execution) и многое другое. По результатам проделанной нашей командой работы, я составил своеобразный хит-парад наиболее интересных ошибок, обнаруженных в проектах С# в 2016 году.
Десятое место: когда в минуте не всегда 60 секунд
Начну хит-парад с ошибки, обнаруженной при проверке проекта Orchard CMS. Описание ошибки можно найти в статье [1]. Вообще же, со всеми статьями про проверку проектов можно ознакомиться здесь [2].
V3118 [3] 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 секунды. Невероятно, но даже опытные разработчики допускают подобные ошибки.
Девятое место: выражение всегда истинно
Следующая ошибка приведена в статье [4] о проверке проекта GitExtensions.
V3022 [5] 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. Вероятно, ему вовсе тут не место. Невнимательность при рефакторинге или банальная усталость программиста, и вот мы получаем кардинальное изменение логики работы программы, что приводит к непредсказуемому поведению. Хорошо, что статический анализатор никогда не устаёт.
Восьмое место: возможная опечатка
В статье [6] о проверке исходного кода FlashDevelop приведена интересная ошибка, связанная с опечаткой.
V3056 [7] 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 также была написана статья [8]. Пример интересной, на мой взгляд, ошибки из этой статьи.
V3022 [5] 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. И всё из-за ошибочного использования в условии оператора || вместо &&.
Шестое место: ошибочное условие выхода из цикла
Статья [9] о проверке проекта Accord.Net содержит описание нескольких интересных ошибок. Я выбрал две, одна из которых вновь связана с опечаткой.
V3015 [10] 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 для счётчиков — это, своего рода, классика жанра. К сожалению, эти переменные очень схожи по написанию и разработчики часто допускают опечатки в подобном коде. Не думаю, что в данном случае стоит рекомендовать использование более осмысленных имен. Все равно так делать никто не будет :). Поэтому дам другую рекомендацию: используйте статические анализаторы!
Пятое место: использование битового оператора вместо логического
Еще одна интересная и достаточно распространенная ошибка из статьи [9] о проверке проекта Accord.Net.
V3093 [11] 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))
....
}
Очевидно, что даже если не будет выполнено первое условие, ошибочное использование оператора & вместо && приведет к проверке второго условия, что, в свою очередь, повлечёт выход за пределы массива.
Четвертое место: раз кавычка, два кавычка
На четвертом месте — ошибка из статьи [12] о проверке проекта Xamarin.Forms.
V3038 [13] 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, о проверке которого написана статья [14], оказался богат на интересные ошибки. А одна из них — действительно редкий гость.
V3089 [15] 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. В документации [16] к диагностике приведено подробное описание ситуации, а также даны советы, как можно избежать подобных ошибок. Великолепный пример ошибки, которую не так просто найти и исправить обычными средствами.
Второе место: Copy-Paste, эталонно!
Один из эталонных, на мой взгляд, примеров ошибки типа Copy-Paste содержится в уже упомянутой ранее статье [14] о проверке проекта Mono.
V3012 [17] 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». И он на первом месте нашего хит-парада. Но не только потому, что это хороший статический анализатор. А ещё и потому, что в нашей команде работают обычные люди, которые допускают обычные человеческие ошибки в коде. Именно про это и была в своё время написана статья [18]. Ошибка, а точнее сразу две, которые были обнаружены в коде PVS-Studio при помощи PVS-Studio.
V3022 [19] Expression 'RowsCount > 100000' is always false. ProcessingEngine.cs 559
V3022 [19] 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 в будущем будет очень маловероятно.
Заключение
Хочу заметить, что теперь любой из вас может составить свой хит-парад найденных ошибок, воспользовавшись возможностью бесплатного [20] использования статического анализатора PVS-Studio для проверки собственных проектов.
Скачать и попробовать PVS-Studio: http://www.viva64.com/ru/pvs-studio/ [21]
По вопросам приобретения коммерческой лицензии просим Вас связаться [22] с нами в почте. Вы также можете написать нам, чтобы получить временную лицензию для всестороннего изучения PVS-Studio, если хотите снять ограничения [23] демонстрационной версии.
Автор: PVS-Studio
Источник [24]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/c-2/246597
Ссылки в тексте:
[1] статье: http://www.viva64.com/ru/b/0456/
[2] здесь: http://www.viva64.com/ru/inspections/
[3] V3118: http://www.viva64.com/ru/w/V3118/
[4] статье: http://www.viva64.com/ru/b/0440/
[5] V3022: http://www.viva64.com/ru/w/V3022/
[6] статье: http://www.viva64.com/ru/b/0412/
[7] V3056: http://www.viva64.com/ru/d/0453/
[8] статья: http://www.viva64.com/ru/b/0461/
[9] Статья: http://www.viva64.com/ru/b/0410/
[10] V3015: https://www.viva64.com/ru/w/V3015/
[11] V3093: http://www.viva64.com/ru/d/0494/
[12] статьи: http://www.viva64.com/ru/b/0400/
[13] V3038: http://www.viva64.com/ru/d/0423/
[14] статья: http://www.viva64.com/ru/b/0431/
[15] V3089: http://www.viva64.com/ru/d/0496/
[16] документации: http://www.viva64.com/ru/w/V3089/
[17] V3012: http://www.viva64.com/ru/d/0383/
[18] статья: http://www.viva64.com/ru/b/0382/
[19] V3022: http://www.viva64.com/ru/d/0391/
[20] бесплатного: http://www.viva64.com/ru/b/0457/
[21] http://www.viva64.com/ru/pvs-studio/: http://www.viva64.com/ru/pvs-studio/
[22] связаться: http://www.viva64.com/ru/about-feedback/
[23] ограничения: http://www.viva64.com/ru/m/0009/
[24] Источник: https://habrahabr.ru/post/322662/?utm_source=habrahabr&utm_medium=rss&utm_campaign=best
Нажмите здесь для печати.