MSBuild – популярная сборочная платформа с открытым исходным кодом от Microsoft, которую используют разработчики по всему миру. В далёком 2016 году мы уже проверяли проект при помощи PVS-Studio и нашли несколько подозрительных мест. Давайте посмотрим, что удалось обнаружить в коде MSBuild при повторной проверке.

Введение

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

Для сравнения результатов анализа приведу две диаграммы:

Общее число срабатываний при повторной проверке составило 839, в прошлый раз их было всего 262. Больше, чем в четыре раза стало предупреждений уровня Medium, именно они превалируют в статье. На уровне Low их число увеличилось примерно в два с половиной раза. Почти вдвое больше стало срабатываний уровня High.

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

Приступим же к разбору наиболее интересных срабатываний.

Ошибочная инкрементация

Issue 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 increment for variable 'parsePoint' is senseless because this variable is overwritten. Scanner.cs 310

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

Следовательно, в метод будет передано изначальное значение parsePoint. Результат выполнения ScanForPropertyExpressionEnd присваивается переменной parsePoint, вследствие чего её увеличенное значение будет перезаписано. Получается, что в данном фрагменте кода операция инкремента вообще ни на что не влияет.

Возможным вариантом исправления будет изменение постфиксной записи на префиксную:

parsePoint = ScanForPropertyExpressionEnd(expression, ++parsePoint);

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

Issue 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 Expression 'leftConflictReference.IsPrimary && rightConflictReference.IsPrimary' is always false. ReferenceTable.cs 2388
V3063 A part of conditional expression is always true if it is evaluated: !isNonUnified. ReferenceTable.cs 2389
V3063 A part of conditional expression is always true if it is evaluated: !isNonUnified. ReferenceTable.cs 2390

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

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

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

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

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

Получается, значение этих переменных будет зависеть лишь от правого операнда оператора '&&'. Код можно упростить, изменив тело if на следующее:

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

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

Issue 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 A part of conditional expression is always false if it is evaluated: dllArchitecture == SystemProcessorArchitecture.MSIL. ReferenceTable.cs 2968

Переменная dllArchitecture инициализируется значением SystemProcessorArchitecture.None. Другое значение может быть присвоено ей лишь в теле switch. Рассмотрев его, можно понять, что SystemProcessorArchitecture.MSIL не присваивается ни в одном из блоков case. Стоит отметить, что (SystemProcessorArchitecture) 6 не соответствует элементу MSIL. В default-ветке в принципе нет присваивания этой переменной.

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

Также в коде есть комментарий, поясняющий часть условия: "If the assembly is MSIL or none it can work anywhere so there does not need to be any warning ect." Получается, что данная проверка была не случайной, что делает код очень подозрительным.

Issue 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 The '_toolsetProvider' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 284, 282. BuildParameters.cs 284

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

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

Путаница с аргументами

Issue 5

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

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'SdkResult' constructor: 'sdkResult.Warnings' and '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. Скорее всего, здесь был перепутан порядок аргументов, так как тяжело представить ситуацию, в которой предупреждение и ошибка будут взаимозаменяемы.

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

Issue 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 The 'project' object was used after it was verified against null. Check lines: 2446, 2439. Engine.cs 2446

Переменная project была проверена на null в условии:

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

В следующем условии обращаются к свойству project.FullFileName. Проблема заключается в том, что здесь project на null уже не проверяют. Странно, что буквально семь строчек кода назад для переменной предполагалась возможность значения null, а сейчас – нет.

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

Issue 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 The 'item' object was used after it was verified against null. Check lines: 139, 134. BuildItemCacheEntry.cs 139

В теле foreach переменная item проверяется на null. В случае, если item имеет значение null, в поток записывается 0. Дальше без какого-либо условия в поток запишется 1, а потом... Потом будет выброшено исключение типа NullReferenceException. Произойдёт это из-за вызова WriteToStream у item.

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

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

Issue 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 Enumerating the result of null-conditional access operator can lead to NullReferenceException. Consider inspecting: properties?.Keys. MockEngine.cs 165

Обратите внимание на коллекцию, по которой производят итерирование в foreach. Она получена с использованием оператора '?.'. Возможно, разработчик предполагал, что, если properties имеет значение null, код в теле foreach просто не выполнится. Он был прав, но есть некоторая проблема — будет выброшено исключение.

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

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

Issue 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 The 'propertyValue' object was used after it was verified against null. Check lines: 3301, 3253. Expander.cs 3301
V3095 The 'propertyValue' object was used before it was verified against null. Check lines: 3301, 3324. Expander.cs 3301

По сути, оба срабатывания указывают на одну проблему. Рассмотрим условие первого if. Его часть состоит из проверки propertyValue на null. Это говорит о том, что разработчик предполагал такое значение для этой переменной. Может возникнуть ситуация, когда propertyValue == null будет true, а другая часть условия – false. Следовательно, выполнится код в else-ветви этого if. В этой ветви произойдет разыменование нулевой ссылки при вызове метода propertyValue.GetType. Также стоит отметить, что и далее перед вызовом метода у propertyValue проверка на null присутствует.

Заключение

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

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

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Panevin. Build to order? Checking MSBuild for the second time.

Соберёмся? Вторая проверка проекта MSBuild