Errors and suspicious places in the sources of .NET 8

Errors and suspicious places in the sources of .NET 8

Every year a new version of .NET is released. This event not only gives us an opportunity to learn about the latest improvements in .NET itself and innovations in the language, but also gives us a reason to explore the .NET source code. You need to take advantage of this chance!

By the way, we already have several articles dedicated to the latest updates in the world of .NET and C#. If you are interested in what Microsoft has added this time, I recommend that you look at the following materials:

In these articles, we look at the key changes that have appeared in the latest versions of .NET and C#. I invite you to read to keep up with the latest developments.

In addition, in the latest PVS-Studio 7.28 release, support for analysis of projects using .NET 8 has already been implemented. The source analysis used the .NET 8 release code, which is available on GitHub at the link.

Before we start looking at the bugs found in .NET 8, I want to tell a little story.

As you know, .NET is huge and that can cause problems. There is a script in the source code that allows you to generate solutions for .NET libraries. I analyzed this solution using the PVS-Studio console utility. I started studying the report in the IDE in which I work Visual Studio 2022, but a problem arose. When trying to navigate through Visual Studio 2022 code, something unexpected happened: either the IDE restarted or it simply quit. Moreover, this behavior is repeated not only when navigating the code using the PVS-Studio plugin, but also when switching between files, ‘Go To Definition’, etc.

This made work difficult, but a solution was found quickly.

We recently added support for analyzing .NET projects in VS Code. There is a separate article about this: “Using the VS Code extension “PVS-Studio” to effectively deal with errors in C# code”. Given that VS Code is a lightweight code editor, we didn’t experience the same difficulties we encountered in Visual Studio 2022.

This is what the PVS-Studio window looks like in Visual Studio Code:

.NET is a powerful platform that has high code standards, is written by real professionals, and is well tested. However, even in such a cool project, PVS-Studio is able to find errors.

And now let’s move on to the consideration of detected errors.

Code snippet 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 Warning: V3181 The result of the ‘&’ operator is ‘0’ because the value of ‘SettingsManageability.Roaming’ is ‘0’. LocalFileSettingsProvider.cs 411

In the case of an enumeration constant value SettingsManageability.Roaming equals 0. Since the result of a bitwise “AND” with operand 0 is always equal to 0, it turns out that 0 is compared with 0. It turns out that the result of the expression ((manageAttr.Manageability & SettingsManageability.Roaming) == SettingsManageability.Roaming always is true.

Developers should pay attention to this code.

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

Warning PVS-Studio: V3025 The 1st argument ‘” %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 parser reports an invalid format string in the first argument of the method Trace. Let’s look at this 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 a format string. Arguments are placed in this line. Only the arguments must be placed in placeholders of the form {0}, {1}, etc. There are no such placeholders in this line. Using such a format string will result in an exception of type being thrown System.FormatException about incorrect format.

You may need to use some other login method. If you go through other places using the method Tracethen everything is used correctly there, and the format lines contain markers:

1095_NET8_Errors_ua/image3.png

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

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

In this fragment, you can see that there is a double assignment of a variable ResPrec.

Since between these two operations ResPrec is not used, it indicates an error.

There are two options:

  • One of the appropriations is redundant – no big deal, just an extra operation, though it’s not good;

  • Between the two assignments is a variable ResPrec should be used – this will be an unpleasant error.

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

Warning PVS-Studio: V3022 Expression ‘_validationState == ValidatingReaderState.OnReadBinaryContent’ is always false. XsdValidatingReader.cs 1302

PVS-Studio found that the latter condition if (_validationState == ValidatingReaderState.OnReadBinaryContent) will always be false. Let’s understand why.

Let’s look at the first operator if. It has a field _validationState assigned to:

Therefore, the value of the field cannot be equal ValidatingReaderState.OnReadBinaryContentand the code inside if is not performed.

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

Warning PVS-Studio: V3102 Object access to the ‘type.Instantiation’ element of the object appears as a significant index inside. TypeLoaderEnvironment.GVMResolution.cs 32

Suppose that in this piece of code, a record of the following form is formed from information about the type: ConsoleApp1.Program.MyClass. Here only in the cycle they refer to the object type.Instantiation by a constant index equal to 0. It is possible that everything works as it should, but it looks very strange. Waiting to see GetTypeNameDebug(type.Instantiation[i]).

And yes, I immediately went and checked that everything is displayed correctly in the Visual Studio 2022 debugger, but it is possible that somewhere you can find a type display with an error :).

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

PVS-Studio Warning: V3125 The ‘stack_instr!’ the object was used after it was committed to null. Check lines: 1918, 1913. UnreachableBlocksOptimizer.cs 1918

The developer used the ‘?.’ operator, implying that the field stack_instr may be null. And apparently everything is fine, there is a check, but it was not here. A null reference may be dereferenced in the specified line. Most likely, the developer thought that the expression stack_instr?.Count < length at stack_instr equal null will return trueand there will be an exit from the method, but no – the result will be false.

Moreover, the developer suppressed the compiler’s message about possible dereferencing null link with ‘!’, because thought the compiler’s static analysis just failed and didn’t understand the check.

And what do you think about the nullable context? If our opinion is interesting, or if you are not yet familiar with this mechanism, I suggest you read our articles:

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

Warning PVS-Studio: V3095 ‘ResolvedType’ object was used before it was implemented after null. Check lines: 34, 55. TypeHierarchyCache.cs 34

Parameter resolvedType first used, but before adding to the cache is checked for null. It turns out strange somehow. The analyzer pointed to resolvedType.NameBut the program will crash even earlier. Method TryGetValue will throw an exception if the first argument resolvedType will be null.

Code fragment 8

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

Warning PVS-Studio: V3001 There are identical sub-expressions ‘tr.Namespace’ to the right and the right ‘==’ operator. TypeReferenceExtensions.cs 365

The analyzer found that this code compares two identical subexpressions. A simple but offensive mistake. tr.Namespace compared with tr.Namespaceand must with type.Namespace.

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

Warning PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling the ‘Write’ function. Arguments not used: ClassName ?? “null”. EHInfo.cs 135

Another error with the format string, but this time for a class TextWriter. The developer used the string interpolation symbol $. The number 0 will simply be substituted into the string, and the format string will be “CATCH: 0”. As a result, the text that you wanted to replace the placeholder {0} is not used. The same error in the next one case.

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

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

Argument _outerReader is passed to the constructor twice. If you look at the constructor declaration, you can see that the constructor has a named parameter metadataReader:

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

When the constructor is called, the field is available _metadataReader. Perhaps it should be used as the third argument.

Code fragment 11 is a bonus

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

Warning PVS-Studio: V3001 There are identical sub-expressions ‘context.Target.PointerSize == 4’ to the left and right ‘&&’ operator. MetadataFieldLayoutAlgorithm.cs 648

The expression is checked twice context.Target.PointerSize == 4. In the instance method GetObjectAlignment change context.Target.PointerSize doesn’t happen Maybe something else should be checked here, or maybe it’s just an extra check.

As I’ve written before, .NET has high quality code. However, I never cease to be amazed at some of the mistakes found in projects of this size. Well-established development process, top-class developers, but still there are bugs and surprises in the code. Of course, this is normal, there is no perfect code, but you can and should strive for it.

I suggest that you also check your project for oddities and errors. You can try the analyzer by following the link. Write to us if there are any questions – we promptly solve all the problems that have arisen :).

If you would like to share this article with an English-speaking audience, please use the translation link: Artem Rovenskii. Bugs and suspicious places in .NET 8 source code.

Related posts