A little over six months ago, .NET 10 was released. Have any defects remained in the project's source code after all these months? Let's try to find the answer to this question. Enjoy the article!
The new version brings a lot of changes: performance improvements, library updates, new APIs, and further enhancements to the compiler and runtime. However, as experience shows, even code in projects of this level can contain errors.
So, we checked the .NET 10 source code (version 10.0.8) using PVS-Studio static analyzer.
During the analysis, we found multiple suspicious code fragments, potential bugs, and some curious snippets worth the developers' attention. In this article, we'll go over the most exciting warnings the analyzer has issued.
As mentioned earlier, quite some time has passed since the release of .NET 10. However, a quick refresher on the new features wouldn't hurt:
Before we dive into the .NET 10 code, I'd like to mention that PVS-Studio continues to expand the list of supported languages. You can now try the analyzers for Go, JavaScript, and TypeScript through the early access program. Sign up here.
Let's dig into the code!
Always false
Code snippet 1
public static void ReflectionWriteValue(....)
{
....
if (memberValue == null) // <=
{
context.WriteNull(xmlWriter,
memberType,
DataContract.IsTypeSerializable(memberType));
}
else
{
PrimitiveDataContract? primitiveContract = ....;
if ( primitiveContract != null
&& primitiveContract.UnderlyingType != Globals.TypeOfObject
&& !writeXsiType)
{
primitiveContract.WriteXmlValue(xmlWriter, memberValue, context);
}
else
{
if ( memberValue == null // <=
&& ( memberType == Globals.TypeOfObject
|| (originValueIsNullableOfT && memberType.IsValueType)))
{
context.WriteNull(xmlWriter,
memberType,
DataContract.IsTypeSerializable(memberType));
}
else
{
ReflectionInternalSerialize(xmlWriter,
context,
memberValue!,
memberValue!.GetType()
.TypeHandle
.Equals(memberType.TypeHandle),
writeXsiType,
memberType,
originValueIsNullableOfT);
}
}
}
....
}
PVS-Studio warning: V3022 Expression 'memberValue == null && (memberType == Globals.TypeOfObject || (originValueIsNullableOfT && memberType.IsValueType))' is always false. ReflectionClassWriter.cs 94
Take a look at the top-level if statement that checks memberValue for null. As a result, in the else block of this if statement, the memberValue variable isn't null. However, this code block contains a check:
if ( memberValue == null
&& ( memberType == Globals.TypeOfObject
|| (originValueIsNullableOfT && memberType.IsValueType)))
This condition is always false because the memberValue == null check is an operand of the && operator.
Precedence error
Code snippet 2
protected override string GetName(NodeFactory factory)
{
return "Reflectable delegate type: "
+ _delegateType?.ToString() ?? "All delegates";
}
PVS-Studio warning: V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. ReflectedDelegateNode.cs 38
Quite often, developers forget that the ?? operator has one of the lowest precedence levels. This may cause issues affecting program logic. Fortunately, the logic in this example remains intact, but the issue still exists. If the delegateType field is null when the GetName method executes, instead of the expected message "Reflectable delegate type: All delegates" we will get "Reflectable delegate type: ". This happens because the + operator has higher precedence than ??. To fix the issue, wrap part of the expression in parentheses:
"Reflectable delegate type: " + (_delegateType?.ToString() ?? "All delegates");
They mixed them up
Code snippet 3
public ISymbolNode GenericLookupHelper(
CORINFO_RUNTIME_LOOKUP_KIND runtimeLookupKind,
ReadyToRunHelperId helperId,
object helperArgument,
GenericContext methodContext)
{
switch (helperId)
{
case ReadyToRunHelperId.TypeHandle:
return GenericLookupTypeHelper(
runtimeLookupKind,
ReadyToRunFixupKind.TypeHandle,
helperArgument,
methodContext);
case ReadyToRunHelperId.MethodHandle:
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.MethodHandle,
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.MethodEntry:
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.MethodEntry,
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.MethodDictionary: // <=
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.MethodHandle, // <=
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.TypeDictionary:
return GenericLookupTypeHelper(
runtimeLookupKind,
ReadyToRunFixupKind.TypeDictionary,
(TypeDesc)helperArgument,
methodContext);
case ReadyToRunHelperId.VirtualDispatchCell:
return GenericLookupMethodHelper(
runtimeLookupKind,
ReadyToRunFixupKind.VirtualEntry,
(MethodWithToken)helperArgument,
methodContext);
case ReadyToRunHelperId.FieldHandle:
return GenericLookupFieldHelper(
runtimeLookupKind,
ReadyToRunFixupKind.FieldHandle,
(FieldWithToken)helperArgument,
methodContext);
default:
throw new NotImplementedException(helperId.ToString());
}
}
PVS-Studio warning: V3139 Two or more case-branches perform the same actions. ReadyToRunSymbolNodeFactory.cs 583
The code contains many similar operations written in the same pattern, so an error could easily have slipped in. If helperId equals ReadyToRunHelperId.MethodDictionary, then ReadyToRunFixupKind.MethodHandle is passed to the GenericLookupMethodHelper method. This doesn't look quite right, since the ReadyToRunFixupKind enum already includes a MethodDictionary element. It seems that GenericLookupMethodHelper should receive it when helperId matches ReadyToRunHelperId.MethodDictionary.
Code snippet 4
partial class MutableModule
{
....
public int CompareTo(MutableModule other)
{
return _index.CompareTo(_index);
}
}
PVS-Studio warning: V3062 An object '_index' is used as an argument to its own method. Consider checking the first actual argument of the 'CompareTo' method. MutableModule.Sorting.cs 19
In the CompareTo method, the _index field is compared to itself. Most likely, other._index should be passed as an argument.
Code snippet 5
public void ComputeReturnValueTreatment(....)
{
....
if (descriptor.passedInRegisters)
{
if (descriptor.eightByteCount == 1)
{
if (descriptor.eightByteClassifications0
== SystemVClassificationType.SystemVClassificationTypeSSE)
{
// Structs occupying just one eightbyte are treated as int / double
fpReturnSize = sizeof(double);
}
}
else
{
// Size of the struct is 16 bytes
fpReturnSize = 16;
// The lowest two bits of the size encode
// the order of the int and SSE fields
if (descriptor.eightByteClassifications0 // <=
== SystemVClassificationType.SystemVClassificationTypeSSE)
{
fpReturnSize += 1;
}
if (descriptor.eightByteClassifications0 // <=
== SystemVClassificationType.SystemVClassificationTypeSSE)
{
fpReturnSize += 2;
}
}
break;
}
....
}
PVS-Studio warning: V3029 The conditional expressions of the 'if' statements situated alongside each other are identical. Check lines: 364, 369. TransitionBlock.cs 364
The analyzer reported a series of if statements with identical conditions. Indeed, descriptor.eightByteClassifications0 is compared twice with SystemVClassificationType.SystemVClassificationTypeSSE. It seems likely that the check is incorrect in one of the cases, given the presence of the eightByteClassifications1 field in descriptor. Most likely, the field should be used in one of the checks.
Are you sure it's not null?
Code snippet 6
public static bool SubstEqualTypeArrays(TypeArray taDst,
TypeArray taSrc,
TypeArray typeArgsCls,
TypeArray typeArgsMeth)
{
// Handle the simple common cases first.
if (taDst == taSrc || (taDst != null && taDst.Equals(taSrc))) // <=
{
// The following assertion is not always true and indicates a problem where
// the signature of override method does not match the one inherited from
// the base class. The method match we have found does not take the type
// arguments of the base class into account.
// So actually we are not overriding the method that we "intend" to.
// Debug.Assert(taDst == SubstTypeArray(taSrc,
typeArgsCls,
typeArgsMeth,
grfst));
return true;
}
if (taDst.Count != taSrc.Count) // <=
return false;
if (taDst.Count == 0)
return true;
var ctx = new SubstContext(typeArgsCls, typeArgsMeth, true);
if (ctx.IsNop)
{
return false;
}
for (int i = 0; i < taDst.Count; i++)
{
if (!SubstEqualTypesCore(taDst[i], taSrc[i], ctx))
return false;
}
return true;
}
PVS-Studio warning: V3125 The 'taDst' object was used after it was verified against null. Check lines: 354, 344. TypeManager.cs 354
In the first if statement, the taDst parameter is checked for null, suggesting that it may be null. However, in the next if statement, the taDst parameter is dereferenced without being checked: taDst.Count. If taDst is null and taSrc isn't, then a NullReferenceException will be thrown.
Code snippet 7
internal void WriteXmlnsAttribute(XmlDictionaryString ns)
{
if (dictionaryWriter != null)
{
if (ns != null)
dictionaryWriter.WriteXmlnsAttribute(null, ns);
}
else
WriteXmlnsAttribute(ns.Value);
}
PVS-Studio warning: V3125 The 'ns' object was used and was verified against null in different execution branches. Check lines: 71, 67. XmlWriterDelegator.cs 71
This issue is similar to the previous one. In the then block of the first if statement, the ns parameter is checked for null before being used. This check is missing from the else block. The ns parameter may be related to the dictionaryWriter field, meaning we don't need to check ns for null if dictionaryWriter is null. However, we couldn't find any evidence of this.
It currently looks like if dictionaryWriter and ns are null, accessing ns.Value will throw a NullReferenceException.
Code snippet 8
private struct ModuleAndIntValueKey : IEquatable<ModuleAndIntValueKey>
{
public bool Equals(ModuleAndIntValueKey other) =>
IntValue == other.IntValue
&& ( (Module == null && other.Module == null)
|| Module.Equals(other.Module));
}
PVS-Studio warning: V3125 The 'Module' object was used after it was verified against null. Check lines: 180, 180. ReadyToRunCodegenNodeFactory.cs 180
When comparing objects of the ModuleAndIntValueKey type, their Module fields are checked:
(Module == null && other.Module == null) || Module.Equals(other.Module)
If Module is null and other.Module isn't, a NullReferenceException will be thrown. Since the left operand of the || operator is false, the condition will be evaluated further, resulting in the dereferencing of Module when Equals is called.
To fix this, we'll add a null check before accessing Module.
Code snippet 9
public bool IsInheritanceChainLayoutFixedInCurrentVersionBubble(TypeDesc type)
{
// This method is not expected to be called for value types
Debug.Assert(!type.IsValueType);
if (type.IsObject)
return true;
if (!IsLayoutFixedInCurrentVersionBubble(type))
{
return false;
}
type = type.BaseType;
if (type != null)
{
return false;
while (!type.IsObject && type != null)
{
if (!IsLayoutFixedInCurrentVersionBubble(type))
{
return false;
}
type = type.BaseType;
}
}
return true;
}
PVS-Studio warning: V3027 The variable 'type' was utilized in the logical expression before it was verified against null in the same logical expression. ReadyToRunCodegenCompilation.cs 596
In the while statement, the IsObject property of the type variable is accessed first, and then type is checked for null. Such a check seems ineffective. If type is null, the NullReferenceException is thrown.
To fix this, we need to either swap the null check and the property access, or, if type is never null, remove the null check.
Tricky comparison
Code snippet 10
public class PropertyPseudoDesc : TypeSystemEntity
{
....
public static bool operator ==(PropertyPseudoDesc a, PropertyPseudoDesc b)
=> a._type == b._type && a._handle == b._handle;
....
}
PVS-Studio warning: V3115 Passing 'null' to '==' operator should not result in 'NullReferenceException'. PropertyPseudoDesc.cs 89
If one of the operands is null when comparing objects of the PropertyPseudoDesc type, the NullReferenceException will be thrown. To avoid this, we need to check whether the operand fields are null before accessing them.
Conclusion
The .NET 10 check showed us that even six months after the release, the project still contains bugs and code snippets that could cause issues. Some of them are classic errors: ambiguous conditions, redundant checks, potential NREs, and confusing logic branches. Such issues are difficult to spot during a quick code review, especially in a large, platform-level codebase.
Every year, we check the latest .NET version:
We also frequently post checks of other popular projects. To stay up to date on new checks, subscribe to our newsletter.
If you'd like to try out PVS-Studio yourself, click the link!
Top comments (0)