Сергей Хренов

24 Авг 2016

Теги:

#CSharp

Проверяем исходный код MSBuild с помощью PVS-Studio

24 Авг 2016

Автор: Сергей Хренов

Введение
Исходные данные и общая статистика проверки
Результаты проверки
Заключение

Работая над развитием статического анализатора исходного кода 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 подобных потенциально небезопасных конструкций:

V3095 The 'propertyValue' object was used before it was verified against null. Check lines: 2760, 2799. Expander.cs 2760
V3095 The 'publicKeyToken' object was used before it was verified against null. Check lines: 232, 236. GenerateBindingRedirects.cs 232
V3095 The 'searchLocation' object was used before it was verified against null. Check lines: 170, 178. Resolver.cs 170
V3095 The 'assemblyName' object was used before it was verified against null. Check lines: 176, 194. Resolver.cs 176
V3095 The 'searchLocation' object was used before it was verified against null. Check lines: 249, 264. Resolver.cs 249
V3095 The 'ReferenceInfo' object was used before it was verified against null. Check lines: 87, 97. AxReference.cs 87
V3095 The 'packageFileName' object was used before it was verified against null. Check lines: 1448, 1457. BootstrapperBuilder.cs 1448
V3095 The 'metadataNames' object was used before it was verified against null. Check lines: 243, 253. CommandLineBuilderExtension.cs 243

Ошибочное предположение о длине строки

Предупреждение анализатора 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);
}

Подобные ошибки в коде:

V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. SolutionFile.cs 1217
V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. XMake.cs 2929
V3057 The 'Remove' function could receive the '-1' value while non-negative value is expected. Inspect the first argument. BootstrapperBuilder.cs 767

Приведение типа с потерей точности

Предупреждение анализатора 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 подобных ошибок:

V3022 Expression 'fixedPathInfo != null' is always true. FrameworkLocationHelper.cs 794
V3022 Expression '_shutdownException != null' is always false. InProcNode.cs 527
V3022 Expression 'proj != null' is always true. SolutionFile.cs 817
V3022 Expression '_directMetadata == null' is always false. ProjectItem.cs 755
V3022 Expression 'Constants.defaultToolsVersion == "2.0"' is always true. ToolsetReader.cs 194
V3022 Expression '!isQuotedTransform && functionCapture == null' is always true. ExpressionShredder.cs 281
V3022 Expression '!isQuotedTransform && functionCapture == null' is always true. ExpressionShredder.cs 414

Взаимоисключающие сравнения

Предупреждение анализатора 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) вместо корректной.

Подобные найденные ошибки:

V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 75
V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 82
V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 91
V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Arguments not used: resource. XmlUtil.cs 112

Неиспользуемый параметр

Предупреждение анализатора 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);
}

Еще одно подобное предупреждение:

V3061 Parameter 'defaultToolsVersion' is always rewritten in method body before being used. ToolsetProvider.cs 118

Лишнее присваивание

Предупреждение анализатора 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.

#CSharp