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 @@ -284,22 +284,25 @@ public int GetNodeCount()
{
int result = 1;

if (IsNested)
{
result += DeclaringType.GetNodeCount();
}
else if (IsConstructedGenericType)
TypeName? declaring = _declaringType;
while (declaring is not null)
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
{
result++;
declaring = declaring._declaringType;
}
else if (IsArray || IsPointer || IsByRef)

if (IsArray || IsPointer || IsByRef)
{
result += GetElementType().GetNodeCount();
}

foreach (TypeName genericArgument in GetGenericArguments())
else if (IsConstructedGenericType)
{
result += genericArgument.GetNodeCount();
result += GetGenericTypeDefinition().GetNodeCount();

foreach (TypeName genericArgument in GetGenericArguments())
{
result += genericArgument.GetNodeCount();
Copy link
Member

Choose a reason for hiding this comment

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

Should this use checked arithmetic to avoid silent overflows?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, because we can not parse more than Int32.MaxValue nodes because TypeNameParseOptions.MaxNodes is also an Int32.

Copy link
Member

Choose a reason for hiding this comment

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

TypeName.Parse is not the only way to create new type names after the last PR. Try something like this:

using System.Reflection.Metadata;
using System.Collections.Immutable;

var simpleTypeName = TypeName.Parse("A");
var tn = simpleTypeName;

for (int i = 0; i < 20; i++)
{
    tn = simpleTypeName.MakeGenericTypeName(ImmutableArray.Create(tn, tn, tn));
    Console.WriteLine(tn.GetNodeCount());
}

It is going to hit the overflow in this code.

}
}

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 @@ -67,13 +67,13 @@ private TypeNameParser(ReadOnlySpan<char> name, bool throwOnError, TypeNameParse
// 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 +147,31 @@ 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 definition represents a nested type, this needs to be taken into account.
if (nestedNameLengths is not null)
{
recursiveDepth += nestedNameLengths.Count;
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
if (IsMaxDepthExceeded(_parseOptions, recursiveDepth))
{
return null;
}
}
}

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 +262,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 @@ -388,5 +394,18 @@ internal static void ThrowInvalidOperation_HasToBeArrayClass()
throw new InvalidOperationException();
#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);
}
}
}
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