# В Microsoft тоже ошибаются\. Проверяем MSBuild

MSBuild — это платформа автоматизированной сборки приложений от Microsoft, которая используется для компиляции, упаковки и развёртывания приложений\. Проект пользуется большой популярностью среди разработчиков, и мы сами активно применяем его в работе нашего анализатора\. В статье мы расскажем о потенциальных ошибках, которые нам удалось выявить в продукте от Microsoft\.

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

## Введение

Это третья наша проверка проекта MSBuild, [первая](https://pvs-studio.ru/ru/blog/posts/csharp/0424/) была в 2016 году, а [вторая](https://pvs-studio.ru/ru/blog/posts/csharp/0986/) — в 2022 году\. 

Состояние кода проекта на момент анализа соответствует [этому коммиту](https://github.com/dotnet/msbuild/tree/7e539c169a657040eeb768e8115c44f1780e59df)\.

В течение этого времени Microsoft активно развивала свой продукт, что даёт много нового материала для анализа\. Мы проводим повторную проверку не только из\-за увеличения количества строк кода в самом проекте, но и потому, что наш анализатор активно развивается\. Например, менее чем за 2\.5 года в PVS\-Studio было добавлено более 40 C\# диагностик\. Помимо этого, было улучшено множество старых диагностик и механизмов анализа\.

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

## Issue 1: Некорректная строка формата

```cpp
private int QueryConsoleBufferWidth()
{
  int consoleBufferWidth = -1;
  try
  {
    consoleBufferWidth = Console.BufferWidth;
  }
  catch (Exception ex)
  {
    CommunicationsUtilities.Trace("MSBuild client warning:" +
            "problem during querying console buffer width.", ex);
  }

  return consoleBufferWidth;
}
```

Сообщение анализатора PVS\-Studio: [V3025](https://pvs-studio.ru/ru/docs/warnings/v3025/)\. The 1st argument '"MSBuild client warning: problem during querying console buffer width\."' is used as incorrect format string inside method\. A different number of format items is expected while calling 'Trace' function\. Arguments not used: 1st\. MSBuildClient\.cs 394

Анализатор сообщает о некорректной строке формата, передаваемой в метод `Trace`\.

Для полной ясности рассмотрим метод `Trace`:

```cpp
internal static void Trace<T>(string format, T arg0)
{
    Trace(nodeId: -1, format, arg0);
}

internal static void Trace<T>(int nodeId, string format, T arg0)
{
    if (s_trace)
    {
        TraceCore(nodeId, string.Format(format, arg0));
    }
}
```

В метод `Trace` передаётся строка, в нём она используется в качестве строки формата для `string.Format(format, arg0)`\. Эта строка должна содержать плейсхолдеры для `arg0`\. Однако в приведённом выше коде строка формата, переданная в `Trace`, не содержит их, что приводит к потере сообщения об исключении\.

## Issue 2: Неправильная отписка от события

```cpp
private static void SubscribeImmutablePathsInitialized()
{
  NotifyOnScopingReadiness?.Invoke();

  FileClassifier.Shared.OnImmutablePathsInitialized -= () =>
    NotifyOnScopingReadiness?.Invoke();
}
```

Сообщение анализатора: [V3084](https://pvs-studio.ru/ru/docs/warnings/v3084/)\. Anonymous function is used to unsubscribe from 'OnImmutablePathsInitialized' event\. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration\. CheckScopeClassifier\.cs 67

В этом случае отписка от делегата не будет выполнена, поскольку каждый раз, когда объявляется анонимная функция, создаётся новый экземпляр делегата\. Попытка отписаться от этого экземпляра не приведёт к желаемому результату\. Условно `OnImmutablePathsInitialized` подписан на 'делегат 1', а отписывается от 'делегата 2', что ничего не поменяет\.

## Issue 3: WriteLine вместо Write

```cpp
public override void WriteLine(string format, params object?[] arg)
{
  lock (_lock)
  {
    _internalWriter.WriteLine(format, arg);                          // <=
  }
}

public override void Write(string format, object? arg0)
{
  lock (_lock)
  {
    _internalWriter.Write(format, arg0);
  }
}

public override void Write(string format, object? arg0, object? arg1)
{
  lock (_lock)
  {
    _internalWriter.Write(format, arg0, arg1);
  }
}

public override void Write(string format, object? arg0,
                            object? arg1, object? arg2)
{
  lock (_lock)
  {
    _internalWriter.Write(format, arg0, arg1, arg2);
  }
}

public override void Write(string format, params object?[] arg)
{
  lock (_lock)
  {
    _internalWriter.WriteLine(format, arg);                          // <=
  }
}
```

Предупреждение PVS\-Studio: [V3013](https://pvs-studio.ru/ru/docs/warnings/v3013/)\. It is odd that the body of 'Write' function is fully equivalent to the body of 'WriteLine' function \(594, line 738\)\. OutOfProcServerNode\.cs 594

Анализатор посчитал странным, что у методов `WriteLine` и `Write` полностью одинаковое тело\.

Если посмотреть на другие перегрузки `Write`, то мы увидим, что используется `_internalWriter.Write`, в то время как для `void Write(string format, params object?[] arg)` используется `_internalWriter.WriteLine`\. Очень вероятно, что это опечатка или результат использования Copy\-Paste\.

## Issue 4: Пустая проверка на null

```cpp
private void ProcessParameters()
{
  if (Parameters == null)
  {
  }

  string[] parameters = Parameters.Split(s_semicolonChar);

  if (parameters.Length != 1)
  {
    ....
  }

  _logFile = parameters[0];
}
```

Сообщение анализатора: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/)\. The 'Parameters' object was used after it was verified against null\. Check lines: 91, 87\. TaskUsageLogger\.cs 91

Анализатор обнаружил, что свойство `Parameters` используется после проверки на `null`\.

При загадочных обстоятельствах тело оператора `if` оказалось пустым\. Это делает дальнейшее использование `Parameters` небезопасным\. Более того, поиск похожего кода в другом файле выявил метод с таким же названием и похожим телом\.

Схожий код в файле XmlFileLogger\.cs:

```cpp
private void ProcessParameters()
{
  ....
  if (Parameters == null)
  {
    throw new LoggerException(invalidParamSpecificationMessage);
  }

  string[] parameters = Parameters.Split(';');

  if (parameters.Length != 1)
  {
    throw new LoggerException(invalidParamSpecificationMessage);
  }

  _logFile = parameters[0];
}
```

Как мы видим, здесь также выполняется проверка `Parameters` на `null`, но после этого производится генерация исключения\. Вероятно, такая реализация должна быть и у проверки в первом блоке кода\.

## Issue 5: Подозрительная проверка на null

```cpp
private async Task<WorkUnitResult> InitializeAndExecuteTask(....)
{
  if (!_taskExecutionHost.InitializeForBatch(....))
  {
    ProjectErrorUtilities.ThrowInvalidProject(_targetChildInstance.Location,
                                              "TaskDeclarationOrUsageError",
                                              _taskNode.Name);
  }

  using var assemblyLoadsTracker = 
    AssemblyLoadsTracker.StartTracking(taskLoggingContext,
                                   AssemblyLoadingContext.TaskRun,
                                   _taskExecutionHost?.TaskInstance?.GetType());
}
```

Предупреждение PVS\-Studio: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/)\. The '\_taskExecutionHost' object was used before it was verified against null\. Check lines: 650, 655\. TaskBuilder\.cs 650

После вызова метода `_taskExecutionHost.InitializeForBatch` в операторе `if` идёт проверка на `null` в следующем виде: `_taskExecutionHost?.TaskInstance?.GetType`\. Однако, если значение `_taskExecutionHost` равно `null`, эта проверка не предотвратит возникновение исключения, поскольку поле уже используется до неё\. Чтобы обеспечить надежность, следует добавить такую проверку и для выражения в операторе `if`\.

## Issue 6: Ошибка приоритетов операторов

```cpp
public bool Equals(TaskItem other)
{
  ....
  int capacity = _itemDefinitions?.Count ?? 0 + _directMetadata?.Count ?? 0;
  var thisNames = new HashSet<string>(capacity,     
                                      MSBuildNameIgnoreCaseComparer.Default);
  ....
}
```

Сообщение анализатора PVS\-Studio: [V3123](https://pvs-studio.ru/ru/docs/warnings/v3123/)\. Perhaps the '??' operator works in a different way than it was expected\. Its priority is lower than priority of other operators in its left part\. ProjectItemInstance\.cs 1628

Вероятно, из\-за приоритета оператора `??` выражение в правой части присваивания `capacity` работает не так, как ожидает разработчик\.

Значения `_itemDefinitions?.Count` и `_directMetadata?.Count` никогда не будут суммироваться, поскольку без скобок приоритет операторов будет следующим:

```cpp
int capacity = _itemDefinitions?.Count ??((0 + _directMetadata?.Count)?? 0);
```

Возможно, разработчики хотели, чтобы при `null` подставлялось значение 0 для суммирования, но не учли приоритеты операторов\.

В таком случае корректный код должен выглядеть так:

```cpp
int capacity = (_itemDefinitions?.Count ?? 0) + (_directMetadata?.Count ?? 0);
```

## Issue 7: Дублирование присваивания

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

```cpp
internal ProjectInstance(string projectFile, 
                         ProjectInstance projectToInheritFrom,
                         IDictionary<string, string> globalProperties)
{
  _projectFileLocation = ElementLocation.Create(projectFile);
  _globalProperties = new PropertyDictionary<ProjectPropertyInstance>(
    globalProperties.Count);
  this.Toolset = projectToInheritFrom.Toolset;
  this.SubToolsetVersion = projectToInheritFrom.SubToolsetVersion;
  _explicitToolsVersionSpecified =
    projectToInheritFrom._explicitToolsVersionSpecified;
  _properties = new PropertyDictionary<ProjectPropertyInstance>(
    projectToInheritFrom._properties);
  _items = new ItemDictionary<ProjectItemInstance>();
  _actualTargets = new RetrievableEntryHashSet<ProjectTargetInstance>(
    StringComparer.OrdinalIgnoreCase);
  _targets = new ObjectModel.ReadOnlyDictionary<....>(_actualTargets);
  _environmentVariableProperties =
    projectToInheritFrom._environmentVariableProperties;
  _itemDefinitions = ....;
  _hostServices = projectToInheritFrom._hostServices;
  this.ProjectRootElementCache =
    projectToInheritFrom.ProjectRootElementCache;
  _explicitToolsVersionSpecified = 
    projectToInheritFrom._explicitToolsVersionSpecified;
  this.InitialTargets = new List<string>();
  this.DefaultTargets = new List<string>();
  this.DefaultTargets.Add("Build");
  this.TaskRegistry = projectToInheritFrom.TaskRegistry;
  _isImmutable = projectToInheritFrom._isImmutable;
  _importPaths = projectToInheritFrom._importPaths;
  ImportPaths = new ObjectModel.ReadOnlyCollection<string>(_importPaths);
  _importPathsIncludingDuplicates = 
    projectToInheritFrom._importPathsIncludingDuplicates;
  ImportPathsIncludingDuplicates = new
    ObjectModel.ReadOnlyCollection<string>(_importPathsIncludingDuplicates);
  this.EvaluatedItemElements = new List<ProjectItemElement>();
  IEvaluatorData<....> thisAsIEvaluatorData = this;
  thisAsIEvaluatorData.AfterTargets = new 
    Dictionary <string,List<TargetSpecification>>();
  thisAsIEvaluatorData.BeforeTargets = new 
    Dictionary<string, List<TargetSpecification>>();
  foreach (KeyValuePair<string, string> property in globalProperties)
  {
    _globalProperties[property.Key] = 
    ProjectPropertyInstance.Create(property.Key, property.Value,
                                   false /* may not be reserved */,
                                   _isImmutable);
  }
}
```

Уберём лишнее для наглядности:

```cpp
internal ProjectInstance(....)
{
  ....
  _explicitToolsVersionSpecified = 
    projectToInheritFrom._explicitToolsVersionSpecified;            // <=
  _properties = ....
  _items = ....
  _actualTargets = ....
  _targets = ....
  _environmentVariableProperties = ....
  _itemDefinitions = ....
  _hostServices = ....
  _explicitToolsVersionSpecified = 
    projectToInheritFrom._explicitToolsVersionSpecified;             // <=
  ....
}
```

Предупреждение анализатора PVS\-Studio: [V3008](https://pvs-studio.ru/ru/docs/warnings/v3008/)\. The '\_explicitToolsVersionSpecified' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 547, 538\. ProjectInstance\.cs 547

В этом фрагменте кода осуществляется присваивание для множества полей и свойств\. Однако среди них присутствует дубликат\. Иногда вместо одной переменной по ошибке используют другую, из\-за чего могут возникнуть проблемы\.

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

## Issue 8: Использование массива перед проверкой на null

```cpp
private async Task<BuildEngineResult> BuildProjectFilesInParallelAsync(....,
                                                 string[] toolsVersion, ....)
{
  if(.... && toolsVersion[0] == null)                               // <=
  {
    ....
  }
  else
  {
    ....    
  }

  IRequestBuilderCallback builderCallback = _requestEntry.Builder as 
                                             IRequestBuilderCallback;
  BuildResult[] results = await builderCallback.BuildProjects(
          projectFileNames,
          propertyDictionaries,
          toolsVersion ?? [],                                      // <=
          targetNames ?? [],
          waitForResults: true,
          skipNonexistentTargets: skipNonexistentTargets);
  ....
}
```

Сообщение PVS\-Studio: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/)\. The 'toolsVersion' object was used before it was verified against null\. Check lines: 1153, 1208\. TaskHost\.cs 1153

В коде происходит обращение к элементу массива `toolsVersion`, а затем к этому же массиву применяется `??`\. Это указывает на потенциальную возможность возникновения ошибки NullReferenceException, так как использование `toolsVersion` происходит до проверки на`null`\.

## Заключение

Повторные проверки проектов выявили множество новых ошибок, что подчёркивает необходимость [внедрения статического анализа](https://pvs-studio.ru/ru/blog/posts/0687/) в активно развивающиеся проекты\. MSBuild — это продукт компании Microsoft, и, вероятно, они уделяют много внимания поиску и устранению ошибок\. Тем не менее, мы по\-прежнему находим проблемные места в их коде\.

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

Мы постоянно совершенствуем наш статический анализатор PVS\-Studio\. Если вы хотите самостоятельно оценить возможности статического анализа, можете [бесплатно попробовать PVS\-Studio](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.

Спасибо за внимание :\)