Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[TypeName] Nested types should respect MaxNode count #106334

Merged
merged 11 commits into from
Sep 3, 2024
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,13 @@
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection.Metadata;
using System.Text;

namespace System.Reflection.Metadata
{
internal struct TypeNameParseOptions
{
public TypeNameParseOptions() { }
#pragma warning disable CA1822 // Mark members as static
// CoreLib does not enforce any limits
public bool IsMaxDepthExceeded(int _) => false;
public int MaxNodes
{
get
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -301,24 +301,28 @@ public string Name
/// </remarks>
public int GetNodeCount()
{
// This method uses checked arithmetic to avoid silent overflows.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add test to validate at least one of the overflow cases? It can be an outer loop test since it takes a bit of time.

// It's impossible to parse a TypeName with NodeCount > int.MaxValue
// (TypeNameParseOptions.MaxNodes is an int), but it's possible
// to create such names with the Make* APIs.
int result = 1;

if (IsNested)
if (IsArray || IsPointer || IsByRef)
{
result += DeclaringType.GetNodeCount();
result = checked(result + GetElementType().GetNodeCount());
}
else if (IsConstructedGenericType)
{
result++;
}
else if (IsArray || IsPointer || IsByRef)
{
result += GetElementType().GetNodeCount();
}
result = checked(result + GetGenericTypeDefinition().GetNodeCount());

foreach (TypeName genericArgument in GetGenericArguments())
foreach (TypeName genericArgument in GetGenericArguments())
{
result = checked(result + genericArgument.GetNodeCount());
}
}
else if (IsNested)
{
result += genericArgument.GetNodeCount();
result = checked(result + DeclaringType.GetNodeCount());
}

return result;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ private TypeNameParser(ReadOnlySpan<char> name, bool throwOnError, TypeNameParse
{
if (throwOnError)
{
if (parser._parseOptions.IsMaxDepthExceeded(recursiveDepth))
if (IsMaxDepthExceeded(parser._parseOptions, recursiveDepth))
{
ThrowInvalidOperation_MaxNodesExceeded(parser._parseOptions.MaxNodes);
}
Expand All @@ -61,19 +61,21 @@ private TypeNameParser(ReadOnlySpan<char> name, bool throwOnError, TypeNameParse
return null;
}

Debug.Assert(parsedName.GetNodeCount() == recursiveDepth, $"Node count mismatch for '{typeName.ToString()}'");

return parsedName;
}

// this method should return null instead of throwing, so the caller can get errorIndex and include it in error msg
private TypeName? ParseNextTypeName(bool allowFullyQualifiedName, ref int recursiveDepth)
{
if (!TryDive(ref recursiveDepth))
if (!TryDive(_parseOptions, ref recursiveDepth))
{
return null;
}

List<int>? nestedNameLengths = null;
if (!TryGetTypeNameInfo(ref _inputString, ref nestedNameLengths, out int fullTypeNameLength))
if (!TryGetTypeNameInfo(_parseOptions, ref _inputString, ref nestedNameLengths, ref recursiveDepth, out int fullTypeNameLength))
{
return null;
}
Expand Down Expand Up @@ -147,14 +149,24 @@ private TypeNameParser(ReadOnlySpan<char> name, bool throwOnError, TypeNameParse
{
_inputString = capturedBeforeProcessing;
}
else
{
// Every constructed generic type needs the generic type definition.
if (!TryDive(_parseOptions, ref recursiveDepth))
{
return null;
}
// If that generic type is a nested type, we don't increase the recursiveDepth any further,
// as generic type definition uses exactly the same declaring type as the constructed generic type.
}

int previousDecorator = default;
// capture the current state so we can reprocess it again once we know the AssemblyName
capturedBeforeProcessing = _inputString;
// iterate over the decorators to ensure there are no illegal combinations
while (TryParseNextDecorator(ref _inputString, out int parsedDecorator))
{
if (!TryDive(ref recursiveDepth))
if (!TryDive(_parseOptions, ref recursiveDepth))
{
return null;
}
Expand Down Expand Up @@ -245,15 +257,5 @@ private bool TryParseAssemblyName(ref AssemblyNameInfo? assemblyName)

return declaringType;
}

private bool TryDive(ref int depth)
{
if (_parseOptions.IsMaxDepthExceeded(depth))
{
return false;
}
depth++;
return true;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,8 @@ internal static bool IsBeginningOfGenericArgs(ref ReadOnlySpan<char> span, out b
return false;
}

internal static bool TryGetTypeNameInfo(ref ReadOnlySpan<char> input, ref List<int>? nestedNameLengths, out int totalLength)
internal static bool TryGetTypeNameInfo(TypeNameParseOptions options, ref ReadOnlySpan<char> input,
ref List<int>? nestedNameLengths, ref int recursiveDepth, out int totalLength)
{
bool isNestedType;
totalLength = 0;
Expand Down Expand Up @@ -248,6 +249,11 @@ internal static bool TryGetTypeNameInfo(ref ReadOnlySpan<char> input, ref List<i
#endif
if (isNestedType)
{
if (!TryDive(options, ref recursiveDepth))
{
return false;
}

(nestedNameLengths ??= new()).Add(length);
totalLength += 1; // skip the '+' sign in next search
}
Expand Down Expand Up @@ -389,6 +395,19 @@ internal static void ThrowInvalidOperation_HasToBeArrayClass()
#endif
}

internal static bool IsMaxDepthExceeded(TypeNameParseOptions options, int depth)
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
#if SYSTEM_PRIVATE_CORELIB
=> false; // CoreLib does not enforce any limits
#else
=> depth > options.MaxNodes;
jkotas marked this conversation as resolved.
Show resolved Hide resolved
#endif

internal static bool TryDive(TypeNameParseOptions options, ref int depth)
{
depth++;
return !IsMaxDepthExceeded(options, depth);
}

#if SYSTEM_REFLECTION_METADATA
[DoesNotReturn]
internal static void ThrowInvalidOperation_NotSimpleName(string fullName)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,5 @@ public int MaxNodes
_maxNodes = value;
}
}

internal bool IsMaxDepthExceeded(int depth) => depth >= _maxNodes;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -122,14 +122,17 @@ public void TryGetTypeNameInfoGetsAllTheInfo(string input, bool expectedResult,
{
List<int>? nestedNameLengths = null;
ReadOnlySpan<char> span = input.AsSpan();
bool result = TypeNameParserHelpers.TryGetTypeNameInfo(ref span, ref nestedNameLengths, out int totalLength);
TypeNameParseOptions options = new();
int recursiveDepth = 0;
bool result = TypeNameParserHelpers.TryGetTypeNameInfo(options, ref span, ref nestedNameLengths, ref recursiveDepth, out int totalLength);

Assert.Equal(expectedResult, result);

if (expectedResult)
{
Assert.Equal(expectedNestedNameLengths, nestedNameLengths?.ToArray());
Assert.Equal(expectedTotalLength, totalLength);
Assert.Equal(expectedNestedNameLengths?.Length ?? 0, recursiveDepth);
}
}

Expand Down
Loading
Loading