Сборка на заказ? Проверка MSBuild во второй раз

MSBuild — это популярная платформа сборки с открытым исходным кодом, созданная компанией Microsoft. Разработчики по всему миру используют MSBuild. В 2016 году мы проверили ее в первый раз и нашли несколько подозрительных мест. Сможем ли мы найти что-нибудь в этот раз? Давайте посмотрим!

Введение

С момента предыдущей проверки проект сильно вырос. Наш анализатор тоже стал более продвинутым. Это делает задачу еще интереснее! Несмотря на высокое качество продукта MSBuild и известное имя его создателя, нам снова удалось найти некоторые проблемы в исходном коде MSBuild. Проект практически полностью написан на C#. Вы можете ознакомиться с ним на GitHub. Мы взяли код из этого коммита.

Чтобы сравнить результаты анализа, давайте посмотрим на две диаграммы:

После второй проверки анализатор выдал 839 предупреждений. В прошлый раз их было всего 262. Количество предупреждений среднего уровня увеличилось в четыре раза. Предупреждения этого уровня уверенности преобладают в нашей статье. Количество предупреждений Низкого уровня увеличилось примерно в два с половиной раза. Предупреждения высокого уровня увеличились почти в два раза.

С момента первой проверки прошло шесть лет — и мы, разработчики PVS-Studio, не теряли времени даром :). С момента первой проверки MSBuild мы добавили в анализатор C# 64 GA (General Analysis) и 23 OWASP диагностики. Мы также улучшили существующие диагностические правила. Но не только разработчики C# проделали значительную работу. Если вы хотите проследить, как менялся анализатор — нажмите здесь.

Давайте рассмотрим самые интересные предупреждения.

Неправильный инкремент

Проблема 1

private string ParsePropertyOrItemMetadata()
{
  int start = parsePoint;
  parsePoint++;

  if (parsePoint < expression.Length && expression[parsePoint] != '(')
  {
    errorState = true;
    errorPosition = start + 1;
    errorResource = "IllFormedPropertyOpenParenthesisInCondition";
    unexpectedlyFound = Convert
                        .ToString(expression[parsePoint],
                                  CultureInfo.InvariantCulture);
    return null;
  }

  parsePoint = ScanForPropertyExpressionEnd(expression, parsePoint++); // <=
  ....
}

Предупреждение PVS-Studio: V3133 Инкремент Postfix для переменной ‘parsePoint’ не имеет смысла, так как эта переменная перезаписывается. Scanner.cs 310

Возможно, разработчик ожидал, что ScanForPropertyExpressionEnd примет инкрементированное значение parsePoint в качестве второго аргумента. К сожалению, этого не произойдет. Проблема заключается в использовании постфиксной нотации для инкремента. В этом случае сначала возвращается текущее значение переменной, и только потом происходит ее инкремент.

Поэтому в метод передается начальное значение parsePoint. Значение, полученное после выполнения ScanForPropertyExpressionEnd, присваивается переменной parsePoint. Из-за этого увеличенное значение переменной перезаписывается. Таким образом, операция инкремента ни на что не влияет в этом фрагменте кода.

Эту проблему можно исправить, заменив постфиксную нотацию на префиксную:

parsePoint = ScanForPropertyExpressionEnd(expression, ++parsePoint);

Подозрительные логические выражения

Проблема 2

private static int ResolveAssemblyNameConflict(...., ....);
{
  ....
  if (   leftConflictReference.IsPrimary 
      && !rightConflictReference.IsPrimary)
  {
    ....  
  }
  else if (   !leftConflictReference.IsPrimary 
           && rightConflictReference.IsPrimary)
  {
    ....  
  }
  else if (   !leftConflictReference.IsPrimary 
           && !rightConflictReference.IsPrimary)
  {
    ....
    bool isNonUnified =   leftConflictReference.IsPrimary   // <=
                       && rightConflictReference.IsPrimary; // <=

    bool leftConflictLegacyUnified =   !isNonUnified        // <=
                                    && assemblyReference0
                                       .reference
                                       .IsPrimary;

    bool rightConflictLegacyUnified =    !isNonUnified      // <=
                                      && assemblyReference1
                                         .reference
                                         .IsPrimary;
    ....
  }
}

