[TypeName] Nested types should respect MaxNode count #106334

adamsitnik · 2024-08-13T14:57:55Z

Fixes a bug discovered by @jkotas in #103713 (comment)

…nerics

...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs

jkotas · 2024-08-13T19:53:52Z

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs

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


Should this use checked arithmetic to avoid silent overflows?

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

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.

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs

...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs

- 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

…ount # Conflicts: # src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs

jkotas · 2024-08-15T00:25:55Z

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs

+                    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:


Namespace.Declaring+NestedGeneric[GenericArg] should only require 4 TypeName instances.

I think this comment is not correct. Also, the implementation of GetNodeCount seems to have a bug - it is double-counting nested TypeName instances.

Namespace.Declaring+NestedGeneric[GenericArg] should only require 4 TypeName instances.

Which node from the list below should not be included in your opinion?

- 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)

We define it as the total number of TypeName instances required to represent it:

runtime/src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs

Lines 275 to 276 in 8983904

/// Represents the total number of <see cref="TypeName"/> instances that are used to describe

/// this instance, including any generic arguments or underlying types.

And we have 5 instances.

The declaring type of the generic type definition and of the constructed generic type is the same object:

Do you suggest that because it's the same object, it should not be included?

Also, the implementation of GetNodeCount seems to have a bug - it is double-counting nested TypeName instances.

It does that on purpose: to include the nodes from generic type definition.

Do you suggest that because it's the same object, it should not be included?

Yes. If it is the same object, it is the same instance and it should be only counted once.

We have multiple ways to get to that instance through the object model. I do not think that it should be a reason to count it multiple times.

Yes. If it is the same object, it is the same instance and it should be only counted once

You are right. I've changed the implementation and the tests, PTAL.

jkotas · 2024-08-26T21:19:36Z

I have opened #106991 to make sure that this does not get missed for .NET 9.

adamsitnik · 2024-08-29T15:49:58Z

I have opened #106991 to make sure that this does not get missed for .NET 9.

Thank you for doing that and apologies for the delay in responding.

…ount

- 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

jkotas

LGTM otherwise

jkotas · 2024-08-30T16:55:42Z

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs

@@ -301,24 +301,29 @@ public string Name
        /// </remarks>
        public int GetNodeCount()
        {
+            // This method uses checked arithmetic to avoid silent overflows.


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.

jkotas · 2024-08-30T17:39:12Z

src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeName.cs

            {
-                result += GetElementType().GetNodeCount();
-            }
+                checked { result++; } // the generic type definition


Suggested change

checked { result++; } // the generic type definition

result = checked(result + GetGenericTypeDefinition().GetNodeCount());

I think the logic would be easier to understand if it is written like this, and the if (IsNested) check is moved to else if after the IsConstructedGenericType check.

TypeName's form a tree structure and GetNodeCount should be a simple walk over this tree structure. The code as written makes it hard to see.

- reorder the conditions - add test for int32 overflow

adamsitnik · 2024-09-02T14:22:35Z

/azp list

azure-pipelines · 2024-09-02T14:22:40Z

CI/CD Pipelines for this repository: runtime-coreclr outerloop runtime-coreclr jitstress runtime-coreclr jitstressregs runtime-coreclr jitstress2-jitstressregs runtime-coreclr gcstress0x3-gcstress0xc runtime-coreclr gcstress-extra runtime-coreclr r2r-extra runtime-coreclr jitstress-isas-x86 runtime-coreclr jitstress-isas-arm runtime-coreclr jitstressregs-x86 runtime-coreclr libraries-jitstressregs runtime-coreclr libraries-jitstress2-jitstressregs runtime-coreclr r2r runtime-coreclr runincontext runtime-coreclr crossgen2 runtime-libraries-coreclr outerloop runtime-libraries-coreclr outerloop-windows runtime-libraries-coreclr outerloop-linux runtime-libraries-coreclr outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-dev-innerloop runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly runtime-coreclr crossgen2-composite runtime-jit-experimental runtime-coreclr libraries-jitstress dotnet-linker-tests runtime-coreclr ilasm runtime-coreclr crossgen2-composite gcstress runtime-staging runtime-coreclr pgo runtime-coreclr libraries-pgo Antigen runtime-community Fuzzlyn runtime-coreclr superpmi-replay runtime-wasm runtime-coreclr superpmi-diffs runtime-coreclr superpmi-asmdiffs-checked-release runtime-extra-platforms jit-cfg runtime-wasm-perf runtime-llvm runtime-coreclr jitstress-random runtime-coreclr libraries-jitstress-random runtime-android-grpc-client-tests runtime-wasm-non-libtests runtime-android runtime-androidemulator runtime-ioslike runtime-ioslikesimulator runtime-linuxbionic runtime-maccatalyst runtime-coreclr pgostress runtime-coreclr jitstress-isas-avx512 runtime-libraries-mono outerloop runtime-sanitized runtime-wasm-dbgtests runtime-wasm-optional runtime-nativeaot-outerloop runtime-coreclr jitstress-isas-sve runtime-coreclr superpmi-collect-test

adamsitnik · 2024-09-02T14:23:08Z

/azp run runtime-libraries-coreclr outerloop-linux

azure-pipelines · 2024-09-02T14:23:17Z

Azure Pipelines successfully started running 1 pipeline(s).

jkotas · 2024-09-02T14:58:59Z

src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs

+            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<TypeName>.Builder genericArgs = ImmutableArray.CreateBuilder<TypeName>(initialCapacity: int.MaxValue / 64);


This does not need to allocate very large array. Instead, it can repeat MakeGenericTypeName multiple times. It will make the test a lot less resource intensive, faster and compatible with 32-bit platforms.

This does not need to allocate very large array. Instead, it can repeat MakeGenericTypeName multiple times.

I know and in my opinion this is exactly what it's doing right now. But I assume that int.MaxValue / 64 is still very large array for you. What would not be? int.MaxValue / 1024? Or Math.Sqrt(int.MaxValue)?

It can be very small, like 10.

Math.Sqrt(int.MaxValue) does what we need, I've pushed the change

jkotas · 2024-09-02T14:59:26Z

src/libraries/System.Reflection.Metadata/tests/Metadata/TypeNameTests.cs

@@ -493,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


Suggested change

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] // 32 is likely to OOM

