Никита Прядко

14 Фев 2025

Теги:

#CSharp

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

14 Фев 2025

Автор: Никита Прядко

Введение
Issue 1: Некорректная строка формата
Issue 2: Неправильная отписка от события
Issue 3: WriteLine вместо Write
Issue 4: Пустая проверка на null
Issue 5: Подозрительная проверка на null
Issue 6: Ошибка приоритетов операторов
Issue 7: Дублирование присваивания
Issue 8: Использование массива перед проверкой на null
Заключение

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

Введение

Это третья наша проверка проекта MSBuild, первая была в 2016 году, а вторая — в 2022 году.

Состояние кода проекта на момент анализа соответствует этому коммиту.

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

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

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

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. 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:

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: Неправильная отписка от события

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

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

Сообщение анализатора: 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

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. 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

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

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

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

  _logFile = parameters[0];
}

Сообщение анализатора: 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:

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

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. 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: Ошибка приоритетов операторов

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

Сообщение анализатора PVS-Studio: 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 никогда не будут суммироваться, поскольку без скобок приоритет операторов будет следующим:

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

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

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

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

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

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

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);
  }
}

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

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

Предупреждение анализатора PVS-Studio: V3008. The '_explicitToolsVersionSpecified' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 547, 538. ProjectInstance.cs 547

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

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

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

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. The 'toolsVersion' object was used before it was verified against null. Check lines: 1153, 1208. TaskHost.cs 1153

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

Заключение

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

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

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

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

#CSharp