Avoid infinite loop in recursive type hierarchies #113572
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses an infinite loop in recursive type hierarchies by introducing tests for self-recursive and mutually recursive type definitions and adding a check in the runtime type builder to detect such recursion.
- Added tests in TypeBuilderSetParent.cs for self and base recursive type scenarios.
- Updated RuntimeTypeBuilder.cs to throw an ArgumentException when a recursive hierarchy is detected.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderSetParent.cs | Added tests to validate proper error handling on recursive type hierarchy creation. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs | Modified type equality and subclass checks to catch and throw on recursive type hierarchy situations. |
| @@ -171,7 +172,7 @@ internal static bool IsTypeEqual(Type? t1, Type? t2) | |||
| if (tb1 != null && tb2 != null && ReferenceEquals(tb1, tb2)) | |||
| return true; | |||
|
|
|||
| // if the runtimetype view is eqaul than it is equal | |||
| // if the runtimetype view is equal than it is equal | |||
There was a problem hiding this comment.
[nitpick] Consider updating the comment to 'if the runtime type view is equal, then the types are considered equal' for better clarity and grammatical correctness.
| // if the runtimetype view is equal than it is equal | |
| // If the runtime type view is equal, then the types are considered equal |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| TypeBuilder typeDerived = module.DefineType("DerivedType", TypeAttributes.Class | TypeAttributes.Public); | ||
| typeBase.SetParent(typeDerived); | ||
| typeDerived.SetParent(typeBase); | ||
| Assert.Throws<ArgumentException>(() => typeBase.CreateType()); |
There was a problem hiding this comment.
I assume that the following variant of the repro:
ModuleBuilder module = assembly.DefineDynamicModule("TestModule");
TypeBuilder type = module.DefineType("TestType", TypeAttributes.Class | TypeAttributes.Public);
TypeBuilder type2 = module.DefineType("TestType2", TypeAttributes.Class | TypeAttributes.Public);
type.SetParent(type2);
type2.SetParent(type2);
type.CreateType();
is still broken. Is that correct?
I think it is one of many situations that can introduce infinite loop in Ref.Emit. Are we going to fix all of them as they are found? |
|
If we end up fixing this, Mono and persisted Reflection.Emit should get the same fix. |
| @@ -945,6 +946,10 @@ public override bool IsSubclassOf(Type c) | |||
|
|
|||
| while (p != null) | |||
| { | |||
| // We can't use IsTypeEqual(p, this) because it would be recursive at least for Enums. | |||
There was a problem hiding this comment.
I think it would be better to just return false instead of throwing ArgumentException.
ArgumentException would be fine to throw from SetParent, but that has its own set of problems (it would be a breaking change).
|
Closing; I don't think this is worth pursuing. |
Fixes #112072
We have a somewhat high bar for adding error detection to the builder classes, but this one causes an infinite loop.