Artem Rovenskii

Dec 22 2023

Tags:

#CSharp

Bugs and suspicious places in .NET 8 source code

Dec 22 2023

Author: Artem Rovenskii

Every year, Microsoft releases a new version of .NET. This annual event is a great chance to learn about the latest .NET enhancements as well as to explore the .NET source code. Let's take the bull by the horns!

By the way, we have several articles about the latest updates in .NET and C#. If you're interested in the new features Microsoft has added, I suggest you read the following:

In these articles, we take a look at the key changes that have been added in the latest versions of .NET and C#. Check them out to keep up with the latest updates.

Moreover, the latest PVS-Studio 7.28 release already supports the analysis of .NET 8 projects. To examine .NET 8, we've looked at the release code available on GitHub at this link.

Before we start to explore the bugs found in .NET 8, I'd like to tell you a little story.

As we know, .NET is huge, and that can be a problem. Its source code contains a script for generating a solution for .NET libraries. I analyzed the solution with the PVS-Studio console utility. Then I started to study the report in the IDE I worked in — Visual Studio 2022 — but there was an issue. When I was trying to navigate through the code in Visual Studio 2022, something unexpected happened: either the IDE restarted or just terminated. Moreover, the behavior repeated not only when we were navigating through the code with the PVS-Studio plugin but also when switching files, using 'Go To Definition', etc.

It complicated the process, but our team quickly found a solution.

We recently added support for analyzing .NET projects in VS Code. We had a separate article about it: "Using the PVS-Studio extension for VS Code to effectively handle errors in C# code." We don't encounter the same issues as in Visual Studio 2022 because VS Code is a lightweight code editor.

The PVS-Studio window looks like this:

.NET is a powerful platform with high standards for code. It's written and well-tested by experienced developers. However, PVS-Studio can detect errors even in such an impressive project.

Now go ahead and explore the detected errors.

Fragment 1

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

The PVS-Studio warning: V3181 The result of '&' operator is '0' because the value of 'SettingsManageability.Roaming' is '0'. LocalFileSettingsProvider.cs 411

In this case, the value of the SettingsManageability.Roaming enumeration constant is 0. Since the result of a bitwise AND with an operand equal to 0 is always 0, it means that 0 is compared to 0. The output of the ((manageAttr.Manageability & SettingsManageability.Roaming) == SettingsManageability.Roaming) expression is always true.

This code fragment is worth paying attention to.

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

The PVS-Studio warning: 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

The analyzer reports an incorrect format string in the first argument of the Trace method. Let's take a look at the method:

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

Indeed, the first argument is used as the format string. Arguments are substituted into this line. However, the arguments are to be substituted into placeholders of the format {0}, {1}, etc. There are no such placeholders in this string. Using such a format string results in an exception of the System.FormatException type being thrown, reporting an incorrect format.

Perhaps another logging method needs to be used. If we go through other places where the Trace method is used, everything is correct. The format strings contain markers:

Fragment 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;                            // <=
  ....
}

The PVS-Studio warning: V3008 The 'ResPrec' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1689, 1685. SQLDecimal.cs 1689

We can see the ResPrec variable is assigned values twice here.

Since ResPrec isn't used between these two operations, it indicates an error.

Here are two options:

One of the assignments is redundant. It's no big deal, it's just an unnecessary operation, although it's not good.
The ResPrec variable should be used between two assignments — and that is a truly unpleasant error.

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

The PVS-Studio warning: V3022 Expression '_validationState == ValidatingReaderState.OnReadBinaryContent' is always false. XsdValidatingReader.cs 1302

PVS-Studio has detected that the last if (_validationState == ValidatingReaderState.OnReadBinaryContent) condition is always false. Let's see why it may happen.

Let's take a look at the first if statement. There, the _validationState field is assigned:

in then branch — ValidatingReaderState.OnAttribute
in else branch — ValidatingReaderState.OnDefaultAttribute

