Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UUM-78961] Fixed initialization of a class which has last fields in a table with 65535 field entries and the next class having no fields #2081

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

alexey-zakharov
Copy link

@alexey-zakharov alexey-zakharov commented Oct 24, 2024

UUM-78961 Fixed initialization of a class which has last fields in a table with 65535 field entries and the next class having no fields.

Mono crashes on allocation when initializing a class which has fields or methods that run till the end of the metadata table and the last TypeDef has no fields. This happens die to the last TypeDef without any fields having FieldList index 0 (null field).

The assembly was generated by Mono.Cecil in IL postrpocessor and thus several fix options were considered:

  • (Current PR) Fix mono
  • Fix Mono.Cecil
  • Fix in IL postprocessor

There are several related sections in the ECMA-335 standard interpreting which lead to the current fix.

  • II.22.37 TypeDef : 0x02 section

FieldList (an index into the Field table; it marks the first of a contiguous run of
Fields owned by this Type). The run continues to the smaller of:

  • the last row of the Field table
  • the next run of Fields, found by inspecting the FieldList of the next row in this TypeDef table
  • II.22.37 TypeDef : 0x02 "This contains informative text only" section
  1. FieldList can be null or non-null
  2. If FieldList is non-null, it shall index a valid row in the Field table, where valid
    means 1 <= row <= rowcount+1 [ERROR]
  • II.24.2.6 #~ stream section

If e is a simple index into a table with index i, it is stored using 2 bytes if table i has
less than 216 rows, otherwise it is stored using 4 bytes

Fixing Mono.Cecil to use large indices for the FieldList doesn't play well with II.24.2.6 #~ stream section as every existing reader won't be able to read the generated assembly.

Fixing IL postprocessor to generate a dummy field fixes that specific postprocessor usage case and won't fix loading any other assembly compiled externally.

I think fixing mono is the best option as it would ensure we can load assembly with this edge case metadata table generated by other sources too (cecil, netcore compiler, etc).
The current fix addressed FieldList and MethodList cases.

Related discussions:

  • Should this pull request have release notes?
    • Yes
    • No
  • Do these changes need to be back ported?
    • Yes
    • No
  • Do these changes need to be upstreamed to mono/mono or dotnet/runtime repositories?
    • Yes
    • No

Reviewers: please consider these questions as well! ❤️

Release notes

Fixed UUM-78961 @alexeyzakharov:
Mono: Fixed crash when loading a class which contains fields at the end of the metadata table with a table size 65535.

Backports
6, 2022.3

… 65535 field entries and the next class having no fields

(cherry picked from commit 0888d04)
@alexey-zakharov alexey-zakharov changed the title Fixed initialization of a class which has last fields in a table with 65535 field entries and the next class having no fields [UUM-78961] Fixed initialization of a class which has last fields in a table with 65535 field entries and the next class having no fields Oct 24, 2024
@alexey-zakharov alexey-zakharov marked this pull request as ready for review October 24, 2024 14:02
@alexey-zakharov alexey-zakharov self-assigned this Oct 24, 2024
@scott-ferguson-unity
Copy link

@alexey-zakharov If we don't update Mono.Cecil as well both the managed linker and IL2CPP will be broken.

@alexey-zakharov
Copy link
Author

alexey-zakharov commented Nov 4, 2024

@alexey-zakharov If we don't update Mono.Cecil as well both the managed linker and IL2CPP will be broken.

@scott-ferguson-unity there is an open PR for this for Mono.Cecil - jbevain/cecil#914
but it is not merged for some reasons

I agree that we should fix Mono.Cecil (and potentially CoreCLR), as well as update Mono.Cecil everywhere it is used. Perhaps the complete fix would be:

  • Land mono fixes
  • Land test (perhaps integration which builds player)
  • Land Mono.Cecil fixes
  • Update Mono.Cecil for
    • Burst
    • IL2CPP
    • other ILPPs
  • [Maybe] Fix CoreCLR

…f the table with 65635 methods and 0 in MethodList index in the last type doesnt crash
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants