From 6cdd040ee2c53a221f59cb09b175f529683f7cc5 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 13 Aug 2024 13:07:10 +0200 Subject: [PATCH 1/8] add a failing unit test --- .../tests/Metadata/TypeNameTests.cs | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs index a5e22b5d6c503b..bf1221b1b2082e 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs @@ -238,6 +238,45 @@ static void Validate(TypeName parsed, int maxDepth) } } + [Theory] + [InlineData(10)] + [InlineData(100)] + public void MaxNodesIsRespected_TooManyNestedNames(int maxDepth) + { + TypeNameParseOptions options = new() + { + MaxNodes = maxDepth + }; + + string tooMany = GetName(maxDepth); + string notTooMany = GetName(maxDepth - 1); + + Assert.Throws(() => TypeName.Parse(tooMany.AsSpan(), options)); + Assert.False(TypeName.TryParse(tooMany.AsSpan(), out _, options)); + + TypeName parsed = TypeName.Parse(notTooMany.AsSpan(), options); + Validate(parsed, maxDepth - 1); + + Assert.True(TypeName.TryParse(notTooMany.AsSpan(), out parsed, options)); + Validate(parsed, maxDepth - 1); + + static string GetName(int depth) + => $"Namespace.NotNestedType+{string.Join("+", Enumerable.Repeat("NestedType", depth))}"; + + static void Validate(TypeName parsed, int maxDepth) + { + Assert.True(parsed.IsNested); + + while (parsed.IsNested) + { + maxDepth--; + parsed = parsed.DeclaringType; + } + + Assert.Equal(0, maxDepth); + } + } + public static IEnumerable GenericArgumentsAreSupported_Arguments() { yield return new object[] From da4df6904f32703db7d7f0232084128575847fa3 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 13 Aug 2024 13:07:42 +0200 Subject: [PATCH 2/8] the fix --- .../src/System/Reflection/TypeNameResolver.cs | 5 ---- .../Reflection/Metadata/TypeNameParser.cs | 18 +++---------- .../Metadata/TypeNameParserHelpers.cs | 26 ++++++++++++++++++- .../Metadata/TypeNameParserOptions.cs | 2 -- .../Metadata/TypeNameParserHelpersTests.cs | 5 +++- 5 files changed, 33 insertions(+), 23 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs index 9e36505b390eac..61e1996c50e19f 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs @@ -4,16 +4,12 @@ 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 @@ -22,7 +18,6 @@ public int MaxNodes return 0; } } -#pragma warning restore CA1822 } } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs index 5a5bbe04a0c18c..4fadebb4a3e8d8 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs @@ -49,7 +49,7 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse { if (throwOnError) { - if (parser._parseOptions.IsMaxDepthExceeded(recursiveDepth)) + if (IsMaxDepthExceeded(parser._parseOptions, recursiveDepth)) { ThrowInvalidOperation_MaxNodesExceeded(parser._parseOptions.MaxNodes); } @@ -67,13 +67,13 @@ private TypeNameParser(ReadOnlySpan 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? 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; } @@ -154,7 +154,7 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse // 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; } @@ -245,15 +245,5 @@ private bool TryParseAssemblyName(ref AssemblyNameInfo? assemblyName) return declaringType; } - - private bool TryDive(ref int depth) - { - if (_parseOptions.IsMaxDepthExceeded(depth)) - { - return false; - } - depth++; - return true; - } } } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs index 2fa33ea1dfe3bf..f8916b5c016c7e 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs @@ -220,7 +220,8 @@ internal static bool IsBeginningOfGenericArgs(ref ReadOnlySpan span, out b return false; } - internal static bool TryGetTypeNameInfo(ref ReadOnlySpan input, ref List? nestedNameLengths, out int totalLength) + internal static bool TryGetTypeNameInfo(TypeNameParseOptions options, ref ReadOnlySpan input, + ref List? nestedNameLengths, ref int recursiveDepth, out int totalLength) { bool isNestedType; totalLength = 0; @@ -248,6 +249,11 @@ internal static bool TryGetTypeNameInfo(ref ReadOnlySpan input, ref List false; +#else + => depth >= options.MaxNodes; +#endif + + internal static bool TryDive(TypeNameParseOptions options, ref int depth) + { + if (IsMaxDepthExceeded(options, depth)) + { + return false; + } + depth++; + return true; + } } } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs index 551876e73147c9..b7420c40aa9eab 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs @@ -27,7 +27,5 @@ public int MaxNodes _maxNodes = value; } } - - internal bool IsMaxDepthExceeded(int depth) => depth >= _maxNodes; } } diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameParserHelpersTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameParserHelpersTests.cs index b9d23a2be15244..01ec45b50cc3e1 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameParserHelpersTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameParserHelpersTests.cs @@ -122,7 +122,9 @@ public void TryGetTypeNameInfoGetsAllTheInfo(string input, bool expectedResult, { List? nestedNameLengths = null; ReadOnlySpan 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); @@ -130,6 +132,7 @@ public void TryGetTypeNameInfoGetsAllTheInfo(string input, bool expectedResult, { Assert.Equal(expectedNestedNameLengths, nestedNameLengths?.ToArray()); Assert.Equal(expectedTotalLength, totalLength); + Assert.Equal(expectedNestedNameLengths?.Length ?? 0, recursiveDepth); } } From f3183e04e95ec7db3d6e5610ae1839a642f4757f Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 13 Aug 2024 14:50:52 +0200 Subject: [PATCH 3/8] handle 100% of the cases: nested generic types, generics of nested generics --- .../src/System/Reflection/TypeNameResolver.cs | 2 + .../System/Reflection/Metadata/TypeName.cs | 21 +- .../Reflection/Metadata/TypeNameParser.cs | 17 ++ .../Metadata/TypeNameParserHelpers.cs | 8 +- .../tests/Metadata/TypeNameTests.cs | 241 +++++++++++------- 5 files changed, 178 insertions(+), 111 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs index 61e1996c50e19f..3192c4dde4c55e 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs @@ -10,6 +10,7 @@ namespace System.Reflection.Metadata internal struct TypeNameParseOptions { public TypeNameParseOptions() { } +#pragma warning disable CA1822 // Mark members as static public int MaxNodes { get @@ -19,6 +20,7 @@ public int MaxNodes } } } +#pragma warning restore CA1822 } namespace System.Reflection diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs index 3f1ea8394393c8..16cbfc4faa0512 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs @@ -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) { 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(); + } } return result; diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs index 4fadebb4a3e8d8..2c0567e9a4b547 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs @@ -147,6 +147,23 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse { _inputString = capturedBeforeProcessing; } + else + { + // Every 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; + if (IsMaxDepthExceeded(_parseOptions, recursiveDepth)) + { + return null; + } + } + } int previousDecorator = default; // capture the current state so we can reprocess it again once we know the AssemblyName diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs index f8916b5c016c7e..2e88ea2da03b12 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs @@ -400,17 +400,13 @@ internal static bool IsMaxDepthExceeded(TypeNameParseOptions options, int depth) // CoreLib does not enforce any limits => false; #else - => depth >= options.MaxNodes; + => depth > options.MaxNodes; #endif internal static bool TryDive(TypeNameParseOptions options, ref int depth) { - if (IsMaxDepthExceeded(options, depth)) - { - return false; - } depth++; - return true; + return !IsMaxDepthExceeded(options, depth); } } } diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs index bf1221b1b2082e..8ee51168c584d9 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs @@ -123,158 +123,207 @@ static void Verify(Type type, AssemblyName expectedAssemblyName, TypeName parsed } } - [Theory] - [InlineData(10, "*")] // pointer to pointer - [InlineData(10, "[]")] // array of arrays - [InlineData(100, "*")] - [InlineData(100, "[]")] - public void MaxNodesIsRespected_TooManyDecorators(int maxDepth, string decorator) + private static void EnsureMaxNodesIsRespected(string typeName, int expectedNodeCount, Action validate) { - TypeNameParseOptions options = new() + // Specified MaxNodes is equal the actual node count + TypeNameParseOptions equal = new() { - MaxNodes = maxDepth + MaxNodes = expectedNodeCount }; - string notTooMany = $"System.Int32{string.Join("", Enumerable.Repeat(decorator, maxDepth - 1))}"; - string tooMany = $"System.Int32{string.Join("", Enumerable.Repeat(decorator, maxDepth))}"; + TypeName parsed = TypeName.Parse(typeName.AsSpan(), equal); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); + validate(parsed); + Assert.True(TypeName.TryParse(typeName.AsSpan(), out parsed, equal)); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); + validate(parsed); - Assert.Throws(() => TypeName.Parse(tooMany.AsSpan(), options)); - Assert.False(TypeName.TryParse(tooMany.AsSpan(), out _, options)); + // Specified MaxNodes is less than the actual node count + TypeNameParseOptions less = new() + { + MaxNodes = expectedNodeCount - 1 + }; - TypeName parsed = TypeName.Parse(notTooMany.AsSpan(), options); - ValidateElementType(maxDepth, parsed, decorator); + Assert.Throws(() => TypeName.Parse(typeName.AsSpan(), less)); + Assert.False(TypeName.TryParse(typeName.AsSpan(), out _, less)); - Assert.True(TypeName.TryParse(notTooMany.AsSpan(), out parsed, options)); - ValidateElementType(maxDepth, parsed, decorator); + // Specified MaxNodes is more than the actual node count + TypeNameParseOptions more = new() + { + MaxNodes = expectedNodeCount + 1 + }; + + parsed = TypeName.Parse(typeName.AsSpan(), more); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); + validate(parsed); + Assert.True(TypeName.TryParse(typeName.AsSpan(), out parsed, more)); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); + validate(parsed); + } - static void ValidateElementType(int maxDepth, TypeName parsed, string decorator) + [Theory] + [InlineData("*", 10)] // pointer to pointer + [InlineData("[]", 10)] // array of arrays + [InlineData("*", 100)] + [InlineData("[]", 100)] + public void MaxNodesIsRespected_Decorators(string decorator, int decoratorsCount) + { + string name = $"System.Int32{string.Join("", Enumerable.Repeat(decorator, decoratorsCount))}"; + + // Expected node count: + // - +1 for the simple type name (System.Int32) + // - +1 for every decorator + int expectedNodeCount = 1 + decoratorsCount; + + EnsureMaxNodesIsRespected(name, expectedNodeCount, parsed => { - for (int i = 0; i < maxDepth - 1; i++) + for (int i = 0; i < decoratorsCount; i++) { + Assert.Equal(expectedNodeCount - i, parsed.GetNodeCount()); + Assert.Equal(decorator == "*", parsed.IsPointer); Assert.Equal(decorator == "[]", parsed.IsSZArray); Assert.False(parsed.IsConstructedGenericType); + Assert.False(parsed.IsSimple); parsed = parsed.GetElementType(); } - } + }); } [Theory] [InlineData(10)] [InlineData(100)] - public void MaxNodesIsRespected_TooDeepGenerics(int maxDepth) + public void MaxNodesIsRespected_GenericsOfGenerics(int depth) { - TypeNameParseOptions options = new() + // MakeGenericType is not used here, as it crashes for larger depths + string coreLibName = typeof(object).Assembly.FullName; + string name = typeof(int).AssemblyQualifiedName!; + for (int i = 0; i < depth; i++) { - MaxNodes = maxDepth - }; - - string tooDeep = GetName(maxDepth); - string notTooDeep = GetName(maxDepth - 1); - - Assert.Throws(() => TypeName.Parse(tooDeep.AsSpan(), options)); - Assert.False(TypeName.TryParse(tooDeep.AsSpan(), out _, options)); - - TypeName parsed = TypeName.Parse(notTooDeep.AsSpan(), options); - Validate(maxDepth, parsed); - - Assert.True(TypeName.TryParse(notTooDeep.AsSpan(), out parsed, options)); - Validate(maxDepth, parsed); - - static string GetName(int depth) - { - // MakeGenericType is not used here, as it crashes for larger depths - string coreLibName = typeof(object).Assembly.FullName; - string fullName = typeof(int).AssemblyQualifiedName!; - for (int i = 0; i < depth; i++) - { - fullName = $"System.Collections.Generic.List`1[[{fullName}]], {coreLibName}"; - } - return fullName; + name = $"System.Collections.Generic.List`1[[{name}]], {coreLibName}"; } - static void Validate(int maxDepth, TypeName parsed) + // Expected node count: + // - +1 for the simple type name (System.Int32) + // - +2 * depth (+1 for the generic type definition and +1 for the constructed generic type) + int expectedNodeCount = 1 + 2 * depth; + + EnsureMaxNodesIsRespected(name, expectedNodeCount, parsed => { - for (int i = 0; i < maxDepth - 1; i++) + for (int i = 0; i < depth - 1; i++) { Assert.True(parsed.IsConstructedGenericType); parsed = parsed.GetGenericArguments()[0]; } - } + }); } [Theory] [InlineData(10)] [InlineData(100)] - public void MaxNodesIsRespected_TooManyGenericArguments(int maxDepth) + public void MaxNodesIsRespected_FlatGenericArguments(int genericArgsCount) { - TypeNameParseOptions options = new() - { - MaxNodes = maxDepth - }; - - string tooMany = GetName(maxDepth); - string notTooMany = GetName(maxDepth - 1); - - Assert.Throws(() => TypeName.Parse(tooMany.AsSpan(), options)); - Assert.False(TypeName.TryParse(tooMany.AsSpan(), out _, options)); + string name = $"Some.GenericType`{genericArgsCount}[{string.Join(",", Enumerable.Repeat("System.Int32", genericArgsCount))}]"; - TypeName parsed = TypeName.Parse(notTooMany.AsSpan(), options); - Validate(parsed, maxDepth); + // Expected node count: + // - +1 for the constructed generic type itself + // - +1 * genericArgsCount (all arguments are simple) + // - +1 for generic type definition + int expectedNodeCount = 1 + genericArgsCount + 1; - Assert.True(TypeName.TryParse(notTooMany.AsSpan(), out parsed, options)); - Validate(parsed, maxDepth); - - static string GetName(int depth) - => $"Some.GenericType`{depth}[{string.Join(",", Enumerable.Repeat("System.Int32", depth))}]"; - - static void Validate(TypeName parsed, int maxDepth) + EnsureMaxNodesIsRespected(name, expectedNodeCount, parsed => { Assert.True(parsed.IsConstructedGenericType); ImmutableArray genericArgs = parsed.GetGenericArguments(); - Assert.Equal(maxDepth - 1, genericArgs.Length); + Assert.Equal(genericArgsCount, genericArgs.Length); Assert.All(genericArgs, arg => Assert.False(arg.IsConstructedGenericType)); - } + Assert.All(genericArgs, arg => Assert.Equal(1, arg.GetNodeCount())); + Assert.Equal(1, parsed.GetGenericTypeDefinition().GetNodeCount()); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); + }); } [Theory] [InlineData(10)] [InlineData(100)] - public void MaxNodesIsRespected_TooManyNestedNames(int maxDepth) + public void MaxNodesIsRespected_NestedNames(int nestingDepth) { - TypeNameParseOptions options = new() - { - MaxNodes = maxDepth - }; - - string tooMany = GetName(maxDepth); - string notTooMany = GetName(maxDepth - 1); - - Assert.Throws(() => TypeName.Parse(tooMany.AsSpan(), options)); - Assert.False(TypeName.TryParse(tooMany.AsSpan(), out _, options)); - - TypeName parsed = TypeName.Parse(notTooMany.AsSpan(), options); - Validate(parsed, maxDepth - 1); - - Assert.True(TypeName.TryParse(notTooMany.AsSpan(), out parsed, options)); - Validate(parsed, maxDepth - 1); + string name = $"Namespace.NotNestedType+{string.Join("+", Enumerable.Repeat("NestedType", nestingDepth))}"; - static string GetName(int depth) - => $"Namespace.NotNestedType+{string.Join("+", Enumerable.Repeat("NestedType", depth))}"; + // Expected node count: + // - +1 for the nested type name itself + // - +1 * nestingDepth + int expectedNodeCount = 1 + nestingDepth; - static void Validate(TypeName parsed, int maxDepth) + EnsureMaxNodesIsRespected(name, expectedNodeCount, parsed => { Assert.True(parsed.IsNested); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); + int depth = nestingDepth; while (parsed.IsNested) { - maxDepth--; parsed = parsed.DeclaringType; + Assert.Equal(depth, parsed.GetNodeCount()); + depth--; } - Assert.Equal(0, maxDepth); - } + Assert.False(parsed.IsNested); + Assert.Equal(1, parsed.GetNodeCount()); + Assert.Equal(0, depth); + }); + } + + [Theory] + [InlineData("System.Int32*", 2)] // a pointer to a simple type + [InlineData("System.Int32[]*", 3)] // a pointer to an array of a simple type + [InlineData("System.Declaring+Nested", 2)] // a nested and declaring types + [InlineData("System.Declaring+Nested*", 3)] // a pointer to a nested type, which has the declaring type + // "Namespace.Declaring+NestedGeneric`2[A, B]" requires 6 TypeName instances: + // - constructed generic type: Namespace.Declaring+NestedGeneric`2[A, B] (+1) + // - declaring type: Namespace.Declaring (+1) + // - generic type definition: Namespace.Declaring+NestedGeneric`2 (+1) + // - declaring type: Namespace.Declaring (+1) + // - generic arguments: + // - generic arguments: + // - simple: A (+1) + // - simple: B (+1) + [InlineData("Namespace.Declaring+NestedGeneric`2[A, B]", 6)] + // A pointer to the above + [InlineData("Namespace.Declaring+NestedGeneric`2[A, B]*", 7)] + // A pointer to the array of the above + [InlineData("Namespace.Declaring+NestedGeneric`2[A, B][,]*", 8)] + // "Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]]" requires 7 TypeName instances: + // - constructed generic type: Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]] (+1) + // - declaring type: Namespace.Declaring (+1) + // - generic type definition: Namespace.Declaring+NestedGeneric`1 (+1) + // - declaring type: Namespace.Declaring (+1) + // - generic arguments: + // - constructed generic type: AnotherGeneric`1[A] (+1) + // - generic type definition: AnotherGeneric`1 (+1) + // - generic arguments: + // - simple: A (+1) + [InlineData("Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]]", 7)] + public void MaxNodesIsRespected(string typeName, int expectedNodeCount) + { + TypeNameParseOptions tooMany = new() + { + MaxNodes = expectedNodeCount - 1 + }; + TypeNameParseOptions notTooMany = new() + { + MaxNodes = expectedNodeCount + }; + + Assert.Throws(() => TypeName.Parse(typeName.AsSpan(), tooMany)); + Assert.False(TypeName.TryParse(typeName.AsSpan(), out _, tooMany)); + + TypeName parsed = TypeName.Parse(typeName.AsSpan(), notTooMany); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); + + Assert.True(TypeName.TryParse(typeName.AsSpan(), out parsed, notTooMany)); + Assert.Equal(expectedNodeCount, parsed.GetNodeCount()); } public static IEnumerable GenericArgumentsAreSupported_Arguments() @@ -512,10 +561,10 @@ public static IEnumerable GetAdditionalConstructedTypeData() [InlineData(typeof(int[,][]), 3)] [InlineData(typeof(Nullable<>), 1)] // open generic type treated as elemental [InlineData(typeof(NestedNonGeneric_0), 2)] // declaring and nested - [InlineData(typeof(NestedGeneric_0), 3)] // declaring, nested and generic arg + [InlineData(typeof(NestedGeneric_0), 5)] // declaring, nested, generic arg and generic type definition and its declaring type [InlineData(typeof(NestedNonGeneric_0.NestedNonGeneric_1), 3)] // declaring, nested 0 and nested 1 // TypeNameTests+NestedGeneric_0`1+NestedGeneric_1`2[[Int32],[String],[Boolean]] (simplified for brevity) - [InlineData(typeof(NestedGeneric_0.NestedGeneric_1), 6)] // declaring, nested 0 and nested 1 and 3 generic args + [InlineData(typeof(NestedGeneric_0.NestedGeneric_1), 9)] // declaring, nested 0 and nested 1 and 3 generic args and generic type definition and its declaring types [MemberData(nameof(GetAdditionalConstructedTypeData))] public void GetNodeCountReturnsExpectedValue(Type type, int expected) { From 84c7fbca5d614da92e96beb37445698da2721e30 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Tue, 13 Aug 2024 16:55:29 +0200 Subject: [PATCH 4/8] minor polishing after reading the code again --- .../src/System/Reflection/TypeNameResolver.cs | 2 +- .../src/System/Reflection/Metadata/TypeNameParser.cs | 2 +- .../src/System/Reflection/Metadata/TypeNameParserHelpers.cs | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs b/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs index 3192c4dde4c55e..04ab8e7ee08598 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.cs @@ -19,8 +19,8 @@ public int MaxNodes return 0; } } - } #pragma warning restore CA1822 + } } namespace System.Reflection diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs index 2c0567e9a4b547..af0c708b342980 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs @@ -149,7 +149,7 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse } else { - // Every generic type needs the generic type definition. + // Every constructed generic type needs the generic type definition. if (!TryDive(_parseOptions, ref recursiveDepth)) { return null; diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs index 2e88ea2da03b12..6b12a0bd212c30 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs @@ -397,8 +397,7 @@ internal static void ThrowInvalidOperation_HasToBeArrayClass() internal static bool IsMaxDepthExceeded(TypeNameParseOptions options, int depth) #if SYSTEM_PRIVATE_CORELIB - // CoreLib does not enforce any limits - => false; + => false; // CoreLib does not enforce any limits #else => depth > options.MaxNodes; #endif From 3f933562ba77638439885f39f38bd04449520584 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Wed, 14 Aug 2024 10:38:47 +0200 Subject: [PATCH 5/8] address code review feedback: - revert the change that seemed like optimization to make the code more readable - add a comment explaining why we don't need to check of integer overflows - add a comment explaining the details of generic nested types node count --- .../src/System/Reflection/Metadata/TypeName.cs | 8 ++++---- .../src/System/Reflection/Metadata/TypeNameParser.cs | 9 ++++++++- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs index 16cbfc4faa0512..8aa5413fc5aa24 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs @@ -282,13 +282,13 @@ public string Name /// public int GetNodeCount() { + // This method does not check for integer overflows because it's impossible to parse + // a TypeName with NodeCount > int.MaxValue (TypeNameParseOptions.MaxNodes is an int). int result = 1; - TypeName? declaring = _declaringType; - while (declaring is not null) + if (IsNested) { - result++; - declaring = declaring._declaringType; + result += DeclaringType.GetNodeCount(); } if (IsArray || IsPointer || IsByRef) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs index af0c708b342980..bf9b2ac895dd2c 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs @@ -154,7 +154,14 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse { return null; } - // If that definition represents a nested type, this needs to be taken into account. + // If that generic type is a nested type, this needs to be taken into account. Example: + // "Namespace.Declaring+NestedGeneric`1[GenericArg]" requires 5 TypeName instances: + // - constructed generic type: Namespace.Declaring+NestedGeneric`2[GenericArg] (+1, handled by ParseNextTypeName) + // - declaring type: Namespace.Declaring (+1, handled by TryGetTypeNameInfo) + // - generic type definition: Namespace.Declaring+NestedGeneric`1 (+1, handled by the TryDive above) + // - declaring type: Namespace.Declaring (+1, handled by the if below) + // - generic arguments: + // - simple: GenericArg (+1, handled by ParseNextTypeName) if (nestedNameLengths is not null) { recursiveDepth += nestedNameLengths.Count; From b90de5b5a14b3114ec97d266f90d825317852666 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Fri, 30 Aug 2024 11:55:23 +0200 Subject: [PATCH 6/8] address code review feedback: - handle overflows - don't include the declaring type of generic type definition, as it's the same instance uses by the constructed generic type and add sth extra: assert for ensuring that GetNodeCount returns the same result as calculated by the parser during parsing --- .../System/Reflection/Metadata/TypeName.cs | 14 +++++++------ .../Reflection/Metadata/TypeNameParser.cs | 20 ++++--------------- .../tests/Metadata/TypeNameTests.cs | 20 +++++++++---------- 3 files changed, 22 insertions(+), 32 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs index 4d74dd9a560e60..90cdd33d6ffdbe 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs @@ -301,26 +301,28 @@ public string Name /// public int GetNodeCount() { - // This method does not check for integer overflows because it's impossible to parse - // a TypeName with NodeCount > int.MaxValue (TypeNameParseOptions.MaxNodes is an int). + // This method uses checked arithmetic to avoid silent overflows. + // 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) { - result += DeclaringType.GetNodeCount(); + result = checked(result + DeclaringType.GetNodeCount()); } if (IsArray || IsPointer || IsByRef) { - result += GetElementType().GetNodeCount(); + result = checked(result + GetElementType().GetNodeCount()); } else if (IsConstructedGenericType) { - result += GetGenericTypeDefinition().GetNodeCount(); + checked { result++; } // the generic type definition foreach (TypeName genericArgument in GetGenericArguments()) { - result += genericArgument.GetNodeCount(); + result = checked(result + genericArgument.GetNodeCount()); } } diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs index bf9b2ac895dd2c..dcb4f42b7433bf 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs @@ -61,6 +61,8 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse return null; } + Debug.Assert(parsedName.GetNodeCount() == recursiveDepth, $"Node count mismatch for '{typeName.ToString()}'"); + return parsedName; } @@ -154,22 +156,8 @@ private TypeNameParser(ReadOnlySpan name, bool throwOnError, TypeNameParse { return null; } - // If that generic type is a nested type, this needs to be taken into account. Example: - // "Namespace.Declaring+NestedGeneric`1[GenericArg]" requires 5 TypeName instances: - // - constructed generic type: Namespace.Declaring+NestedGeneric`2[GenericArg] (+1, handled by ParseNextTypeName) - // - declaring type: Namespace.Declaring (+1, handled by TryGetTypeNameInfo) - // - generic type definition: Namespace.Declaring+NestedGeneric`1 (+1, handled by the TryDive above) - // - declaring type: Namespace.Declaring (+1, handled by the if below) - // - generic arguments: - // - simple: GenericArg (+1, handled by ParseNextTypeName) - if (nestedNameLengths is not null) - { - recursiveDepth += nestedNameLengths.Count; - if (IsMaxDepthExceeded(_parseOptions, 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; diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs index 06f2fb025eee74..f5c760f0d282ac 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs @@ -278,31 +278,31 @@ public void MaxNodesIsRespected_NestedNames(int nestingDepth) [InlineData("System.Int32[]*", 3)] // a pointer to an array of a simple type [InlineData("System.Declaring+Nested", 2)] // a nested and declaring types [InlineData("System.Declaring+Nested*", 3)] // a pointer to a nested type, which has the declaring type - // "Namespace.Declaring+NestedGeneric`2[A, B]" requires 6 TypeName instances: + // "Namespace.Declaring+NestedGeneric`2[A, B]" requires 5 TypeName instances: // - constructed generic type: Namespace.Declaring+NestedGeneric`2[A, B] (+1) // - declaring type: Namespace.Declaring (+1) // - generic type definition: Namespace.Declaring+NestedGeneric`2 (+1) - // - declaring type: Namespace.Declaring (+1) + // - declaring type is the same as of the constructed generic type (+0) // - generic arguments: // - generic arguments: // - simple: A (+1) // - simple: B (+1) - [InlineData("Namespace.Declaring+NestedGeneric`2[A, B]", 6)] + [InlineData("Namespace.Declaring+NestedGeneric`2[A, B]", 5)] // A pointer to the above - [InlineData("Namespace.Declaring+NestedGeneric`2[A, B]*", 7)] + [InlineData("Namespace.Declaring+NestedGeneric`2[A, B]*", 6)] // A pointer to the array of the above - [InlineData("Namespace.Declaring+NestedGeneric`2[A, B][,]*", 8)] - // "Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]]" requires 7 TypeName instances: + [InlineData("Namespace.Declaring+NestedGeneric`2[A, B][,]*", 7)] + // "Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]]" requires 6 TypeName instances: // - constructed generic type: Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]] (+1) // - declaring type: Namespace.Declaring (+1) // - generic type definition: Namespace.Declaring+NestedGeneric`1 (+1) - // - declaring type: Namespace.Declaring (+1) + // - declaring type is the same as of the constructed generic type (+0) // - generic arguments: // - constructed generic type: AnotherGeneric`1[A] (+1) // - generic type definition: AnotherGeneric`1 (+1) // - generic arguments: // - simple: A (+1) - [InlineData("Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]]", 7)] + [InlineData("Namespace.Declaring+NestedGeneric`1[AnotherGeneric`1[A]]", 6)] public void MaxNodesIsRespected(string typeName, int expectedNodeCount) { TypeNameParseOptions tooMany = new() @@ -559,10 +559,10 @@ public static IEnumerable GetAdditionalConstructedTypeData() [InlineData(typeof(int[,][]), 3)] [InlineData(typeof(Nullable<>), 1)] // open generic type treated as elemental [InlineData(typeof(NestedNonGeneric_0), 2)] // declaring and nested - [InlineData(typeof(NestedGeneric_0), 5)] // declaring, nested, generic arg and generic type definition and its declaring type + [InlineData(typeof(NestedGeneric_0), 4)] // declaring, nested, generic arg and generic type definition [InlineData(typeof(NestedNonGeneric_0.NestedNonGeneric_1), 3)] // declaring, nested 0 and nested 1 // TypeNameTests+NestedGeneric_0`1+NestedGeneric_1`2[[Int32],[String],[Boolean]] (simplified for brevity) - [InlineData(typeof(NestedGeneric_0.NestedGeneric_1), 9)] // declaring, nested 0 and nested 1 and 3 generic args and generic type definition and its declaring types + [InlineData(typeof(NestedGeneric_0.NestedGeneric_1), 7)] // declaring, nested 0 and nested 1 and 3 generic args and generic type definition [MemberData(nameof(GetAdditionalConstructedTypeData))] public void GetNodeCountReturnsExpectedValue(Type type, int expected) { From 593b00d5fabccdf6acf92aeeab10f9c7474a9e04 Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 2 Sep 2024 15:34:26 +0200 Subject: [PATCH 7/8] address code review feedback: - reorder the conditions - add test for int32 overflow --- .../System/Reflection/Metadata/TypeName.cs | 11 +++++----- .../tests/Metadata/TypeNameTests.cs | 20 +++++++++++++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs index 90cdd33d6ffdbe..22ae86f08e6b53 100644 --- a/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs +++ b/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs @@ -307,24 +307,23 @@ public int GetNodeCount() // to create such names with the Make* APIs. int result = 1; - if (IsNested) - { - result = checked(result + DeclaringType.GetNodeCount()); - } - if (IsArray || IsPointer || IsByRef) { result = checked(result + GetElementType().GetNodeCount()); } else if (IsConstructedGenericType) { - checked { result++; } // the generic type definition + result = checked(result + GetGenericTypeDefinition().GetNodeCount()); foreach (TypeName genericArgument in GetGenericArguments()) { result = checked(result + genericArgument.GetNodeCount()); } } + else if (IsNested) + { + result = checked(result + DeclaringType.GetNodeCount()); + } return result; } diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs index f5c760f0d282ac..2d599350b6bc2f 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs @@ -581,6 +581,26 @@ public void GetNodeCountReturnsExpectedValue(Type type, int expected) } } + [OuterLoop("It takes a lot of time")] + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] // 32 is likely to OOM + public void GetNodeCountThrowsForInt32Overflow() + { + TypeName genericType = TypeName.Parse("Generic".AsSpan()); + TypeName genericArg = TypeName.Parse("Arg".AsSpan()); + // Don't allocate Array.MaxLength array as it may make this test flaky (frequent OOMs). + ImmutableArray.Builder genericArgs = ImmutableArray.CreateBuilder(initialCapacity: int.MaxValue / 64); + for (int i = 0; i < genericArgs.Capacity; ++i) + { + genericArgs.Add(genericArg); + } + TypeName constructedGenericType = genericType.MakeGenericTypeName(genericArgs.MoveToImmutable()); + // Just repeat the same reference to a large closed generic type multiple times. + // It's going to give us large node count without allocating too much. + TypeName largeNodeCount = genericType.MakeGenericTypeName(Enumerable.Repeat(constructedGenericType, 64 + 1).ToImmutableArray()); + + Assert.Throws(() => largeNodeCount.GetNodeCount()); + } + [Fact] public void MakeGenericTypeName() { From 2a56a1f1b368e9417269fc6e04bfb4196ad0dabd Mon Sep 17 00:00:00 2001 From: Adam Sitnik Date: Mon, 2 Sep 2024 18:24:13 +0200 Subject: [PATCH 8/8] address code review feedback: allocate smaller arrays so it can run on 32 bit --- .../tests/Metadata/TypeNameTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs index 2d599350b6bc2f..047e37b792cbfc 100644 --- a/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs +++ b/src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs @@ -582,13 +582,13 @@ public void GetNodeCountReturnsExpectedValue(Type type, int expected) } [OuterLoop("It takes a lot of time")] - [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] // 32 is likely to OOM + [Fact] public void GetNodeCountThrowsForInt32Overflow() { TypeName genericType = TypeName.Parse("Generic".AsSpan()); TypeName genericArg = TypeName.Parse("Arg".AsSpan()); // Don't allocate Array.MaxLength array as it may make this test flaky (frequent OOMs). - ImmutableArray.Builder genericArgs = ImmutableArray.CreateBuilder(initialCapacity: int.MaxValue / 64); + ImmutableArray.Builder genericArgs = ImmutableArray.CreateBuilder(initialCapacity: (int)Math.Sqrt(int.MaxValue)); for (int i = 0; i < genericArgs.Capacity; ++i) { genericArgs.Add(genericArg); @@ -596,7 +596,7 @@ public void GetNodeCountThrowsForInt32Overflow() TypeName constructedGenericType = genericType.MakeGenericTypeName(genericArgs.MoveToImmutable()); // Just repeat the same reference to a large closed generic type multiple times. // It's going to give us large node count without allocating too much. - TypeName largeNodeCount = genericType.MakeGenericTypeName(Enumerable.Repeat(constructedGenericType, 64 + 1).ToImmutableArray()); + TypeName largeNodeCount = genericType.MakeGenericTypeName(Enumerable.Repeat(constructedGenericType, (int)Math.Sqrt(int.MaxValue)).ToImmutableArray()); Assert.Throws(() => largeNodeCount.GetNodeCount()); }