Сергей Васильев

14 Дек 2022

Теги:

#CSharp

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

14 Дек 2022

Автор: Сергей Васильев

.NET 7 зарелизился. Это хороший повод покопаться в исходниках, чтобы поискать ошибки и странные места. За комментариями по находкам обратимся к самим разработчикам .NET — кому знать код, как не им? Погнали!

Я анализировал релизный код .NET 7. Взять его можно на GitHub: ссылка.

Перед релизом было 2 выпуска RC (release candidate), а поэтому основные баги должны быть устранены. Но тем даже интереснее, просочилось ли что-нибудь "в прод".

На каждый подозрительный фрагмент кода я открыл issue на GitHub. Это помогло понять, какой код ошибочен, какой избыточен, и какие правки вносили разработчики.

Issue 1

Проверка внимательности — что не так с кодом ниже?

internal sealed record IncrementalStubGenerationContext(
  StubEnvironment Environment,
  SignatureContext SignatureContext,
  ContainingSyntaxContext ContainingSyntaxContext,
  ContainingSyntax StubMethodSyntaxTemplate,
  MethodSignatureDiagnosticLocations DiagnosticLocation,
  ImmutableArray<AttributeSyntax> ForwardedAttributes,
  LibraryImportData LibraryImportData,
  MarshallingGeneratorFactoryKey<
    (TargetFramework, Version, LibraryImportGeneratorOptions)
  > GeneratorFactoryKey,
  ImmutableArray<Diagnostic> Diagnostics)
{
  public bool Equals(IncrementalStubGenerationContext? other)
  {
    return    other is not null
           && StubEnvironment.AreCompilationSettingsEqual(Environment, 
                                                          other.Environment)
           && SignatureContext.Equals(other.SignatureContext)
           && ContainingSyntaxContext.Equals(other.ContainingSyntaxContext)
           && StubMethodSyntaxTemplate.Equals(other.StubMethodSyntaxTemplate)
           && LibraryImportData.Equals(other.LibraryImportData)
           && DiagnosticLocation.Equals(DiagnosticLocation)
           && ForwardedAttributes.SequenceEqual(other.ForwardedAttributes, 
                (IEqualityComparer<AttributeSyntax>)
                  SyntaxEquivalentComparer.Instance)
          && GeneratorFactoryKey.Equals(other.GeneratorFactoryKey)
          && Diagnostics.SequenceEqual(other.Diagnostics);
    }

    public override int GetHashCode()
    {
      throw new UnreachableException();
    }
}

Ответ

Код проверяет эквивалентность двух объектов: this и other. Однако в одном из выражений допустили ошибку, сравнив свойство DiagnosticLocation с собой же.

Так неправильно:

DiagnosticLocation.Equals(DiagnosticLocation)

Так правильно:

DiagnosticLocation.Equals(other.DiagnosticLocation)

Эту проблему я нашёл в классе LibraryImportGenerator (ссылка на GitHub). Чуть позже нашёл ещё 2 записи с такими же ошибками, но уже в других классах:

класс JSImportGenerator, ссылка на GitHub;
класс JSExportGenerator, ссылка на GitHub.

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

В .NET 8 рассмотренный код сильно переписан, а для .NET 7 фикс пока не применялся. Подробности можно почитать в issue на GitHub.

Issue 2

internal static void CheckNullable(JSMarshalerType underlyingSig)
{
    MarshalerType underlying = underlyingSig._signatureType.Type;
    if (underlying == MarshalerType.Boolean
        || underlying == MarshalerType.Byte
        || underlying == MarshalerType.Int16
        || underlying == MarshalerType.Int32
        || underlying == MarshalerType.BigInt64
        || underlying == MarshalerType.Int52
        || underlying == MarshalerType.IntPtr
        || underlying == MarshalerType.Double
        || underlying == MarshalerType.Single // <=
        || underlying == MarshalerType.Single // <=
        || underlying == MarshalerType.Char
        || underlying == MarshalerType.DateTime
        || underlying == MarshalerType.DateTimeOffset
        ) return;
    throw new ArgumentException("Bad nullable value type");
}

Location: JSMarshalerType.cs, 387 (link)

