Strange code fragments - Githubissues

rip-mm commented 8 months ago

Hi! I have checked the .NET 8 release version with the PVS-Studio static analyzer. If you are interested, you could take a look at the errors and suspicious code fragments we found:

[ ] 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);
}
```
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. Link to the method.

[ ] 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 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. Link to the method.

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

We can see the ResPrec variable is assigned values twice here. Since ResPrec isn't used between these two operations, it indicates an error. Link to the method.

[ ] 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;
}
}
```
Analyzer 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. Link to the method.

[x] 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
{
....
}
....
}

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. Link to the method.

[ ] 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 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. Link to the method.

[ ] 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 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. Link to the method.

[x] Fragment 8
```
public static bool IsTypeOf<T> (this TypeReference tr)
{
var type = typeof (T);
return tr.Name == type.Name && tr.Namespace == tr.Namespace;
}
```
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. Link to the method.

[x] 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 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. Link to the 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 _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. Link to the method.

[x] Fragment 11
```
protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(....)
{
bool requiresAlign8 
=    !largestAlignmentRequired.IsIndeterminate 
  && context.Target.PointerSize == 4
  && context.Target.GetObjectAlignment(....).AsInt > 4 
  && context.Target.PointerSize == 4;
}
```
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. Link to the method.

ghost commented 8 months ago

Tagging subscribers to this area: @dotnet/area-system-configuration See info in area-owners.md if you want to be subscribed.

Issue Details

Hi! I have checked the .NET 8 release version with the PVS-Studio static analyzer. If you are interested, you could take a look at the errors and suspicious code fragments we found: **Fragment 1** ``` private static bool IsRoamingSetting(SettingsProperty setting) { List> callSitesByIndex = new(); .... SettingsManageabilityAttribute manageAttr = ....; return manageAttr != null && ((manageAttr.Manageability & SettingsManageability.Roaming) == SettingsManageability.Roaming); } ``` 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/LocalFileSettingsProvider.cs#L408). **Fragment 2** ``` internal DataView(....) { .... DataCommonEventSource.Log.Trace(" %d#, table=%d, RowState=%d{ds.DataViewRowState}\n", ObjectID, (table != null) ? table.ObjectID : 0, (int)RowState); .... } ``` 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(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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Data.Common/src/System/Data/DataView.cs#L163). **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; // <= .... } ``` We can see the `ResPrec` variable is assigned values twice here. Since `ResPrec` isn't used between these two operations, it indicates an error. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLDecimal.cs#L1653). **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; } } ``` Analyzer 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdValidatingReader.cs#L1272). **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 { .... } .... } ``` Here, maybe the following string is created from information about type: ConsoleApp1.Program.MyClass. 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.GVMResolution.cs#L22). **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 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/tools/illink/src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs#L1936). **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 `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`. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/tools/illink/src/linker/Linker/TypeHierarchyCache.cs#L28). **Fragment 8** ``` public static bool IsTypeOf (this TypeReference tr) { var type = typeof (T); return tr.Name == type.Name && tr.Namespace == tr.Namespace; } ``` 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`. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/tools/illink/src/linker/Linker/TypeReferenceExtensions.cs#L362). **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 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/EHInfo.cs#L115). **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 `_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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunSignature.cs#L639). **Fragment 11** ``` protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(....) { bool requiresAlign8 = !largestAlignmentRequired.IsIndeterminate && context.Target.PointerSize == 4 && context.Target.GetObjectAlignment(....).AsInt > 4 && context.Target.PointerSize == 4; } ``` 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs#L521).

Author:	rip-mm
Assignees:	-
Labels:	`area-System.Configuration`, `untriaged`
Milestone:	-

ghost commented 8 months ago

Tagging subscribers to this area: @dotnet/area-meta See info in area-owners.md if you want to be subscribed.

Issue Details

Hi! I have checked the .NET 8 release version with the PVS-Studio static analyzer. If you are interested, you could take a look at the errors and suspicious code fragments we found: **Fragment 1** ``` private static bool IsRoamingSetting(SettingsProperty setting) { List> callSitesByIndex = new(); .... SettingsManageabilityAttribute manageAttr = ....; return manageAttr != null && ((manageAttr.Manageability & SettingsManageability.Roaming) == SettingsManageability.Roaming); } ``` 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Configuration.ConfigurationManager/src/System/Configuration/LocalFileSettingsProvider.cs#L408). **Fragment 2** ``` internal DataView(....) { .... DataCommonEventSource.Log.Trace(" %d#, table=%d, RowState=%d{ds.DataViewRowState}\n", ObjectID, (table != null) ? table.ObjectID : 0, (int)RowState); .... } ``` 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(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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Data.Common/src/System/Data/DataView.cs#L163). **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; // <= .... } ``` We can see the `ResPrec` variable is assigned values twice here. Since `ResPrec` isn't used between these two operations, it indicates an error. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Data.Common/src/System/Data/SQLTypes/SQLDecimal.cs#L1653). **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; } } ``` Analyzer 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/libraries/System.Private.Xml/src/System/Xml/Core/XsdValidatingReader.cs#L1272). **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 { .... } .... } ``` Here, maybe the following string is created from information about type: ConsoleApp1.Program.MyClass. 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/nativeaot/System.Private.TypeLoader/src/Internal/Runtime/TypeLoader/TypeLoaderEnvironment.GVMResolution.cs#L22). **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 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/tools/illink/src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs#L1936). **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 `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`. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/tools/illink/src/linker/Linker/TypeHierarchyCache.cs#L28). **Fragment 8** ``` public static bool IsTypeOf (this TypeReference tr) { var type = typeof (T); return tr.Name == type.Name && tr.Namespace == tr.Namespace; } ``` 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`. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/tools/illink/src/linker/Linker/TypeReferenceExtensions.cs#L362). **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 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/EHInfo.cs#L115). **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 `_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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/ReadyToRunSignature.cs#L639). **Fragment 11** ``` protected ComputedInstanceFieldLayout ComputeAutoFieldLayout(....) { bool requiresAlign8 = !largestAlignmentRequired.IsIndeterminate && context.Target.PointerSize == 4 && context.Target.GetObjectAlignment(....).AsInt > 4 && context.Target.PointerSize == 4; } ``` 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. [Link to the method](https://github.com/dotnet/runtime/blob/9cc490e89f635db3712d8e955e85b232a4c3f560/src/coreclr/tools/Common/TypeSystem/Common/MetadataFieldLayoutAlgorithm.cs#L521).

Author:	rip-mm
Assignees:	-
Labels:	`area-Meta`, `untriaged`
Milestone:	-

filipnavara commented 8 months ago

Fragment 1

This code dates back all the way to the .NET Framework. Unfortunately, it seems to be something that slipped through the API review. The SettingsManageability enum definition doesn't make any sense as it is. That said, you can still achieve the desired effect by including the SettingsManageabilityAttribute (implies SettingsManageability.Roaming) or not including it (implies local profile). The code error resulting from this API definition error is quite harmless. Ideally we could fix the API to add SettingsManageability.Local with non-zero value but since it's a legacy API I don't think that is worth the effort. We may just remove the ((manageAttr.Manageability & SettingsManageability.Roaming) == SettingsManageability.Roaming) condition and add a comment instead. Thoughts?

Fragment 8/9

Submitted PRs to fix those since they are obvious cut & paste errors.

filipnavara commented 8 months ago

Fragment 11

It was pointed out in the original PR (https://github.com/dotnet/runtime/pull/73738/files#r943221072) and never fixed.

dotnet / runtime

Strange code fragments #96310