Fix token memory ordering issue reading from the MethodDef and FieldDef token tables #123557
Conversation
…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
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 <175728472+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @mangod9 |
…d FieldDef token tables" This reverts commit e793f3b.
…/davidwrighton/runtime into fix_memory_ordering_token_tables
| // dependency instead of an explicit barrier here. However, the access pattern between | ||
| // m_TypeDefToMethodTableMap and m_MethodDefToDescMap/m_FieldDefToDescMap is such that a | ||
| // data dependency is not sufficient to ensure that the MethodTable is visible when we | ||
| // access the MethodDesc/FieldDesc. Since those loads are independent. So we use |
There was a problem hiding this comment.
| // access the MethodDesc/FieldDesc. Since those loads are independent. So we use | |
| // access the MethodDesc/FieldDesc. Since those loads are independent, we use |
| // LookupMap's hold pointers to data which is immutable, so normally we could use a data | ||
| // dependency instead of an explicit barrier here. However, the access pattern between | ||
| // m_TypeDefToMethodTableMap and m_MethodDefToDescMap/m_FieldDefToDescMap is such that a | ||
| // data dependency is not sufficient to ensure that the MethodTable is visible when we | ||
| // access the MethodDesc/FieldDesc. Since those loads are independent. So we use | ||
| // VolatileLoad here to ensure proper ordering. | ||
| TADDR value = VolatileLoad(pValue); |
There was a problem hiding this comment.
This fix only applies to the LookupMap<SIZE_T> template specialization, but the maps mentioned in the comment (m_TypeDefToMethodTableMap, m_MethodDefToDescMap, and m_FieldDefToDescMap) are actually LookupMap<PTR_MethodTable>, LookupMap<PTR_MethodDesc>, and LookupMap<PTR_FieldDesc> respectively. These use the generic LookupMap template at line 16, which still uses VolatileLoadWithoutBarrier. The memory ordering fix needs to be applied to the generic template as well, otherwise the race condition described in the PR is not actually fixed.
| // access the MethodDesc/FieldDesc. Since those loads are independent. So we use | ||
| // VolatileLoad here to ensure proper ordering. |
There was a problem hiding this comment.
The sentence "Since those loads are independent." is incomplete. It should be connected to the previous sentence or completed with a clause explaining the consequence. Consider: "However, the access pattern between m_TypeDefToMethodTableMap and m_MethodDefToDescMap/m_FieldDefToDescMap is such that a data dependency is not sufficient to ensure that the MethodTable is visible when we access the MethodDesc/FieldDesc, since those loads are independent."
| // access the MethodDesc/FieldDesc. Since those loads are independent. So we use | |
| // VolatileLoad here to ensure proper ordering. | |
| // access the MethodDesc/FieldDesc, because those loads are independent. Therefore, we | |
| // use VolatileLoad here to ensure proper ordering. |
| // data dependency is not sufficient to ensure that the MethodTable is visible when we | ||
| // access the MethodDesc/FieldDesc. Since those loads are independent. So we use | ||
| // VolatileLoad here to ensure proper ordering. | ||
| TADDR value = VolatileLoad(pValue); |
There was a problem hiding this comment.
The PR description states the fix will insert "a memory barrier if there is ever a read of NULL from either of those tables, and retrying", but the actual implementation unconditionally adds a memory barrier via VolatileLoad without any NULL check or retry logic. This discrepancy suggests either the PR description is inaccurate or the implementation is incomplete.
3 participants
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 #120754