Therefore, the field value can't be equal to ValidatingReaderState.OnReadBinaryContent, and the code inside if isn't executed.

Fragment 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
  {
    ....
  }
  ....
}

The PVS-Studio warning: V3102 Suspicious access to element of 'type.Instantiation' object by a constant index inside a loop. TypeLoaderEnvironment.GVMResolution.cs 32

Here, maybe the following string is created from information about type: ConsoleApp1.Program.MyClass<string, int, double>. However, the type.Instantiation object is accessed in the loop by a constant index of 0. It is possible that it works as it should, but it looks odd because GetTypeNameDebug(type.Instantiation[i]) is expected.

I've immediately checked it. The Visual Studio 2022 debugger displays everything correctly, but, unexceptionally, we can encounter a type displayed with an error somewhere :).

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

The PVS-Studio warning: V3125 The 'stack_instr!' object was used after it was verified against null. Check lines: 1918, 1913. UnreachableBlocksOptimizer.cs 1918

The developer used the '?.' operator, suggesting that the stack_instr field could be null. Everything seemed fine, there was a check, but... no such luck. It was possible to dereference a null reference in the specified line. Most likely, the developer thought that the stack_instr?.Count < length expression with stack_instr equal to null would return true, and the method would exit. But no — the result would be false.

Moreover, the developer suppressed the compiler message about possible dereference of a null reference with '!'. They thought that the static analysis of the compiler just failed and didn't understand the check.

What do you think about nullable context? If you're interested in our opinion or not yet familiar with it, I suggest you read our articles:

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

The PVS-Studio warning: V3095 The 'resolvedType' object was used before it was verified against null. Check lines: 34, 55. TypeHierarchyCache.cs 34

The resolvedType parameter is used first but is checked for null before being added to the cache. That's kind of weird. The analyzer points to resolvedType.Name, but issues will arise even earlier. The TryGetValue method throws an exception if the first resolvedType argument is null.

Fragment 8

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

The PVS-Studio warning: V3001 There are identical sub-expressions 'tr.Namespace' to the left and to the right of the '==' operator. TypeReferenceExtensions.cs 365

The analyzer has detected that two identical subexpressions are compared here. It's a simple yet frustrating error. Instead of being compared to type.Namespace, tr.Namespace is being compared to tr.Namespace.

Fragment 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;
    ....
  }
  ....
}

The PVS-Studio warning: V3025 Incorrect format. A different number of format items is expected while calling 'Write' function. Arguments not used: ClassName ?? "null". EHInfo.cs 135

Here is another format string error, but in the TextWriter class. The developer used the string interpolation '$' character. It simply substitutes 0 into the string, and the format string will be equal to " CATCH: 0". As a result, the text they wanted to substitute for the {0} placeholder isn't used. The same error occurs in the next case method.

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

The PVS-Studio warning: V3038 The argument was passed to constructor several times. It is possible that other argument should be passed instead. ReadyToRunSignature.cs 707

The _outerReader argument is passed to the constructor twice. If we look at the constructor declaration, we can see that the constructor has the metadataReader parameter:

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

The _metadataReader field is available when calling the constructor. Perhaps it would be better to use this field as the third argument.

Bonus fragment 11

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

The PVS-Studio warning: V3001 There are identical sub-expressions 'context.Target.PointerSize == 4' to the left and to the right of the '&&' operator. MetadataFieldLayoutAlgorithm.cs 648

The expression checks context.Target.PointerSize == 4 twice. In the GetObjectAlignment instance method, context.Target.PointerSize isn't changed. It's possible that something else should be checked here, or maybe it's just an unnecessary check.

As I've written before, .NET has high-quality code. However, I'm always amazed at some of the errors that are found in such big projects. They have a well-oiled development process and experienced developers, but still, there are bugs and suspicious places in the code. Indeed, it's OK; perfect code doesn't exist, but you can and should strive for it.

Check your project for oddities and bugs, too. Try the analyzer at the link. Contact us if you have any questions — we will quickly solve any issues :).

#CSharp