.NET 7: Suspicious Places and Errors in the Source Code

.NET 7 has been released! Dig into its source code for errors and strange code fragments. Here, see comments on our findings from the .NET developers.

Sergey Vasiliev

Dec. 30, 22 · Analysis

Likes (1)

Comment

Save

3.6K Views

.NET 7 has been released! It's time for us to dig into its source code and start looking for errors and strange code fragments. In this article, you'll see comments on our findings from the .NET developers. After all, they know the platform code better than anyone else. Buckle up!

I analyzed the release code of .NET 7.

There were two release candidates (RC) prior to the main release, so most of the bugs must have been fixed. It's more interesting that way — we can investigate whether some of them have gotten into production.

I created an issue on GitHub for each suspicious code fragment. This helped me understand which ones are redundant, which ones are incorrect, and what was fixed by developers.

Issue 1

Can you spot an error here? Let's check!

     C# 
   
 
 
   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 code fragment checks whether the this and other objects are equivalent. However, the developer made a mistake and compared the DiagnosticLocation property with itself.

Incorrect comparison:

     C# 
   
   DiagnosticLocation.Equals(DiagnosticLocation)

Correct comparison:

     C# 
   
   DiagnosticLocation.Equals(other.DiagnosticLocation)

I found this error in the LibraryImportGenerator class. A bit later I found two more fragments — the same error, but in different classes:

Fun fact: .NET 7 has a test for this feature. However, the test is also incorrect, that's why it doesn't detect this error.

In .NET 8 the code is heavily rewritten. However, the developers haven't fixed the .NET 7 code yet — they decided to wait for the feedback. You can read more about it in the issue on GitHub.

Issue 2

     C# 
   
 
 
   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

Here the developer double-checks if the underlying variable equals to MarshalerType.Single. Sometimes such checks hide errors: for example, the left and right variables should have been checked, but instead, the left variable is checked twice. Here's a list of similar errors found in open-source projects.

I created an issue on GitHub. Luckily, this code fragment wasn't erroneous — it was just a redundant check.

Issue 3

     C# 
   
 
 
   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

The TryParse method always returns true. This is weird. Let's see where this method is used:

     C# 
   
 
 
   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

The return value of the TryParse method is used as the condition of the if statement. If specString cannot be parsed, the original value should be logged. Otherwise, the received representation (spec) is logged, and some operations are performed on it.

The problem is TryParse always returns true. Thus, the then branch of the if statement is never executed — the parsing is always successful (link here to the Issue on GitHub).

As a result of the fix, TryParse became Parse, and the caller method lost the if statement. The developers also changed Substring to AsSpan in TryParse.

By the way, this is the same code fragment that I noted when digging in the .NET 6 source code. But back then, the interpolation character was missing in the logging method:

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

You can read more about this issue in the article about .NET 6 check (issue 14).

Issue 4

Since we mentioned methods with strange return values, let's look at another one:

     C# 
   
 
 
   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

The method either returns true or throws an exception — it never returns false. This is a public API, so there's more demand for quality.

Let's look at the description on learn.microsoft.com:

Oopsie. I created an issue on GitHub for it as well, but at the moment of writing this article, there was no news on it.

Issue 5

     C# 
   
 
 
   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

In this code fragment, the same expression — attrib.Length == 0 — is checked twice: first as a right operand of the || operator, then as a condition of the if statement.

Sometimes this may be an error — developers want to check one thing but instead, check another. We were lucky here: the second check was just redundant and the developers removed it.

See the issue on GitHub.

Issue 6

     C# 
   
 
 
   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

The developer created an instance of the XmlTextWriter type. Then a reference to this instance is assigned to the writer variable. However, in the next line the developer checked writer for null. The check always returns true, which means the condition is redundant here.

It's not horrific, but it's better to remove the check. The developers did that, actually (issue on GitHub).

Issue 7

Redundant code again, but this time it's less obvious:

     C# 
   
 
 
   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

Let's look at how the range of the year variable is checked throughout the code execution:

     C# 
   
   ToFourDigitYear(int year, int twoDigitYearMax)

year is a parameter of the int type. Which means its value is within the [int.MinValue; int.MaxValue] range.

When the code is executed, the if statement is met first; in this statement, an exception is thrown:

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

If there's no exception, then the year value is within [0; int.MaxValue].

Then, another if statement:

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

If the code execution is in the then branch of if, then the year value is within the [0; 99] range. This leads to an interesting result — to the operation of taking the remainder of the division:

     C# 
   
   int y = year % 100;

The year value is always less than 100 (i.e., the value is between 0-99). Therefore, the result of the year % 100 operation is always equal to the left operand — year. Thus, y is always equal to year.

Either the code is redundant or it's an error. After I opened the issue on GitHub, the code was fixed and the y variable was removed.

Issue 8

     C# 
   
 
 
   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

We need to dig a bit deeper here. First, let's look at the second if statement:

     C# 
   
 
 
   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;
} 
  

The value of the LastIndirectLocationInput property is written to the input variable. After that, input is checked in two asserts: it's checked for null (input != null) and for the presence of result (input.HasResult).

Let's look at the LastIndirectLocationInput property's body to understand which value can be written to the input variable:

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

On the one hand, the property may return null. On the other hand, if HasIndirectLocationInputs is true, then IndirectLocationInputs[IndirectLocationInputs.Count - 1] is returned instead of explicit null.

