Repository navigation
Handle missing signatures for substituted type custom modifiers - #136847
NinoFloris wants to merge 7 commits into
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
dotnet-policy-service
Bot
added
the
community-contribution
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-reflection |
|
Two more native helpers could do with bounds checks tbh:
|
|
This was also unhandled on NativeAOT, also fixed. |
There was a problem hiding this comment.
🟢 Approval recommended
The review reports no active issues with the nested-type custom-modifier changes.
No open issues 🎉
What changed in this PR
Fixes custom-modifier queries on nested types of substituted generic parameters, addressing the invalid signature-buffer read reported in #136848 across CoreCLR and NativeAOT.
Changes:
- Returns empty custom-modifier arrays when a substituted type has no nested signature.
- Adds native bounds validation before reading a signature offset.
- Extends reflection tests for generic class and method substitutions, reusing recursive checks for synthetic array parameters.
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Reflection.Tests/ParameterInfoTests.cs | Adds substitution cases and shares recursive modified-type assertions. |
| src/coreclr/vm/runtimehandles.cpp | Rejects out-of-range offsets before accessing signature data. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/ModifiedType.CoreCLR.cs | Skips modifier lookup for negative signature offsets. |
| src/coreclr/nativeaot/System.Private.CoreLib/src/System/Reflection/ModifiedType.NativeAot.cs | Handles nested substituted types without a signature. |
Review effort: Balanced
|
Does System.Reflection.MetadataLoadContext handle this correctly? |
My clanker states it's ok because MetadataLoadContext never computes offsets, it decodes directly as it walks. |
Could you please add a matching test for MetadataLoadContext to validate it? |
|
How does this work for a signature like |
Return an empty modifier array for the -1 offset used for nested types of substituted generic parameters, avoiding a native read before the signature buffer. Cover substitutions through generic classes and methods alongside synthetic array parameters.
Return a modified-type wrapper without nested signature metadata for type-variable and method-variable signatures. Assert that recursive modified-type results are non-null in the regression tests.
…tadataLoadContext
8ae2e95 to
4ca2f19
Compare
|
This is now ready and green @jkotas |
Return an empty modifier array for the -1 offset used for nested types of substituted generic parameters, avoiding a native read before the signature buffer. Cover substitutions through generic classes and methods alongside synthetic array parameters.
Closes #136848