Ошибки и подозрительные места в исходниках .NET 8

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

Кстати говоря, у нас уже есть несколько статей, посвящённых последним обновлениям в мире .NET и C#. Если вас интересует, что в этот раз добавили Microsoft, рекомендую вам заглянуть в следующие материалы:

• Что нового в .NET 8?

• Обзор нововведений в C# 12

В этих статьях мы рассматриваем ключевые изменения, которые появились в последних версиях .NET и C#. Приглашаю вас прочитать, чтобы быть в курсе последних событий.

Кроме того, в последнем релизе PVS-Studio 7.28 уже реализована поддержка анализа проектов, использующих .NET 8. Для проведения анализа исходников использовался релизный код .NET 8, который доступен на GitHub по ссылке.

Перед тем, как мы приступим к изучению обнаруженных ошибок в .NET 8, хочу рассказать небольшую историю.

Как известно, .NET огромен, и это может создавать проблемы. В исходниках есть скрипт, который позволяет сгенерировать решение для .NET библиотек. Это решение я проанализировал с помощью консольной утилиты PVS-Studio. Отчёт же я принялся изучать в IDE, в которой я работаю — Visual Studio 2022, но возникла проблема. При попытках навигации по коду в Visual Studio 2022 происходило нечто непредвиденное: либо происходила перезагрузка IDE, либо она просто завершала свою работу. Причём такое поведение повторяется не только при навигации по коду с помощью плагина PVS-Studio, но и при обычном переключении между файлами, использовании 'Go To Definition' и т. д.

Это усложнило работу, но выход нашёлся быстро.

Не так давно у нас появилась поддержка анализа .NET проектов в VS Code. Про это есть отдельная статья: "Использование расширения VS Code "PVS-Studio" для эффективной борьбы с ошибками в C# коде". Учитывая, что VS Code представляет собой легковесный редактор кода, подобных трудностей, с которыми мы столкнулись в Visual Studio 2022, там не возникло.

Вот так выглядит окно PVS-Studio в Visual Studio Code:

.NET — мощная платформа, которая имеет высокие стандарты для кода, пишется настоящими профессионалами и хорошо тестируется. Однако даже в таком крутом проекте PVS-Studio способен найти ошибки.

А теперь давайте перейдём к рассмотрению обнаруженных ошибок.

Фрагмент кода 1

private static bool IsRoamingSetting(SettingsProperty setting)
{
  List<KeyValuePair<int, ServiceCallSite>> callSitesByIndex = new();
  ....
  SettingsManageabilityAttribute manageAttr = ....;
  return    manageAttr != null 
         && ((manageAttr.Manageability & SettingsManageability.Roaming) ==
             SettingsManageability.Roaming);
}

Предупреждение PVS-Studio: V3181 The result of '&' operator is '0' because the value of 'SettingsManageability.Roaming' is '0'. LocalFileSettingsProvider.cs 411

