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

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

![0986_MSBuild_ru/image1.png](https://import.viva64.com/docx/blog/0986_MSBuild_ru/image1.png)

## Введение

С момента [предыдущей проверки](https://pvs-studio.ru/ru/blog/posts/csharp/0424/) проект сильно вырос, да и анализатор стал более совершенным\. Тем интереснее задача\! Несмотря на высокое качество продукта и известное имя разработчика, в этот раз нам снова удалось обнаружить в исходниках MSBuild несколько интересных дефектов\. Код проекта практически полностью написан на C\# и доступен на [GitHub](https://github.com/dotnet/msbuild/tree/c1b61ebeaa0f77d1382c6a0734024557ecf255c3)\. Состояние кода соответствует [коммиту](https://github.com/dotnet/msbuild/tree/c1b61ebeaa0f77d1382c6a0734024557ecf255c3)\.

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

![0986_MSBuild_ru/image2.png](https://import.viva64.com/docx/blog/0986_MSBuild_ru/image2.png)

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

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

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

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

**Issue 1**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3133/) Postfix increment for variable 'parsePoint' is senseless because this variable is overwritten\. Scanner\.cs 310

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

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

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

```cpp
parsePoint = ScanForPropertyExpressionEnd(expression, ++parsePoint);
```

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

**Issue 2**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'leftConflictReference\.IsPrimary && rightConflictReference\.IsPrimary' is always false\. ReferenceTable\.cs 2388 
* [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: \!isNonUnified\. ReferenceTable\.cs 2389
* [V3063](https://pvs-studio.ru/ru/docs/warnings/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_ используется в качестве части выражения для инициализации ещё двух переменных:

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

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

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

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

Скорее всего, код не содержит ошибку, просто разработчик произвёл ряд лишних операций\. Но и предупреждение анализатора ложным назвать нельзя\. Как раз на этот счёт недавно была написана [заметка](https://pvs-studio.ru/ru/blog/posts/cpp/0968/), рекомендую с ней ознакомиться\.

**Issue 3**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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**

Попробуйте найти ошибку в следующем коде:

```cpp
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;
}
```

Что\-то мне подсказывает, что обнаружить проблему вам либо не удалось, либо на это было потрачено приличное количество времени\. Сократим вышеописанный код:

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

Предупреждение PVS\-Studio: [V3008](https://pvs-studio.ru/ru/docs/warnings/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**

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

Предупреждение PVS\-Studio: [V3066](https://pvs-studio.ru/ru/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'SdkResult' constructor: 'sdkResult\.Warnings' and 'sdkResult\.Errors'\. InternalEngineHelpers\.cs 83

Чтобы разобраться в данном срабатывании, необходимо рассмотреть определение конструктора _SdkResult_:

```cpp
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**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'project' object was used after it was verified against null\. Check lines: 2446, 2439\. Engine\.cs 2446

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

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

В следующем условии обращаются к свойству _project\.FullFileName_\. Проблема заключается в том, что здесь _project_ на _null_ уже не проверяют\. Странно, что буквально семь строчек кода назад для переменной предполагалась возможность значения _null_, а сейчас – нет\.

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

**Issue 7**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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\. Возможный вариант исправления ошибки:

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

**Issue 8**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/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_\.

Более подробный разбор таких проблем вы можете найти в [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0832/)\.

**Issue 9**

```cpp
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](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'propertyValue' object was used after it was verified against null\. Check lines: 3301, 3253\. Expander\.cs 3301
* [V3095](https://pvs-studio.ru/ru/docs/warnings/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](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.