Анализатор выдал три предупреждения для этого фрагмента кода:

• V3022 Выражение ‘leftConflictReference.IsPrimary && rightConflictReference.IsPrimary’ всегда ложно. ReferenceTable.cs 2388
• V3063 Часть условного выражения всегда истинна, если вычислено: !isNonUnified. ReferenceTable.cs 2389
• V3063 Часть условного выражения всегда истинна, если оценивается: !isNonUnified. ReferenceTable.cs 2390

Второе и третье предупреждения являются следствием проблемы, обозначенной первым предупреждением. Давайте посмотрим на условие последнего if. Как мы видим, значения leftConflictReference.IsPrimary и rightConflictReference.IsPrimary в теле if всегда ложны.

Переменная isNonUnified инициализируется значением, полученным после выполнения leftConflictReference.IsPrimary && rightConflictReference.IsPrimary. Обе эти переменные равны false. Поэтому isNonUnified всегда ложна.

Затем isNonUnified используется как часть выражения для инициализации еще двух переменных:

bool leftConflictLegacyUnified =   !isNonUnified 
                                && assemblyReference0.reference
                                                     .IsPrimary;

bool rightConflictLegacyUnified =    !isNonUnified 
                                  && assemblyReference1.reference
                                                       .IsPrimary;

Таким образом, значение этих переменных зависит только от правого операнда оператора ‘&&’. Код можно упростить, заменив тело if на следующее:

bool leftConflictLegacyUnified = assemblyReference0.reference.IsPrimary;
bool rightConflictLegacyUnified = assemblyReference1.reference.IsPrimary;

Скорее всего, код не содержит ошибок, просто в нем есть несколько лишних операций. Однако мы не можем игнорировать предупреждение анализатора — это не ложное срабатывание. Мой коллега по команде написал об этом статью, очень рекомендую вам ее прочитать.

Проблема 3

private bool VerifyArchitectureOfImplementationDll(string dllPath,
                                                   string winmdFile)
{
  try
  {
    UInt16 machineType = _readMachineTypeFromPEHeader(dllPath);
    SystemProcessorArchitecture dllArchitecture = 
                                  SystemProcessorArchitecture.None;
    switch (machineType)
    {
      case NativeMethods.IMAGE_FILE_MACHINE_AMD64:
        dllArchitecture = SystemProcessorArchitecture.Amd64;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_ARM:
      case NativeMethods.IMAGE_FILE_MACHINE_ARMV7:
        dllArchitecture = SystemProcessorArchitecture.Arm;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_ARM64:
        dllArchitecture = (SystemProcessorArchitecture) 6; 
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_I386:
        dllArchitecture = SystemProcessorArchitecture.X86;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_IA64:
        dllArchitecture = SystemProcessorArchitecture.IA64;
        break;
      case NativeMethods.IMAGE_FILE_MACHINE_UNKNOWN:
        dllArchitecture = SystemProcessorArchitecture.None;
        break;
      default:
        ....
        break;
    }

    // If the assembly is MSIL or none it can work anywhere
    // so there does not need to be any warning ect.
    if (   dllArchitecture == SystemProcessorArchitecture.MSIL     // <=
        || dllArchitecture == SystemProcessorArchitecture.None)
    {
      return true;
    }
    ....
  }
}

Предупреждение PVS-Studio: V3063 Часть условного выражения всегда ложна, если оценивается: dllArchitecture == SystemProcessorArchitecture.MSIL. ReferenceTable.cs 2968

Переменная dllArchitecture инициализируется значением SystemProcessorArchitecture.None. Этой переменной может быть присвоено другое значение только в теле переключателя. Если присмотреться, можно заметить, что SystemProcessorArchitecture.MSIL не присваивается ни в одном из блоков case. Обратите внимание, что (SystemProcessorArchitecture) 6 не соответствует элементу MSIL. В ветви по умолчанию нет присвоения этой переменной.

