[TypeName] Nested types should respect MaxNode count (dotnet#106334)

tomeksowi · Sep 3, 2024 · 16fe4d4 · 16fe4d4
1 parent fa0b3f0
commit 16fe4d4
Show file tree

Hide file tree

Showing 7 changed files with 233 additions and 102 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
@@ -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

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
@@ -301,24 +301,28 @@ public string Name
         /// </remarks>
         public int GetNodeCount()
         {
+            // 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)
+            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;

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
@@ -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);
                     }
@@ -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;
             }
@@ -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;
                 }
@@ -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;
-        }
     }
 }
diff --git a/...raries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs b/...raries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs
@@ -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;
@@ -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
                 }
@@ -389,6 +395,19 @@ internal static void ThrowInvalidOperation_HasToBeArrayClass()
 #endif
         }
 
+        internal static bool IsMaxDepthExceeded(TypeNameParseOptions options, int depth)
+#if SYSTEM_PRIVATE_CORELIB
+            => false; // CoreLib does not enforce any limits
+#else
+            => depth > options.MaxNodes;
+#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)

diff --git a/...raries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserOptions.cs b/...raries/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
@@ -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);
             }
         }