Здесь два раза проверили переменную underlying на равенство MarshalerType.Single. Иногда за подобными одинаковыми проверками скрываются ошибки: условно, должны были проверить переменные left и right, но два раза проверили left. Примеры подобных ошибок из Open Source проектов собраны здесь.

Открыл issue на GitHub: link. В рассматриваемом случае из .NET 7 повезло — проверка просто оказалась избыточной.

Issue 3

public static bool TryParse(string text, out MetricSpec spec)
{
  int slashIdx = text.IndexOf(MeterInstrumentSeparator);
  if (slashIdx == -1)
  {
    spec = new MetricSpec(text.Trim(), null);
    return true;
  }
  else
  {
    string meterName = text.Substring(0, slashIdx).Trim();
    string? instrumentName = text.Substring(slashIdx + 1).Trim();
    spec = new MetricSpec(meterName, instrumentName);
    return true;
  }
}

Location: MetricsEventSource.cs, 453 (link)

Метод TryParse всегда возвращает одно и то же значение – true. Это странно. Посмотрим, где он используется:

private void ParseSpecs(string? metricsSpecs)
{
  ....
  string[] specStrings = ....
  foreach (string specString in specStrings)
  {
    if (!MetricSpec.TryParse(specString, out MetricSpec spec))
    {
      Log.Message($"Failed to parse metric spec: {specString}");
    }
    else
    {
      Log.Message($"Parsed metric: {spec}");
      ....
    }
  }
}

Location: MetricsEventSource.cs, 375 (link)

Возвращаемое значение метода TryParse используется как условие оператора if. Если не удалось распарсить строку specString, исходное значение должно логироваться. Иначе логируется полученное представление — spec —, и над ним выполняются ещё какие-то операции.

Проблема в том, что TryParse всегда возвращает true. Значит, then-ветвь оператора if никогда не выполняется, то есть парсинг всегда считается успешным.

Issue на GitHub: link.

В результате исправления TryParse превратился в Parse, а в вызывающем методе пропал оператор if. В TryParse заодно поменяли Substring на AsSpan.

Кстати, этот же фрагмент кода я отмечал в прошлый раз, когда копался в исходниках .NET 6. Тогда в методах логирования был пропущен символ интерполяции:

if (!MetricSpec.TryParse(specString, out MetricSpec spec))
{
  Log.Message("Failed to parse metric spec: {specString}");
}
else
{
  Log.Message("Parsed metric: {spec}");
  ....
}

Подробнее про эту и подобную ошибки можно почитать в статье про проверку .NET 6 (issue 14).

Issue 4

Раз уж мы затронули тему методов со странными возвращаемыми значениями, рассмотрим ещё один:

public virtual bool TryAdd(XmlDictionaryString value, out int key)
{
  ArgumentNullException.ThrowIfNull(value);

  IntArray? keys;

  if (_maps.TryGetValue(value.Dictionary, out keys))
  {
    key = (keys[value.Key] - 1);

    if (key != -1)
    {
      // If the key is already set, then something is wrong
      throw System.Runtime
                  .Serialization
                  .DiagnosticUtility
                  .ExceptionUtility
                  .ThrowHelperError(
      new InvalidOperationException(SR.XmlKeyAlreadyExists));
     }

     key = Add(value.Value);
     keys[value.Key] = (key + 1);
     return true;               // <=
  }

  key = Add(value.Value);
  keys = AddKeys(value.Dictionary, value.Key + 1);
  keys[value.Key] = (key + 1);
  return true;                  // <=
}

Location: XmlBinaryWriterSession.cs, 28 (link)

Метод или возвращает true, или выбрасывает исключение, но никогда не возвращает false. Это уже публичный API, так что и спроса больше.

Посмотрим описание на learn.microsoft.com:

Нестыковочка. Я также открыл issue на GitHub (ссылка), но на момент написания статьи новостей не было.

Issue 5

public static Attribute? GetCustomAttribute(ParameterInfo element, 
                                            Type attributeType, 
                                            bool inherit)
{
  // ....
  Attribute[] attrib = GetCustomAttributes(element, attributeType, inherit);

  if (attrib == null || attrib.Length == 0)
    return null;

  if (attrib.Length == 0)
    return null;

  if (attrib.Length == 1)
    return attrib[0];

  throw new AmbiguousMatchException(SR.RFLCT_AmbigCust);
}