Ниже переключателя есть проверка, что dllArchitecture равна SystemProcessorArchitecture.MSIL. Выглядит странно — dllArchitecture не может иметь такое значение.

Код также содержит комментарий, который объясняет часть условия: «Если сборка является MSIL или не является таковой, то она может работать где угодно, поэтому не нужно никакого предупреждения ect.». Таким образом, проверка не была случайной. Это делает код очень подозрительным.

Вопрос 4

Можете ли вы найти здесь ошибку?

internal BuildParameters(BuildParameters other, bool resetEnvironment = false)
{
  ErrorUtilities.VerifyThrowInternalNull(other, nameof(other));
  _buildId = other._buildId;
  _culture = other._culture;
  _defaultToolsVersion = other._defaultToolsVersion;
  _enableNodeReuse = other._enableNodeReuse;
  _buildProcessEnvironment = resetEnvironment
    ? CommunicationsUtilities.GetEnvironmentVariables()
    : other._buildProcessEnvironment != null
      ? new Dictionary<string, string>(other._buildProcessEnvironment)
      : null;
  _environmentProperties = ....
  _forwardingLoggers = ....
  _globalProperties = ....
  HostServices = other.HostServices;
  _loggers = other._loggers != null ? new List<ILogger>(other._loggers) : null;
  _maxNodeCount = other._maxNodeCount;
  _memoryUseLimit = other._memoryUseLimit;
  _nodeExeLocation = other._nodeExeLocation;
  NodeId = other.NodeId;
  _onlyLogCriticalEvents = other._onlyLogCriticalEvents;
  BuildThreadPriority = other.BuildThreadPriority;
  _toolsetProvider = other._toolsetProvider;
  ToolsetDefinitionLocations = other.ToolsetDefinitionLocations;
  _toolsetProvider = other._toolsetProvider;
  _uiCulture = other._uiCulture;
  DetailedSummary = other.DetailedSummary;
  _shutdownInProcNodeOnBuildFinish = other._shutdownInProcNodeOnBuildFinish;
  ProjectRootElementCache = other.ProjectRootElementCache;
  ResetCaches = other.ResetCaches;
  LegacyThreadingSemantics = other.LegacyThreadingSemantics;
  SaveOperatingEnvironment = other.SaveOperatingEnvironment;
  _useSynchronousLogging = other._useSynchronousLogging;
  _disableInProcNode = other._disableInProcNode;
  _logTaskInputs = other._logTaskInputs;
  _logInitialPropertiesAndItems = other._logInitialPropertiesAndItems;
  WarningsAsErrors = ....
  WarningsNotAsErrors = ....
  WarningsAsMessages = ....
  _projectLoadSettings = other._projectLoadSettings;
  _interactive = other._interactive;
  _isolateProjects = other._isolateProjects;
  _inputResultsCacheFiles = other._inputResultsCacheFiles;
  _outputResultsCacheFile = other._outputResultsCacheFile;
  DiscardBuildResults = other.DiscardBuildResults;
  LowPriority = other.LowPriority;
  ProjectCacheDescriptor = other.ProjectCacheDescriptor;
}

Что-то мне подсказывает, что вы либо не нашли ее, либо нашли, но потратили часы на поиски. Давайте немного сократим этот фрагмент кода:

internal BuildParameters(BuildParameters other, bool resetEnvironment = false)
{
  ....
  _toolsetProvider = other._toolsetProvider;
  ToolsetDefinitionLocations = other.ToolsetDefinitionLocations;
  _toolsetProvider = other._toolsetProvider;
  ....
}

Предупреждение PVS-Studio: V3008 Переменной ‘_toolsetProvider’ дважды подряд присваиваются значения. Возможно, это ошибка. Проверьте строки: 284, 282. BuildParameters.cs 284