…n 32 bit

…ount

adamsitnik · 2024-09-02T17:33:58Z

@MihuBot fuzz TypeName

jkotas

LGTM. Thank you!

adamsitnik · 2024-09-03T07:26:58Z

/ba-g unrelated

jkotas · 2024-09-03T20:53:16Z

@adamsitnik Are you going to port this to release/9.0?

adamsitnik · 2024-09-04T10:13:32Z

@adamsitnik Are you going to port this to release/9.0?

Yes, I am going to send multiple backport PRs (with the fuzzers, the bug fixes they discovered and this PR)

adamsitnik · 2024-09-09T09:01:02Z

@adamsitnik Are you going to port this to release/9.0?

Follow up: #107533

* AssemblyNameInfo fuzzer (#107195) * add initial AssemblyNameInfo Fuzzer * fix the first bug that it has discovered * Fix sbyte overflow in TypeName parsing (#107261) * Add TypeNameFuzzer (#107206) Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com> Co-authored-by: Adam Sitnik <adam.sitnik@gmail.com> * [TypeName] Nested types should respect MaxNode count (#106334) * Improve AssemblyNameInfo Fuzzer (#107257) --------- Co-authored-by: Buyaa Namnan <bunamnan@microsoft.com> Co-authored-by: Miha Zupan <mihazupan.zupan1@gmail.com>

adamsitnik added 4 commits August 13, 2024 13:07

add a failing unit test

6cdd040

the fix

da4df69

handle 100% of the cases: nested generic types, generics of nested ge…

f3183e0

…nerics

minor polishing after reading the code again

84c7fbc

adamsitnik added the area-System.Reflection.Metadata label Aug 13, 2024

adamsitnik added this to the 9.0.0 milestone Aug 13, 2024

adamsitnik requested a review from jkotas August 13, 2024 14:57

adamsitnik self-assigned this Aug 13, 2024

adamsitnik commented Aug 13, 2024

View reviewed changes

...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs Show resolved Hide resolved

adamsitnik commented Aug 13, 2024

View reviewed changes

...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs Show resolved Hide resolved

This was referenced Aug 13, 2024

System.IO.Net5Compat.Tests and System.IO.Tests suddenly exiting with error 137 #100558

Open

SIGKILL (OOM?) while running LibraryImportGenerator.Tests w/o actionable log messages or artifacts dotnet/dnceng#2496

Open

jkotas reviewed Aug 13, 2024

View reviewed changes

adamsitnik added 2 commits August 14, 2024 10:38

address code review feedback:

3f93356

- 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

Merge remote-tracking branch 'upstream/main' into nestedTypeNameNodeC…

43082d5

…ount # Conflicts: # src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs

build-analysis bot mentioned this pull request Aug 14, 2024

System.Threading.ThreadPools.Tests.ThreadPoolTests.IOCompletionPortCountConfigVarTest failure - time out, unexpected exit code #106206

Closed

jkotas reviewed Aug 15, 2024

View reviewed changes

jkotas added the binaryformatter-migration Issues related to the removal of BinaryFormatter and migrations away from it label Aug 26, 2024

jkotas mentioned this pull request Aug 26, 2024

Nested types do not respect TypeNameParseOptions.MaxNodes #106991

Closed

terrajobst linked an issue Aug 27, 2024 that may be closed by this pull request

Nested types do not respect TypeNameParseOptions.MaxNodes #106991

Closed

adamsitnik added 2 commits August 30, 2024 11:35

Merge remote-tracking branch 'upstream/main' into nestedTypeNameNodeC…

815d8af

…ount

adamsitnik requested a review from jkotas August 30, 2024 10:39

jkotas reviewed Aug 30, 2024

View reviewed changes

address code review feedback:

593b00d

- reorder the conditions - add test for int32 overflow

adamsitnik requested a review from jkotas September 2, 2024 14:22

jkotas reviewed Sep 2, 2024

View reviewed changes

build-analysis bot mentioned this pull request Sep 2, 2024

restarted. Azure DevOps can't recover from restarts. dotnet/dnceng#3879

Open

3 tasks

address code review feedback: allocate smaller arrays so it can run o…

2a56a1f

…n 32 bit

adamsitnik requested a review from jkotas September 2, 2024 16:25

build-analysis bot mentioned this pull request Sep 2, 2024

Test failure WindowsAlternateDataStreamOverwrite #83659

Open

adamsitnik mentioned this pull request Sep 2, 2024

Add TypeNameFuzzer #107206

Merged

Merge remote-tracking branch 'upstream/main' into nestedTypeNameNodeC…

09e2901

…ount

MihuBot mentioned this pull request Sep 2, 2024

[Fuzzing] [adamsitnik] [TypeName] Nested types should respect MaxNode count MihuBot/runtime-utils#611

Open

jkotas reviewed Sep 2, 2024

View reviewed changes

jkotas approved these changes Sep 2, 2024

View reviewed changes

build-analysis bot mentioned this pull request Sep 2, 2024

System.Numerics.Tensors.Tests.TensorSpanTests.TensorExtensionsTwoSpanInFloatOut fails with IndexOutOfRangeException #107254

Open

adamsitnik merged commit 16fe4d4 into dotnet:main Sep 3, 2024
142 of 148 checks passed

adamsitnik added a commit to adamsitnik/runtime that referenced this pull request Sep 9, 2024

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

8f9f431

adamsitnik mentioned this pull request Sep 9, 2024

[release/9.0] TypeName fuzzer and bug fixes #107533

Merged

4 tasks

jtschuster pushed a commit to jtschuster/runtime that referenced this pull request Sep 17, 2024

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

166bc8b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TypeName] Nested types should respect MaxNode count #106334