Location: Attribute.CoreCLR.cs, 617 (link)

В этом фрагменте кода 2 раза проверяют одно и то же выражение — attrib.Length == 0: сначала как правый операнд оператора '||', затем как условное выражение оператора if.

Иногда это признак ошибки — хотели проверить одно, а проверили другое. Здесь повезло: вторая проверка оказалась избыточной, и разработчики её убрали.

Issue на GitHub: link.

Issue 6

protected virtual XmlSchema? GetSchema()
{
  if (GetType() == typeof(DataTable))
  {
    return null;
  }
  MemoryStream stream = new MemoryStream();

  XmlWriter writer = new XmlTextWriter(stream, null);
  if (writer != null)
  {
    (new XmlTreeGen(SchemaFormat.WebService)).Save(this, writer);
  }
  stream.Position = 0;
  return XmlSchema.Read(new XmlTextReader(stream), null);
}

Location: DataTable.cs, 6678 (link)

Создали экземпляр типа XmlTextWriter, ссылку на него записали в переменную writer и сразу на следующей строке проверяют её на неравенство null. Проверка всегда будет давать true, значит условие здесь лишнее.

Нестрашно, но проверку можно убрать. Так и сделали (issue на GitHub):

Issue 7

Тоже избыточный код, но чуть менее очевидный.

public int ToFourDigitYear(int year, int twoDigitYearMax)
{
  if (year < 0)
  {
    throw new ArgumentOutOfRangeException(nameof(year), 
                                          SR.ArgumentOutOfRange_NeedPosNum);
  }

  if (year < 100)
  {
    int y = year % 100;
    return (twoDigitYearMax / 100 - (y > twoDigitYearMax % 100 ? 1 : 0)) 
             * 100 + y;
  }
  ....
}

Location: GregorianCalendarHelper.cs, 526 (link)

Давайте проследим за тем, как уточняется значение переменной year по мере исполнения кода.

ToFourDigitYear(int year, int twoDigitYearMax)

year – параметр метода типа int. Значит, его значение находится в диапазоне [int.MinValue; int.MaxValue].

При исполнении кода первым делом встречается оператор if, в котором выбрасывается исключение:

if (year < 0)
{
  throw ....;
}

Если исключение не было выброшено, значит значение year лежит в диапазоне [0; int.MaxValue].

Дальше ещё один оператор if:

if (year < 100)
{
  int y = year % 100;
  ....
}

Если исполнение кода зашло в then-ветвь оператора if, значит значение year находится в диапазоне [0; 99]. Это уже приводит к интересному результату операции взятия остатка от деления:

int y = year % 100;

Значение year всегда меньше 100 (от 0 до 99). Как следствие, результат операции year % 100 всегда будет равен левому операнду – year. Получается, что y и year всегда равны.

Итого, или код избыточный, или здесь какая-то ошибка. Открыл issue на GitHub: код поправили, убрав переменную y.

Issue 8