Теперь вы можете легко найти здесь проблему. Полю _toolsetProvider дважды присваивается значение. Это абсолютно бессмысленно. Трудно сказать, действительно ли это ошибка. Неясно, должно ли быть что-то другое вместо одного из присвоений _toolsetProvider. Возможно, это лишнее назначение, но лучше избегать таких случаев.

Эта проблема — хороший пример того, как статический анализ может помочь. Человеческий глаз почти всегда не найдет проблему в таком коде, а вот статический анализатор — нет.

Перепутанные аргументы

Проблема 5

private SdkResult CloneSdkResult(SdkResult sdkResult)
{
  if (!sdkResult.Success)
  {
    return new SdkResult(sdkResult.SdkReference, 
                         sdkResult.Warnings, 
                         sdkResult.Errors);
  }
  ....
}

Предупреждение PVS-Studio: V3066 Возможен неправильный порядок аргументов, переданных конструктору ‘SdkResult’: ‘sdkResult.Warnings’ и ‘sdkResult.Errors’. InternalEngineHelpers.cs 83

Чтобы понять это предупреждение, нам нужно сначала просмотреть объявление конструктора SdkResult:

public SdkResult(SdkReference sdkReference,
                 IEnumerable<string> errors,
                 IEnumerable<string> warnings)
{
  Success = false;
  SdkReference = sdkReference;
  Errors = errors;
  Warnings = warnings;
}

Довольно редкое и интересное предупреждение. Обычно оно указывает на серьезную ошибку. Судя по названиям параметров, можно сделать вывод, что второй параметр — это набор ошибок, а третий — набор предупреждений. Теперь понятно, почему анализатор выдал предупреждение. При создании объекта в методе CloneSdkResult в качестве второго аргумента передается sdkResult.Warnings, а в качестве третьего — sdkResult.Errors. Скорее всего, здесь был перепутан порядок аргументов — трудно представить ситуацию, когда предупреждение и ошибка были бы взаимозаменяемы.

Потенциальное разыменование нуля

Проблема 6

private BuildRequest CreateLocalBuildRequest(...., Project project, ....)
{
  ....
  BuildRequest buildRequest =  new BuildRequest(....)
  ....
  if (String.IsNullOrEmpty(toolsVersion) && project != null)  // <=
  {
    buildRequest.ToolsetVersion = project.ToolsVersion;
  }

  if (buildRequest.ProjectFileName == null)
  {
    buildRequest.ProjectFileName = project.FullFileName;     // <=
  }

  return buildRequest;
}

Предупреждение PVS-Studio: V3125 Объект ‘project’ был использован после проверки на соответствие null. Проверьте строки: 2446, 2439. Engine.cs 2446

В этом условии переменная project проверяется на null:

if (String.IsNullOrEmpty(toolsVersion) && project != null)

Следующее условие обращается к свойству project.FullFileName. Но там переменная project не проверяется на null — отсюда и проблема. Это странно: разработчик подозревает, что переменная может быть null семью строками кода выше этой, но не подозревает об этом сейчас.

Стоит отметить, что состояние переменной не может измениться, а buildRequest.ProjectFileName никак не связано с проектом. Разыменование нулевой ссылки приведет к NullReferenceException.

Выпуск 7

internal override void WriteToStream(BinaryWriter writer)
{
  base.WriteToStream(writer);
  if (buildItems == null)
  {
    writer.Write((byte)0);
  }
  else
  {
    ....
    foreach (BuildItem item in buildItems)
    {
      if (item == null)
      {
        writer.Write((byte)0);                    // <=
      }
       writer.Write((byte)1);
       item.WriteToStream(writer);                // <=
    }
  }
}

Предупреждение PVS-Studio: V3125 Объект ‘item’ был использован после проверки на null. Проверьте строки: 139, 134. BuildItemCacheEntry.cs 139

В теле foreach переменная item проверяется на null. Если item равен null, в поток записывается 0. Затем, без какого-либо условия, в поток записывается 1, а затем… Затем будет выброшен NullReferenceException. Это произойдет из-за вызова writeToStream элемента.

Возможно, здесь не хватает блока else. Ниже приведен возможный способ устранения ошибки:

