Fix token memory ordering issue reading from the MethodDef and FieldDef token tables #123557
+14
−2
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ef token tables We have logic which loads a typedef, and if the typedef is loaded then we assume the method can be loaded via lookup in the MethodDef/FieldDef token map tables. HOWEVER, what this boils down to is we do a load from the TypeDef table, and if that succeeds we will do a load from the FieldDef/MethodDef token map tables, but that second load is done without an explicit memory barrier. Unfortunately, through the wonder of memory ordering behavior, its legal (and not actually all that uncommon) for the CPU to do the load from the FieldDef/MethodDef tables BEFORE it actually loads from the TypeDef table. So what can happen is that the fielddef table read can happen, then the typedef table read can happen, then the CPU does the if check to see if its ok to do the fielddef table read, then we use the value from the fielddef table read which happened earlier. This fix should fix that problem by inserting a memory barrier if there is ever a read of NULL from either of those tables, and retrying. Reads of NULL are significantly rarer than reads with values filled in, so this fix should not cause any meaningful performance issues. Fixes dotnet#120754
github-actions
bot
added
the
needs-area-label
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request fixes a critical memory ordering issue in the MethodDef and FieldDef token lookup functions that could cause sporadic MissingMethodException errors. The root cause is that these lookup maps are populated before the TypeDef table entry is stored, and due to CPU memory reordering, a read from these maps could happen before the TypeDef check, causing a null value to be used even when the entry was actually populated.
Changes:
- Added memory barrier protection in
LookupMethodDefto prevent CPU reordering when NULL is read - Moved
LookupFieldDeffrom header/cpp to inline file and added the same memory barrier protection - Unified DAC and non-DAC implementations of
LookupFieldDefinto a single implementation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/coreclr/vm/ceeload.inl | Added NULL-check with memory barrier retry logic to LookupMethodDef; moved LookupFieldDef implementation here with same memory barrier fix |
| src/coreclr/vm/ceeload.h | Removed conditional compilation guards and inline implementation of LookupFieldDef, keeping only forward declaration |
| src/coreclr/vm/ceeload.cpp | Removed DACCESS_COMPILE-specific LookupFieldDef implementation |
davidwrighton
added
area-VM-coreclr
and removed
needs-area-label
Co-authored-by: Copilot <[email protected]>
Contributor
|
Tagging subscribers to this area: @mangod9 |
jkotas
reviewed
Jan 23, 2026
…d FieldDef token tables" This reverts commit e793f3b.
…/davidwrighton/runtime into fix_memory_ordering_token_tables
AaronRobinsonMSFT
approved these changes
Jan 24, 2026
Co-authored-by: Aaron R Robinson <[email protected]>
…/davidwrighton/runtime into fix_memory_ordering_token_tables
jkotas
approved these changes
Jan 24, 2026
Member
Author
|
/backport to release/10.0 |
Contributor
|
Started backporting to |
Contributor
|
@davidwrighton backporting to git am output$ git am --3way --empty=keep --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix token memory ordering issue reading from the MethodDef and FieldDef token tables
Using index info to reconstruct a base tree...
M src/coreclr/vm/ceeload.cpp
M src/coreclr/vm/ceeload.h
M src/coreclr/vm/ceeload.inl
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/vm/ceeload.cpp
Auto-merging src/coreclr/vm/ceeload.h
Auto-merging src/coreclr/vm/ceeload.inl
Applying: Update src/coreclr/vm/ceeload.inl
Applying: Revert "Fix token memory ordering issue reading from the MethodDef and FieldDef token tables"
Using index info to reconstruct a base tree...
M src/coreclr/vm/ceeload.cpp
M src/coreclr/vm/ceeload.h
M src/coreclr/vm/ceeload.inl
Falling back to patching base and 3-way merge...
Auto-merging src/coreclr/vm/ceeload.cpp
Auto-merging src/coreclr/vm/ceeload.h
Auto-merging src/coreclr/vm/ceeload.inl
CONFLICT (content): Merge conflict in src/coreclr/vm/ceeload.inl
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config set advice.mergeConflict false"
Patch failed at 0003 Revert "Fix token memory ordering issue reading from the MethodDef and FieldDef token tables"
Error: The process '/usr/bin/git' failed with exit code 128 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We have logic which loads a typedef, and if the typedef is loaded then we assume the method can be loaded via lookup in the MethodDef/FieldDef token map tables. HOWEVER, what this boils down to is we do a load from the TypeDef table, and if that succeeds we will do a load from the FieldDef/MethodDef token map tables, but that second load is done without an explicit memory barrier. Unfortunately, through the wonder of memory ordering behavior, its legal (and not actually all that uncommon) for the CPU to do the load from the FieldDef/MethodDef tables BEFORE it actually loads from the TypeDef table. So what can happen is that the fielddef table read can happen, then the typedef table read can happen, then the CPU does the if check to see if its ok to do the fielddef table read, then we use the value from the fielddef table read which happened earlier.
This fix should fix that problem by using a VolatileLoad for all reads from the lookup maps. This is slightly slower but avoids the extremely easy to mess up dependency ordering rules.
Fixes #120754