[TypeName] Nested types should respect MaxNode count #106334
Conversation
...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs
Show resolved
Hide resolved
...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs
Show resolved
Hide resolved
|
|
||
| foreach (TypeName genericArgument in GetGenericArguments()) | ||
| { | ||
| result += genericArgument.GetNodeCount(); |
There was a problem hiding this comment.
Should this use checked arithmetic to avoid silent overflows?
There was a problem hiding this comment.
No, because we can not parse more than Int32.MaxValue nodes because TypeNameParseOptions.MaxNodes is also an Int32.
There was a problem hiding this comment.
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
Outdated
Show resolved
Hide resolved
...libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParserHelpers.cs
Show resolved
Hide resolved
src/libraries/System.Reflection.Metadata/src/System/Reflection/Metadata/TypeNameParser.cs
Outdated
Show resolved
Hide resolved
- 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
| 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
added
the
binaryformatter-migration
|
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. |
- 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
| @@ -301,24 +301,29 @@ public string Name | |||
| /// </remarks> | |||
| public int GetNodeCount() | |||
| { | |||
| // This method uses checked arithmetic to avoid silent overflows. | |||
There was a problem hiding this comment.
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.
| { | ||
| result += GetElementType().GetNodeCount(); | ||
| } | ||
| checked { result++; } // the generic type definition |
There was a problem hiding this comment.
| 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
|
/azp list |
|
/azp run runtime-libraries-coreclr outerloop-linux |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
Math.Sqrt(int.MaxValue) does what we need, I've pushed the change
| @@ -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 | |||
There was a problem hiding this comment.
| [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.Is64BitProcess))] // 32 is likely to OOM |
|
@MihuBot fuzz TypeName |
|
/ba-g unrelated |
|
@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) |
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>
Fixes a bug discovered by @jkotas in #103713 (comment)
fixes #106991