if (item == null)
{
  writer.Write((byte)0);
}
else
{
  writer.Write((byte)1);
  item.WriteToStream(writer)
}

Выпуск 8

public void LogTelemetry(string eventName,
                         IDictionary<string, string> properties)
{
  ....
  foreach (string key in properties?.Keys)                                // <=
  {
    message += $"  Property '{key}' = '{properties[key]}'{Environment.NewLine}";
  }
  ....
}

Предупреждение PVS-Studio: V3153 Перечисление результата оператора доступа с нулевым условием может привести к NullReferenceException. Рассмотрим проверку: properties?Keys. MockEngine.cs 165

В приведенном выше коде блок foreach выполняет итерацию по коллекции. Чтобы получить эту коллекцию, оператор foreach использует оператор ‘?’. Разработчик мог предположить, что если properties равно null, то код в теле foreach просто не будет выполняться. Хотя это и верно, возникает проблема — будет выброшено исключение.

Для итерируемой коллекции вызывается метод GetEnumerator. Нетрудно догадаться, к чему приведет вызов этого метода для переменной, несущей нулевое значение.

Более подробный анализ подобных проблем вы можете найти в этой статье.

Проблема 9

internal static Function<T> ExtractPropertyFunction(
                string expressionFunction,
                IElementLocation elementLocation,
                object propertyValue,
                UsedUninitializedProperties usedUnInitializedProperties,
                IFileSystem fileSystem)
{
  ....
  if (propertyValue == null && expressionRoot[0] == '[')           // <=
  {
    ....
  }
  else if (expressionFunction[0] == '[')
  {
    ....
    functionBuilder.ReceiverType = propertyValue.GetType();        // <=
    ....
  }
  else
  {
    ....
    if (propertyValue == null && !IsValidPropertyName(functionReceiver))
    {
      ProjectErrorUtilities
      .ThrowInvalidProject(elementLocation,
                           "InvalidFunctionPropertyExpression",
                            expressionFunction, String.Empty);
    }
    var receiverType = propertyValue?.GetType() ?? typeof(string); // <=
    ....
  }
  ....
}

Анализатор выдал два предупреждения для этого участка кода:

• V3125 Объект ‘propertyValue’ был использован после того, как он был проверен на соответствие null. Проверьте строки: 3301, 3253. Expander.cs 3301
• V3095 Объект ‘propertyValue’ использовался до того, как был проверен на соответствие null. Проверьте строки: 3301, 3324. Expander.cs 3301

На самом деле оба эти предупреждения указывают на одну и ту же проблему. Давайте посмотрим на условие первого if. Часть этого условия проверяет propertyValue на null. Это означает, что разработчик ожидал, что это значение может быть null. Возможен случай, когда propertyValue == null истинно, а вторая часть условия ложна. Поэтому будет выполнена ветвь else. В этой ветви ссылка на null будет разыменована при вызове метода propertyValue.GetType. Также стоит отметить, что далее, перед вызовом метода*, PropertyValue* проверяется на null.

Заключение

В этой статье мы описали не только проблемы, диагностики для которых не существовало, когда мы впервые проверяли MSBuild, но и предупреждения относительно старых диагностик.

Очевидно, что новые диагностики помогли найти ошибки, которые мы не увидели при первой проверке. Старые диагностики используют основные механизмы. Мы постоянно совершенствуем эти механизмы для достижения высокого качества анализа, поэтому старые диагностики выдают новые предупреждения.

Вы можете задать вопрос: «Почему вы описали только 9 предупреждений?». Мы хотели показать вам самые интересные из них, не делая статью скучной.

И последнее, но не менее важное, мы хотели бы отметить труд разработчиков MSBuild — они действительно заботятся о качестве проекта.

Мы постоянно работаем над улучшением PVS-Studio: добавляются новые диагностики, модифицируются старые. Это позволяет находить больше участков кода, которые могут быть опасны для вашей программы. Вы можете попробовать PVS-Studio бесплатно и посмотреть, что он может найти в вашем проекте.