The question is, can the value from the IndirectLocationInputs collection be null? Probably yes, although it's not clear from the code. By the way, nullable annotations could help here, but they are not enabled in all .NET projects.

Let's go back to if:

     C# 
   
 
 
   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;
} 
  

The conditional expression is sectionRecord.HasIndirectLocationInputs. It's the same property that's checked in LastIndirectLocationInput, which means LastIndirectLocationInput definitely doesn't return explicit null. However, it's unclear which value will be received from IndirectLocationInputs and written to input.

The developer first checks that input != null and only then checks for the presence of the result — input.HasResult. Looks okay.

Now let's go back to the first if statement:

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

Let's look at the LastLocationInput property:

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

It's written the same way as LastIndirectLocationInput. Just like in the previous case, depending on the flag (HasLocationInputs), either null or a value from the LocationInputs collection is returned.

Now return to the if statement. Its conditional expression is the HasLocationInputs property, which is checked within LastLocationInput. If the code is executed in the then branch of the if statement, this means LastLocationInput cannot return explicit null. Can the value from the LocationInputs collection be null? The question remains unanswered. If it can, then null will be written to input too.

As in the case of the first inspected if, input.HasResult is checked but there's no input != null this time.

Once again. The first inspected code fragment:

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

The second one:

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

Looks like the Debug.Assert(input != null) expression is missing.

I opened an issue on GitHub where I described this and other suspicious places related to null checks (you'll see them below).

The developers decided not to fix this fragment and left it as is:

Issues With Null Checks

I came across several places in code where a reference is dereferenced and only then it's checked for null. I created one issue for all similar code fragments on GitHub.

Let's inspect.

Issue 9

     C# 
   
 
 
   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

First, pOperand1 is dereferenced (pOperand1.ErrorString) and is checked for null in Debug.Assert in the next code line. If pOperand1 is null, then the assert is not triggered, but an exception of the NullReferenceException type is thrown instead.

The code was fixed — pOperand1 is checked before use.

Before:

     C# 
   
   string strOp = pOperand1.ErrorString;
Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);

After:

     C# 
   
   Debug.Assert(pOperand1 != null);
Debug.Assert(pOperand1.Type != null);
string strOp = pOperand1.ErrorString;

Issue 10

     C# 
   
 
 
   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

The _callbacks variable is used first and only then it's checked for null:

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

At the time of writing this article, the developers removed checking _callbacks for null.

By the way, _callbacks is a readonly field that's initialized in a constructor:

     C# 
   
 
 
   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;
  }
  ....
} 
  

In the thread with the fix, the developers discussed whether it was worth adding Debug.Assert and checking _callbacks for null into a constructor. In the end, they decided it wasn't.

Issue 11

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

Location: DocumentSchemaValidator.cs, 421

The controversial code:

The result of the as operator is written to schemaAttribute. If _defaultAttributes[i] – null or the cast failed, the result will be null.
The null-forgiving operator (!) implies that the result of casting cannot be null. Therefore, schemaAttribute cannot be null.
In the next code line, schemaAttribute is dereferenced. Then in a line below, the reference is checked for null.

Here's the question. Can schemaAttribute be null? It's not very clear from the code.

The code was fixed like that:

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

During the discussion of the fix, the developer proposed moving the Debug.Assert call in the line above instead of removing it. The code would look like that:

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

In the end, they decided not to return Assert.

Issue 12

Let's look at the constructor of the XmlConfigurationElementTextContent type:

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

Location: XmlConfigurationElementTextContent.cs, 10

Now let's see where it's used:

     C# 
   
 
 
   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:

Have you noticed anything strange in the code?

Pay attention to the order of arguments and parameters:

arguments: lineNumber, linePosition
parameters: linePosition, lineNumber

I created an issue on GitHub, and the code was fixed: the developers put arguments in the correct order and added a test.

Issue 13

Another suspicious case:

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

Location: DataRelation.cs, 486

Look at the FindForeignKeyConstraint method:

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

Location: ConstraintCollection.cs, 548

Seems like the argument order is mixed up again:

parameters: parent, child
arguments: ChildKey, ParentKey

There's another method call: the argument order is correct there.

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

I created an issue on GitHub. Unfortunately, I haven't received any comments on it at the moment of writing the article.

Issue 14

These are not all places where the argument order is mixed up — I found another one:

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

Location: ConfigurationRootExtensions.cs, 50

Look at the ConfigurationDebugViewContext constructor:

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

Location: ConfigurationDebugViewContext.cs, 11

The order:

parameters: path, key
arguments: child.Key, child.Path

I created an issue on GitHub. According to the developers, this case doesn't have any issues despite the mistake.

However, they still fixed the order of arguments.

Conclusion

The .NET code is of high quality. I believe this is achieved by an established development process — the developers know the exact release date. Besides, release candidates help find the most serious errors and prepare the project for the release.

Nevertheless, I still manage to find something intriguing in the code. This time my favorites are arguments that were mixed up during method calls.

All code fragments described in this article were found by the PVS-Studio analyzer.

GitHub Fragment (logic) .NET

Published at DZone with permission of Sergey Vasiliev. See the original article here.

Opinions expressed by DZone contributors are their own.

Related

Trending