[TypeName] Nested types should respect MaxNode count #106334

adamsitnik commented Aug 13, 2024 •

edited

Loading

jkotas Aug 13, 2024

adamsitnik Aug 14, 2024

jkotas Aug 14, 2024

jkotas Aug 15, 2024 •

edited

Loading

adamsitnik Aug 29, 2024

jkotas Aug 29, 2024 •

edited

Loading

adamsitnik Aug 30, 2024

jkotas commented Aug 26, 2024

adamsitnik commented Aug 29, 2024

jkotas left a comment

jkotas Aug 30, 2024

jkotas Aug 30, 2024 •

edited

Loading

adamsitnik commented Sep 2, 2024

azure-pipelines bot commented Sep 2, 2024

adamsitnik commented Sep 2, 2024

azure-pipelines bot commented Sep 2, 2024

jkotas Sep 2, 2024

adamsitnik Sep 2, 2024 •

edited

Loading

jkotas Sep 2, 2024

adamsitnik Sep 2, 2024

jkotas Sep 2, 2024

adamsitnik commented Sep 2, 2024

jkotas left a comment

adamsitnik commented Sep 3, 2024

jkotas commented Sep 3, 2024

adamsitnik commented Sep 4, 2024

adamsitnik commented Sep 9, 2024

	/// Represents the total number of <see cref="TypeName"/> instances that are used to describe
	/// this instance, including any generic arguments or underlying types.

	checked { result++; } // the generic type definition
	result = checked(result + GetGenericTypeDefinition().GetNodeCount());

[TypeName] Nested types should respect MaxNode count #106334

[TypeName] Nested types should respect MaxNode count #106334

Conversation

adamsitnik commented Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Aug 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Aug 26, 2024

adamsitnik commented Aug 29, 2024

jkotas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Aug 30, 2024 • edited Loading

Choose a reason for hiding this comment

adamsitnik commented Sep 2, 2024

azure-pipelines bot commented Sep 2, 2024

adamsitnik commented Sep 2, 2024

azure-pipelines bot commented Sep 2, 2024

Choose a reason for hiding this comment

adamsitnik Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsitnik commented Sep 2, 2024

jkotas left a comment

Choose a reason for hiding this comment

adamsitnik commented Sep 3, 2024

jkotas commented Sep 3, 2024

adamsitnik commented Sep 4, 2024

adamsitnik commented Sep 9, 2024

adamsitnik commented Aug 13, 2024 •

edited

Loading

jkotas Aug 15, 2024 •

edited

Loading

jkotas Aug 29, 2024 •

edited

Loading

jkotas Aug 30, 2024 •

edited

Loading

adamsitnik Sep 2, 2024 •

edited

Loading