В данном случае значение константы перечисления SettingsManageability.Roaming равно 0. Поскольку результат побитового "И" с операндом 0 всегда равен 0, получается, что 0 сравнивается с 0. Выходит, что результатом выражения ((manageAttr.Manageability & SettingsManageability.Roaming) == SettingsManageability.Roaming всегда является true.

Разработчикам стоит обратить внимание на этот код.

Фрагмент кода 2

internal DataView(....)
{
  ....
  DataCommonEventSource.Log.Trace("<ds.DataView.DataView|API> %d#, table=%d, 
                                   RowState=%d{ds.DataViewRowState}\n",
                ObjectID, (table != null) ? table.ObjectID : 0, (int)RowState);
  ....
}

Предупреждение PVS-Studio: V3025 The 1st argument '"<ds.DataView.DataView|API> %d#, table=%d, RowState=%d{ds.DataViewRowState}\n"' is used as incorrect format string inside method. A different number of format items is expected while calling 'Trace' function. Arguments not used: 1st, 2nd, 3rd. DataView.cs 166, DataCommonEventSource.cs 45

Анализатор сообщает о некорректной строке формата в первом аргументе метода Trace. Посмотрим на этот метод:

internal void Trace<T0, T1, T2>(string format, T0 arg0, T1 arg1, T2 arg2)
{
  if (!Log.IsEnabled()) return;
  Trace(string.Format(format, arg0, arg1, arg2));
}

Действительно, первый аргумент используется в качестве строки формата. В эту строку подставляются аргументы. Вот только аргументы должны подставляться в плейсхолдеры вида {0}, {1} и т. д. В данной строке подобные плейсхолдеры отсутствуют. В итоге использования такой строки формата будет выброшено исключение типа System.FormatException о некорректном формате.

Возможно, нужно использовать какой-то другой метод логирования. Если пройтись по другим местам использования метода Trace, то там всё используется корректно, и строки формата содержат маркеры:

Фрагмент кода 3

public static SqlDecimal operator /(SqlDecimal x, SqlDecimal y)
{
  ....
  bScaleD = x._bScale;
  bPrecD = x._bPrec;
  ResScale = Math.Max(x._bScale + y._bPrec + 1, s_cNumeDivScaleMin);
  ResInteger = x._bPrec - x._bScale + y._bScale;
  ResPrec = ResScale + x._bPrec + y._bPrec + 1;               // <=
  MinScale = Math.Min(ResScale, s_cNumeDivScaleMin);

  ResInteger = Math.Min(ResInteger, s_NUMERIC_MAX_PRECISION);
  ResPrec = ResInteger + ResScale;                            // <=
  ....
}

Предупреждение PVS-Studio: V3008 The 'ResPrec' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1689, 1685. SQLDecimal.cs 1689

В данном фрагменте видно, что происходит двойное присваивание в переменную ResPrec.

Поскольку между этими двумя операциями ResPrec не используется, это свидетельствует об ошибке.

Здесь два варианта:

• Одно из присваиваний является лишним — ничего страшного, просто лишняя операция, хотя это и нехорошо;

• Между двумя присваиваниями переменная ResPrec должна использоваться — вот это уже будет неприятной ошибкой.

Фрагмент кода 4

public override void MoveToAttribute(int i)
{
  ....
  _currentAttrIndex = i;
  if (i < _coreReaderAttributeCount)
  {
    ....
    _validationState = ValidatingReaderState.OnAttribute;
  }
  else
  {
    ....
    _validationState = ValidatingReaderState.OnDefaultAttribute;
  }

  if (_validationState == ValidatingReaderState.OnReadBinaryContent)
  {
    Debug.Assert(_readBinaryHelper != null);
    _readBinaryHelper.Finish();
    _validationState = _savedState;
  }
}

Предупреждение PVS-Studio: V3022 Expression '_validationState == ValidatingReaderState.OnReadBinaryContent' is always false. XsdValidatingReader.cs 1302

PVS-Studio обнаружил, что последнее условие if (_validationState == ValidatingReaderState.OnReadBinaryContent) всегда будет ложным. Давайте разбираться почему.

Взглянем на первый оператор if. В нём полю _validationState присваивается:

• в then ветви — ValidatingReaderState.OnAttribute

• в else ветви — ValidatingReaderState.OnDefaultAttribute

Поэтому значение поля не может быть равно ValidatingReaderState.OnReadBinaryContent, и код внутри if не выполняется.

Фрагмент кода 5

private static string GetTypeNameDebug(TypeDesc type)
{
  string result;
  TypeDesc typeDefinition = type.GetTypeDefinition();
  if (type != typeDefinition)
  {
    result = GetTypeNameDebug(typeDefinition) + "<";
    for (int i = 0; i < type.Instantiation.Length; i++)
      result += (i == 0 ? "" : ",") + GetTypeNameDebug(type.Instantiation[0]);
    return result + ">";
  }
  else
  {
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3102 Suspicious access to element of 'type.Instantiation' object by a constant index inside a loop. TypeLoaderEnvironment.GVMResolution.cs 32

Предположу, что в данном фрагменте кода из информации о типе формируется запись следующего вида: ConsoleApp1.Program.MyClass<string, int, double>. Вот только в цикле обращаются к объекту type.Instantiation по константному индексу, равному 0. Не исключено, что работает всё как надо, но выглядит очень странно. Ожидаешь увидеть GetTypeNameDebug(type.Instantiation[i]).

И да, я сразу пошёл и проверил, в дебаггере Visual Studio 2022 всё отображается корректно, но не исключено, что где-то можно встретить отображение типа с ошибкой :).

Фрагмент кода 6

Instruction[]? GetArgumentsOnStack (MethodDefinition method)
{
  int length = method.GetMetadataParametersCount ();
  Debug.Assert (length != 0);
  if (stack_instr?.Count < length)
    return null;

  var result = new Instruction[length];
  while (length != 0)
    result[--length] = stack_instr!.Pop ();    // <=

  return result;
}

Предупреждение PVS-Studio: V3125 The 'stack_instr!' object was used after it was verified against null. Check lines: 1918, 1913. UnreachableBlocksOptimizer.cs 1918

Разработчик использовал оператор '?.', подразумевая, что поле stack_instr может быть null. И вроде бы всё хорошо, есть проверка, но не тут-то было. В указанной строчке возможно разыменование нулевой ссылки. Скорее всего, разработчик подумал, что выражение stack_instr?.Count < length при stack_instr равным null вернёт true, и произойдёт выход из метода, но нет — результатом будет false.

Более того, разработчик подавил сообщение компилятора о возможном разыменовании null ссылки с помощью '!', т.к. подумал, что статический анализ компилятора просто не справился и не понял проверки.

А как вы относитесь к nullable контексту? Если интересно наше мнение, или если вы ещё не знакомы с данным механизмом, то предлагаю почитать наши статьи:

• Помоги компилятору, и он поможет тебе. Тонкости работы с nullable reference типами в C#

• Nullable Reference не защищают, и вот доказательства

• Nullable Reference типы в C# 8.0 и статический анализ

Фрагмент кода 7

private HierarchyFlags GetFlags (TypeDefinition resolvedType)
{
  if (_cache.TryGetValue (resolvedType, out var flags))
  {
    return flags;
  }

  if (   resolvedType.Name == "IReflect"                // <=
      && resolvedType.Namespace == "System.Reflection") 
  {
    flags |= HierarchyFlags.IsSystemReflectionIReflect;
  }
  ....
  if (resolvedType != null)                             // <=
    _cache.Add (resolvedType, flags);

  return flags;
}

Предупреждение PVS-Studio: V3095 The 'resolvedType' object was used before it was verified against null. Check lines: 34, 55. TypeHierarchyCache.cs 34

Параметр resolvedType сначала используют, но перед добавлением в кэш проверяют на null. Странно как-то выходит. Анализатор указал на resolvedType.Name, но программа упадёт даже раньше. Метод TryGetValue выбросит исключение, если первый аргумент resolvedType будет null.

Фрагмент кода 8

public static bool IsTypeOf<T> (this TypeReference tr)
{
  var type = typeof (T);
  return tr.Name == type.Name && tr.Namespace == tr.Namespace;
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'tr.Namespace' to the left and to the right of the '==' operator. TypeReferenceExtensions.cs 365

Анализатор выявил, что в данном коде сравниваются два одинаковых подвыражения. Простая, но обидная ошибка. tr.Namespace сравнивается с tr.Namespace, а должен с type.Namespace.

Фрагмент кода 9

public void WriteTo(TextWriter writer, int methodRva, bool dumpRva)
{
  ....
  switch (Flags & CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_KIND_MASK)
  {
    case CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_NONE:
      writer.Write($" CATCH: {0}", ClassName ?? "null");
      break;

    case CorExceptionFlag.COR_ILEXCEPTION_CLAUSE_FILTER:
      writer.Write($" FILTER (RVA {0:X4})",
                   ClassTokenOrFilterOffset + methodRva);
      break;
    ....
  }
  ....
}

Предупреждение PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Write' function. Arguments not used: ClassName ?? "null". EHInfo.cs 135

Ещё одна ошибка со строкой формата, но в этот раз для класса TextWriter. Разработчик использовал символ интерполяции строк '$'. В строку просто подставится число 0, и строка формата станет равна " CATCH: 0". В итоге текст, который хотели подставить вместо плейсхолдера {0}, не используется. Такая же ошибка и в следующем case.

Фрагмент кода 10

public TType ParseType()
{
  CorElementType corElemType = ReadElementType();
  switch (corElemType)
  {
    ....
    case CorElementType.ELEMENT_TYPE_GENERICINST:
    {
      TType genericType = ParseType();
      uint typeArgCount = ReadUInt();
      var outerDecoder = new R2RSignatureDecoder<....>(_provider,
                                                       Context,
                                                       _outerReader, // <=
                                                       _image,
                                                       _offset,
                                                       _outerReader, // <=
                                                       _contextReader);
  }
}

Предупреждение PVS-Studio: V3038 The argument was passed to constructor several times. It is possible that other argument should be passed instead. ReadyToRunSignature.cs 707

Аргумент _outerReader передаётся в конструктор два раза. Если взглянуть на объявление конструктора, то можно увидеть, что конструктор имеет параметр с именем metadataReader:

public R2RSignatureDecoder(IR2RSignatureTypeProvider<....> provider,
                           TGenericContext context,
                           MetadataReader metadataReader,  // <=
                           byte[] signature,
                           int offset,
                           MetadataReader outerReader,     // <=
                           ReadyToRunReader contextReader,
                           bool skipOverrideMetadataReader = false)
{
  ....
}

В момент вызова конструктора доступно поле _metadataReader. Возможно, в качестве третьего аргумента стоит использовать именно его.

Фрагмент кода 11 — бонус

protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(....)
{
  bool requiresAlign8 
    =    !largestAlignmentRequired.IsIndeterminate 
      && context.Target.PointerSize == 4
      && context.Target.GetObjectAlignment(....).AsInt > 4 
      && context.Target.PointerSize == 4;
}

Предупреждение PVS-Studio: V3001 There are identical sub-expressions 'context.Target.PointerSize == 4' to the left and to the right of the '&&' operator. MetadataFieldLayoutAlgorithm.cs 648

В выражении два раза проверяется context.Target.PointerSize == 4. В экземпляром методе GetObjectAlignment изменение context.Target.PointerSize не происходит. Возможно, что здесь должно проверяться что-то ещё, а может это просто лишняя проверка.

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

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Artem Rovenskii. Bugs and suspicious places in .NET 8 source code.