Работая над развитием статического анализатора исходного кода PVS-Studio, мы часто сталкиваемся с необходимостью проверки на наличие ошибок больших открытых проектов от именитых разработчиков. Тот факт, что даже в таких проектах удается найти ошибки, делает нашу работу гораздо более осмысленной. К сожалению, все допускают ошибки. Как бы грамотно ни была выстроена система контроля качества выпускаемого программного кода, нет абсолютно никакой возможности избежать особенностей "человеческого фактора". До тех пор, пока разработкой программ занимаются люди, актуальность использования инструментов для поиска ошибок в коде, таких как PVS-Studio, не уменьшится. Сегодня мы будем искать ошибки в исходном коде MSBuild, который, увы, тоже не идеален.
Microsoft Build Engine (MSBuild) - это платформа для автоматизации сборки приложений от компании Microsoft. MSBuild обычно используется совместно с Microsoft Visual Studio, однако не зависит от него. MSBuild обеспечивает для файла проекта (*.csproj, *.vbproj, *.vcxproj) схему XML, которая управляет способами обработки и сборки приложений платформой сборки. MSBuild является частью платформы .NET и разработан на языке программирования C#.
Microsoft предоставляет исходные коды MSBuild для свободной загрузки на ресурсе GitHub. Учитывая высокие стандарты качества разработки приложений, принятые в компании Microsoft, задача поиска ошибок в исходном коде MSBuild является непростой даже для качественного статического анализатора. Но дорогу осилит идущий. Проведем проверку исходного кода MSBuild при помощи PVS-Studio версии 6.07.
Решение MSBuild состоит из 14 проектов, которые, в свою очередь, содержат в совокупности 1256 файлов с исходным кодом на языке программирования C#. Примерное число строк кода составляет 440 000.
После проверки MSBuild статическим анализатором PVS-Studio было получено 262 предупреждения. Общая статистика проверки с разграничением по уровням важности полученных предупреждений имеет вид:
Из диаграммы видно, что было выдано 73 предупреждения высокого уровня, 107 среднего и 82 низкого. Основной упор следует сделать на изучении сообщений с уровнями High и Medium. Здесь содержатся потенциально опасные конструкции и реальные ошибки. Предупреждения уровня Low также указывают на ошибки, но в них высок процент ложных срабатываний, и при написании статей мы обычно их не изучаем.
Проведенный анализ полученных предупреждений выявил, что на уровнях High и Medium содержится порядка 45% ошибочных конструкций (81 ошибка). Оставшиеся предупреждения не являются ошибками, а представляют собой просто подозрительные с точки зрения PVS-Studio конструкции и ложные срабатывания. Некоторые предупреждения были получены для Unit-тестов или в тех частях кода, где присутствуют комментарии, поясняющие, что конструкция заведомо небезопасна и используется для проверки на выброс исключения. Тем не менее, оставшиеся предупреждения анализатора требуют пристального внимания разработчиков, так как только авторы действительно знают свой код и способны дать адекватную оценку правильности того или иного предупреждения.
С учетом этого, коэффициент обнаружения ошибок PVS-Studio на уровнях High и Medium на тысячу строк кода (плотность ошибок) составляет всего 0.184 (ошибок / 1 KLoc). Это неудивительно для продукта, разрабатываемого Microsoft, и свидетельствует о высоком качестве кода MSBuild.
Опишем полученные результаты более подробно. При этом заметим, что разбор всех выданных анализатором предупреждений выходит за рамки данной статьи. Ограничимся описанием наиболее интересных сообщений, сгруппировав однотипные случаи.
Ошибочная проверка на равенство null
Предупреждение анализатора PVS-Studio: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'name'. AssemblyRemapping.cs 64
Пожалуй, уже ставшая классической ошибка, которая встречается почти в каждом проверяемом нами проекте. После приведения типа с помощью оператора as, на равенство null проверяют не ту переменную:
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (obj == null) // <=
{
return false;
}В данном случае необходимо проверить на равенство null переменную name. Корректный вариант кода:
AssemblyNameExtension name = obj as AssemblyNameExtension;
if (name == null)
{
return false;
}Несвоевременная проверка на равенство null
Предупреждение анализатора PVS-Studio: V3095 The 'diskRoots' object was used before it was verified against null. Check lines: 2656, 2659. ToolLocationHelper.cs 2656
Обратите внимание на параметр diskRoots. Это объект класса List, и его значение может быть равно null. Однако, проверка данного факта производится только во втором блоке if уже после того, как переменная diskRoots была использована для вставки значений в список:
private static void ExtractSdkDiskRootsFromEnvironment
(List<string> diskRoots, string directoryRoots)
{
if (!String.IsNullOrEmpty(directoryRoots))
{
....
diskRoots.AddRange(splitRoots); // <=
}
if (diskRoots != null) // <=
....
}В коде MSBuild было найдено еще 8 подобных потенциально небезопасных конструкций:
Ошибочное предположение о длине строки
Предупреждение анализатора PVS-Studio: V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. Utilities.cs 328
Условием входа в блок if является строка, состоящая из одного или более символов. При этом внутри блока производится попытка получения подстроки исходной строки. Очевидно, что в строке, состоящей из одного символа, второй параметр метода Substring будет отрицательным, и метод выбросит исключение ArgumentOutOfRangeException:
if (toolsVersionList.Length > 0)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}Безопасный вариант данного фрагмента кода мог бы выглядеть так:
if (toolsVersionList.Length > 1)
{
toolsVersionList = toolsVersionList.Substring(0,
toolsVersionList.Length - 2);
}Подобные ошибки в коде:
Приведение типа с потерей точности
Предупреждение анализатора PVS-Studio: V3041 The expression was implicitly cast from 'long' type to 'float' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. CommunicationsUtilities.cs 593
Переменные now и s_lastLoggedTicks имеют тип long. Производятся вычисления, результатом которых должно быть значение типа float. Однако, так как операция деления производится над переменными типа long, и только после этого результат выражения приводится к типу float, произойдет потеря точности:
float millisecondsSinceLastLog =
(float)((now – s_lastLoggedTicks)/10000L);Правильный вариант данной конструкции:
float millisecondsSinceLastLog =
(float)(now – s_lastLoggedTicks)/10000;Следует всегда аккуратно относиться к вычислениям, в которых совместно используются целочисленные значения и значения с плавающей запятой.
Метод, который всегда возвращает true
Предупреждение анализатора PVS-Studio: V3009 It's odd that this method always returns one and the same value of 'true'. ComReference.cs 304
Метод GetTypeLibNameForITypeLib возвращает значение true при любых условиях:
internal static bool GetTypeLibNameForITypeLib(....)
{
....
if (typeLib2 == null)
{
....
return true; // <=
}
....
try
{
if (data == null || ...)
{
....
return true; // <=
}
....
}
catch (COMException ex)
{
....
return true; // <=
}
return true; // <=
}При этом возвращаемое методом GetTypeLibNameForITypeLib значение типа bool проверяется в вызывающем коде. Такое поведение может приводить к непредсказуемым последствиям. Необходимо провести рефакторинг кода и устранить ошибку.
Бессмысленное сравнение
Предупреждение анализатора PVS-Studio: V3022 Expression 'itemsAndMetadataFound.Metadata.Values.Count > 0' is always true. Evaluator.cs 1752
После того, как во внешнем блоке if выполняется проверка itemsAndMetadataFound.Metadata.Values.Count > 0, такая же проверка, уже бессмысленная, производится внутри блока:
if (itemsAndMetadataFound.Metadata != null &&
itemsAndMetadataFound.Metadata.Values.Count > 0)
{
....
if (itemsAndMetadataFound.Metadata.Values.Count > 0) // <=
{
needToProcessItemsIndividually = true;
}
....
}Помимо этой, в коде MSBuild было обнаружено еще 7 подобных ошибок:
Взаимоисключающие сравнения
Предупреждение анализатора PVS-Studio: V3011 Two opposite conditions were encountered. The second condition is always false. Check lines: 2840, 2838. XMake.cs 2840
Условием входа в блок if является равенство null переменной logger. Однако уже внутри блока в методе VerifyThrow используется проверка на неравенство null этой же переменной. Таким образом, проверка, производимая для метода VerifyThrow, будет всегда ложной:
if (logger == null)
{
InitializationException.VerifyThrow(logger != null, // <=
"LoggerNotFoundError", unquotedParameter);
}Сложно сказать, как должен выглядеть правильный код, но точно не так. Возможно, использование оператора if в данном случае вообще не требуется.
Неиспользуемые аргументы при форматировании строки
Предупреждение анализатора PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Arguments not used: 1st. Scheduler.cs 2216
Ошибка содержится во второй строке кода. Судя по всему, она была получена путем копирования первой строки (пресловутый copy-paste) и заменой в ней первого параметра. При этом второй, ставший ненужным параметр _schedulingData.EventTime.Ticks, удалить забыли:
file.WriteLine("Scheduler state at timestamp {0}:",
_schedulingData.EventTime.Ticks);
file.WriteLine("------------------------------------------------",
_schedulingData.EventTime.Ticks); // <=Таким образом, во второй строке кода ошибочно используется перегрузка метода WriteLine(string format, object arg0) вместо корректной.
Подобные найденные ошибки:
Неиспользуемый параметр
Предупреждение анализатора PVS-Studio: V3061 Parameter 'numericValue' is always rewritten in method body before being used. NodePacketTranslator.cs 320
Список формальных параметров метода содержит переменную numericValue, значение которой никогда не используется, так как сразу заменяется новым значением:
public void TranslateEnum<T>(ref T value, int numericValue)
{
numericValue = _reader.ReadInt32(); // <=
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}Возможно, производился рефакторинг кода, но сигнатуру метода (в отличие от его тела) было невозможно изменить. В противном случае имеет смысл произвести корректировку данного метода:
public void TranslateEnum<T>(ref T value)
{
int numericValue = _reader.ReadInt32();
Type enumType = value.GetType();
value = (T)Enum.ToObject(enumType, numericValue);
}Еще одно подобное предупреждение:
Лишнее присваивание
Предупреждение анализатора PVS-Studio: V3005 The '_nextProjectId' variable is assigned to itself. LoggingService.cs 325
Анализатор обнаружил конструкцию, в которой производится лишнее присваивание для поля _nextProjectId. Сначала вычисляется значение MaxCPUCount + 2, которое прибавляется к значению _nextProjectId и присваивается ему же оператором +=. А затем полученное значение еще раз присваивается полю _nextProjectId:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId = _nextProjectId += MaxCPUCount + 2; // <=
return _nextProjectId;
}
}
}В данном случае ошибки нет, но код выглядит странно. Эту конструкцию имеет смысл упростить:
public int NextProjectId
{
get
{
lock (_lockObject)
{
_nextProjectId += MaxCPUCount + 2;
return _nextProjectId;
}
}
}В заключение хочется отметить, насколько полезным может быть регулярное использование статических анализаторов кода, таких как PVS-Studio, для поиска потенциальных и реальных ошибок, даже в таких качественных проектах, как MSBuild.
Вы всегда можете повторить приведенные в данной статье примеры поиска ошибок, а также проверить собственные проекты при помощи демонстрационной версии анализатора PVS-Studio.