internal ConfigurationSection
FindImmediateParentSection(ConfigurationSection section)
{
  ....
  SectionRecord sectionRecord = ....
  if (sectionRecord.HasLocationInputs)
  {
    SectionInput input = sectionRecord.LastLocationInput;
    Debug.Assert(input.HasResult, "input.HasResult");
    result = (ConfigurationSection)input.Result;
  }
  else
  {
    if (sectionRecord.HasIndirectLocationInputs)
    {
      Debug.Assert(IsLocationConfig, 
                   "Indirect location inputs exist 
                    only in location config record");
      SectionInput input = sectionRecord.LastIndirectLocationInput;
      Debug.Assert(input != null);
      Debug.Assert(input.HasResult, "input.HasResult");
      result = (ConfigurationSection)input.Result;
    }
    ....
  ....
}

Location: MgmtConfigurationRecord.cs, 341 (link)

Здесь придётся немного повозиться. Для начала посмотрим на второй оператор if:

if (sectionRecord.HasIndirectLocationInputs)
{
  Debug.Assert(IsLocationConfig, 
               "Indirect location inputs exist 
                only in location config record");
  SectionInput input = sectionRecord.LastIndirectLocationInput;
  Debug.Assert(input != null);
  Debug.Assert(input.HasResult, "input.HasResult");
  result = (ConfigurationSection)input.Result;
}

В переменную input записывается значение свойства LastIndirectLocationInput. После этого input проверяется в двух ассертах: на null (input != null) и наличие результата (input.HasResult).

Посмотрим на тело свойства LastIndirectLocationInput, чтобы понять, какое значение может быть записано в переменную input:

internal SectionInput LastIndirectLocationInput
  =>   HasIndirectLocationInputs 
     ? IndirectLocationInputs[IndirectLocationInputs.Count - 1] 
     : null;

Смотрите, с одной стороны, свойство может вернуть значение null. С другой — видно, что если HasIndirectLocationInputs — true, то возвращается IndirectLocationInputs[IndirectLocationInputs.Count - 1], а не явное значение null.

Вопрос вот в чём — может ли значение из коллекции IndirectLocationInputs быть равно null? Возможно — из этого кода непонятно. Кстати, тут могли бы помочь nullable-аннотации, но они включены не во всех проектах .NET.

Возвращаемся в if:

if (sectionRecord.HasIndirectLocationInputs)
{
  Debug.Assert(IsLocationConfig, 
               "Indirect location inputs exist 
                only in location config record");
  SectionInput input = sectionRecord.LastIndirectLocationInput;
  Debug.Assert(input != null);
  Debug.Assert(input.HasResult, "input.HasResult");
  result = (ConfigurationSection)input.Result;
}

Условное выражение — sectionRecord.HasIndirectLocationInputs. Это то же самое свойство, которое проверяется в LastIndirectLocationInput. Значит, LastIndirectLocationInput точно не возвращает явный null. Однако какое значение будет получено из IndirectLocationInputs и записано в input — непонятно.

Разработчики сначала проверяют, что input != null, и лишь затем смотрят наличие результата — input.HasResult. Выглядит нормально.

Теперь вернёмся в первый оператор if:

if (sectionRecord.HasLocationInputs)
{
  SectionInput input = sectionRecord.LastLocationInput;
  Debug.Assert(input.HasResult, "input.HasResult");
  result = (ConfigurationSection)input.Result;
}

Посмотрим на свойство LastLocationInput:

internal SectionInput LastLocationInput 
  =>  HasLocationInputs 
    ? LocationInputs[LocationInputs.Count - 1] 
    : null;

Оно написано по тому же принципу, что и LastIndirectLocationInput. Как и в предыдущем случае, в зависимости от флага (HasLocationInputs) возвращается или null, или значение из коллекции LocationInputs.

Возвращаемся к оператору if. Его условное выражение — свойство HasLocationInputs, которое проверяется и внутри LastLocationInput. Если зашли в тело if, значит LastLocationInput не может вернуть явный null. Может ли значение из коллекции LocationInputs быть null? Вопрос открытый. Если может, то и в input тоже будет записан null.

Как и в случае с первым рассмотренным if, выполняется проверка input.HasResult, а вот проверка input != null на этот раз отсутствует.

Ещё раз. Первый разобранный фрагмент кода:

SectionInput input = sectionRecord.LastIndirectLocationInput;
Debug.Assert(input != null);
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;

Второй:

SectionInput input = sectionRecord.LastLocationInput;
Debug.Assert(input.HasResult, "input.HasResult");
result = (ConfigurationSection)input.Result;

Выглядит так, будто пропустили выражение Debug.Assert(input != null).

Я открыл issue на GitHub, где описал это и другие подозрительные места, связанные с null-проверками (их мы рассмотрим ниже).

Рассмотренный код исправлять не стали, оставив as is:

Issues c проверкой на null

В коде я встретил несколько мест, в которых ссылка сначала разыменовывается, а затем проверяется на null. Чтобы не плодить задачи, собрал их все в общем issue на GitHub.

Давайте разберём эти места.

Issue 9

private static RuntimeBinderException BadOperatorTypesError(Expr pOperand1, 
                                                            Expr pOperand2)
{
  // ....
  string strOp = pOperand1.ErrorString;

  Debug.Assert(pOperand1 != null);
  Debug.Assert(pOperand1.Type != null);

  if (pOperand2 != null)
  {
    Debug.Assert(pOperand2.Type != null);
    return ErrorHandling.Error(ErrorCode.ERR_BadBinaryOps,
                               strOp, 
                               pOperand1.Type, 
                               pOperand2.Type);
  }

  return ErrorHandling.Error(ErrorCode.ERR_BadUnaryOp, strOp, pOperand1.Type);
}

Location: ExpressionBinder.cs, 798 (link)

Сначала pOperand1 разыменовывается (pOperand1.ErrorString), а уже на следующей строке проверяется на null в Debug.Assert. Если pOperand1 будет null, вместо срабатывания ассерта будет выброшено исключение типа NullReferenceException.

Код поправили, вынеся проверку pOperand1 до использования.

Было:

string strOp = pOperand1.ErrorString;

Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);

Стало:

Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);

string strOp = pOperand1.ErrorString;

Issue 10

public void Execute()
{
  var count = _callbacks.Count;
  if (count == 0)
  {
    return;
  }

  List<Exception>? exceptions = null;

  if (_callbacks != null)
  {
    for (int i = 0; i < count; i++)
    {
      var callback = _callbacks[i];
      Execute(callback, ref exceptions);
    }
  }

  if (exceptions != null)
  {
    throw new AggregateException(exceptions);
  }
}

Location: PipeCompletionCallbacks.cs, 20 (link)

Переменную _callbacks сначала используют, а затем проверяют её значение на неравенство null:

public void Execute()
{
  var count = _callbacks.Count;
  ....
  if (_callbacks != null)
  ....
}

На момент написания статьи код исправили, убрав проверку _callbacks на null.

Кстати, _callbacks – это readonly поле, которое инициализируется в конструкторе:

internal sealed class PipeCompletionCallbacks
{
  private readonly List<PipeCompletionCallback> _callbacks;
  private readonly Exception? _exception;
  public PipeCompletionCallbacks(List<PipeCompletionCallback> callbacks, 
                                 ExceptionDispatchInfo? edi)
  {
    _callbacks = callbacks;
    _exception = edi?.SourceException;
  }
  ....
}

В треде с исправлениями обсуждали, стоит ли добавить в конструктор Debug.Assert с проверкой _callbacks на null. В итоге решили не делать.

Issue 11

private void ValidateAttributes(XmlElement elementNode)
{
  ....
  XmlSchemaAttribute schemaAttribute 
    = (_defaultAttributes[i] as XmlSchemaAttribute)!;
  attrQName = schemaAttribute.QualifiedName;
  Debug.Assert(schemaAttribute != null);
  ....
}

Location: DocumentSchemaValidator.cs, 421 (link)

Противоречивый код:

Результат работы оператора as записывается в schemaAttribute. Если _defaultAttributes[i] — null, или приведение выполнить не удалось, результатом будет null.
null-forgiving оператор ('!') намекает, что результат приведения не может быть null. Как следствие, schemaAttribute не должен быть равен null.
На следующей строке schemaAttribute разыменовывается. Ещё строкой ниже проверяется, что она не равна null.

Внимание, вопрос. Может ли schemaAttribute иметь значение null? Из этого кода, конечно, не очень понятно.

Исправили код так:

....
XmlSchemaAttribute schemaAttribute 
  = (XmlSchemaAttribute)_defaultAttributes[i]!;
attrQName = schemaAttribute.QualifiedName;
....

В обсуждении правок предлагали не удалять вызов Debug.Assert, а перенести его строкой выше. Код выглядел бы так:

....
XmlSchemaAttribute schemaAttribute = (XmlSchemaAttribute)_defaultAttributes[i]!;
Debug.Assert(schemaAttribute != null);
attrQName = schemaAttribute.QualifiedName;
....

В итоге Assert решили не возвращать:

Issue 12

Взглянем на конструктор типа XmlConfigurationElementTextContent:

public XmlConfigurationElementTextContent(string textContent, 
                                          int? linePosition, 
                                          int? lineNumber)
{ .... }

Location: XmlConfigurationElementTextContent.cs, 10 (link)

Теперь посмотрим, где он используется:

public static IDictionary<string, string?> Read(....)
{
  ....
  case XmlNodeType.EndElement:
    ....
    var lineInfo = reader as IXmlLineInfo;
    var lineNumber = lineInfo?.LineNumber;
    var linePosition = lineInfo?.LinePosition;
    parent.TextContent = new XmlConfigurationElementTextContent(string.Empty, 
                                                                lineNumber,
                                                                linePosition);
    ....
    break;
  ....
  case XmlNodeType.Text:
    ....
    var lineInfo = reader as IXmlLineInfo;
    var lineNumber = lineInfo?.LineNumber;
    var linePosition = lineInfo?.LinePosition;

    XmlConfigurationElement parent = currentPath.Peek();

    parent.TextContent = new XmlConfigurationElementTextContent(reader.Value,
                                                                lineNumber, 
                                                                linePosition);
    ....
    break;
  ....
}

Locations:

XmlStreamConfigurationProvider.cs, 133 (link)
XmlStreamConfigurationProvider.cs, 148 (link)

Заметили подвох в коде?

Обратите внимание на порядок аргументов и параметров:

аргументы: ..., lineNumber, linePosition;
параметры: ..., linePosition, lineNumber.

Я открыл issue на GitHub (link), код поправили: аргументы поменяли местами и добавили тест.

Issue 13

Ещё один подозрительный случай:

public virtual bool Nested
{
  get {....}
  set 
  {
    ....
    ForeignKeyConstraint? constraint 
      = ChildTable.Constraints
                  .FindForeignKeyConstraint(ChildKey.ColumnsReference, 
                                            ParentKey.ColumnsReference); 
    ....
  }
}

Location: DataRelation.cs, 486 (link)

Посмотрим на метод FindForeignKeyConstraint:

internal ForeignKeyConstraint? 
FindForeignKeyConstraint(DataColumn[] parentColumns, 
                         DataColumn[] childColumns)
{ .... }

Location: ConstraintCollection.cs, 548 (link)

Похоже, порядок аргументов опять неверный:

параметры: parent..., child...
аргументы: ChildKey..., ParentKey...

Есть ещё один вызов этого метода: там к порядку аргументов нет вопросов.

ForeignKeyConstraint? foreignKey
  = relation.ChildTable
            .Constraints
            .FindForeignKeyConstraint(relation.ParentColumnsReference,
                                      relation.ChildColumnsReference);

Открыл issue на GitHub: link. Увы, на момент написания статьи я не получил комментариев по этому коду.

Issue 14

Это не все места, где порядок аргументов перепутан — нашёл ещё одно:

void RecurseChildren(....)
{
  ....
  string? value 
    =  processValue != null
      ? processValue(new ConfigurationDebugViewContext(
                           child.Key, 
                           child.Path, 
                           valueAndProvider.Value, 
                           valueAndProvider.Provider))
      : valueAndProvider.Value;

  ....
}

Location: ConfigurationRootExtensions.cs, 50 (link)

Посмотрим на конструктор ConfigurationDebugViewContext:

public ConfigurationDebugViewContext(
  string path, 
  string key, 
  string? value, 
  IConfigurationProvider configurationProvider) 
{ .... }

Location: ConfigurationDebugViewContext.cs, 11 (link)

Последовательность:

параметры: path, key, ...
аргументы: child.Key, child.Path, ...

Открыл issue на GitHub: link. Со слов разработчиков, несмотря на ошибку, этот кейс не вызывал проблем.

Тем не менее, порядок аргументов изменили на правильный.

Заключение

Код .NET 7 высокого качества. Я думаю, этому способствует в том числе налаженный процесс разработки: даты релизов известны, выпуски RC тоже проводятся не зря.

Тем не менее, каждый раз в исходниках получается найти что-то интересное. В этот раз фаворитами для меня стали аргументы, перепутанные при вызовах методов.

Все фрагменты кода, описанные в статье, я нашёл анализатором PVS-Studio. Да, теперь им можно проверять проекты на .NET 7.

Если захотите проверить свой проект (личный или коммерческий), анализатор можете загрузить с этой страницы. Там же есть ссылка на документацию: в ней мы описали, как ввести лицензию и запустить анализ. Если возникнут проблемы или вопросы — обязательно пишите нам, поможем.

#CSharp