From e793f3b40b8ee8ecb92a2309618313f0a9c0ee66 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 23 Jan 2026 11:17:39 -0800 Subject: [PATCH 1/6] Fix token memory ordering issue reading from the MethodDef and FieldDef 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 #120754 --- src/coreclr/vm/ceeload.cpp | 7 ------- src/coreclr/vm/ceeload.h | 11 ----------- src/coreclr/vm/ceeload.inl | 29 ++++++++++++++++++++++++++++- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 1860164c62e19a..0fea8deaf18cb5 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -4644,13 +4644,6 @@ void Module::EnumMemoryRegions(CLRDataEnumMemoryFlags flags, #endif // FEATURE_METADATA_UPDATER } -FieldDesc *Module::LookupFieldDef(mdFieldDef token) -{ - WRAPPER_NO_CONTRACT; - _ASSERTE(TypeFromToken(token) == mdtFieldDef); - return m_FieldDefToDescMap.GetElement(RidFromToken(token)); -} - #endif // DACCESS_COMPILE diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index b851f2d3275a52..8cf94aa14703ee 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1278,18 +1278,7 @@ class Module : public ModuleBase #endif // !DACCESS_COMPILE #endif // FEATURE_CODE_VERSIONING -#ifndef DACCESS_COMPILE - FieldDesc *LookupFieldDef(mdFieldDef token) - { - WRAPPER_NO_CONTRACT; - - _ASSERTE(TypeFromToken(token) == mdtFieldDef); - return m_FieldDefToDescMap.GetElement(RidFromToken(token)); - } -#else // DACCESS_COMPILE - // FieldDesc isn't defined at this point so PTR_FieldDesc can't work. FieldDesc *LookupFieldDef(mdFieldDef token); -#endif // DACCESS_COMPILE #ifndef DACCESS_COMPILE void EnsureFieldDefCanBeStored(mdFieldDef token) diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index 9989c3e5cf209b..b77d4796d270b8 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -271,7 +271,34 @@ inline PTR_MethodDesc Module::LookupMethodDef(mdMethodDef token) CONTRACTL_END _ASSERTE(TypeFromToken(token) == mdtMethodDef); - return m_MethodDefToDescMap.GetElement(RidFromToken(token)); + PTR_MethodDesc pResult = m_MethodDefToDescMap.GetElement(RidFromToken(token)); + if (pResult == NULL) + { + MemoryBarrier(); // m_MethodDefToDescMap is initialized in a specific order with regard to the m_TypeDefToDescMap, and is used + // by reading the TypeDefToDescMap, and then if a value is found in there, reading from this map. + // Since those two reads are not dependent on each other it is possible for the compiler or CPU to reorder them. + // The MemoryBarrier here prevents that reordering. + + pResult = m_MethodDefToDescMap.GetElement(RidFromToken(token)); + } + return pResult; +} + +FieldDesc *Module::LookupFieldDef(mdFieldDef token) +{ + WRAPPER_NO_CONTRACT; + _ASSERTE(TypeFromToken(token) == mdtFieldDef); + FieldDesc* pResult = m_FieldDefToDescMap.GetElement(RidFromToken(token)); + + if (pResult == NULL) + { + MemoryBarrier(); // m_FieldDefToDescMap is initialized in a specific order with regard to the m_TypeDefToDescMap, and is used + // by reading the TypeDefToDescMap, and then if a value is found in there, reading from this map. + // Since those two reads are not dependent on each other it is possible for the compiler or CPU to reorder them. + // The MemoryBarrier here prevents that reordering. + pResult = m_FieldDefToDescMap.GetElement(RidFromToken(token)); + } + return pResult; } #ifdef FEATURE_CODE_VERSIONING From 74f27ae63f7eea36eff25fffdb25ba04670ed138 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 23 Jan 2026 11:32:59 -0800 Subject: [PATCH 2/6] Update src/coreclr/vm/ceeload.inl Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/coreclr/vm/ceeload.inl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index b77d4796d270b8..b5aa0c04761198 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -293,9 +293,9 @@ FieldDesc *Module::LookupFieldDef(mdFieldDef token) if (pResult == NULL) { MemoryBarrier(); // m_FieldDefToDescMap is initialized in a specific order with regard to the m_TypeDefToDescMap, and is used - // by reading the TypeDefToDescMap, and then if a value is found in there, reading from this map. - // Since those two reads are not dependent on each other it is possible for the compiler or CPU to reorder them. - // The MemoryBarrier here prevents that reordering. + // by reading the TypeDefToDescMap, and then if a value is found in there, reading from this map. + // Since those two reads are not dependent on each other it is possible for the compiler or CPU to reorder them. + // The MemoryBarrier here prevents that reordering. pResult = m_FieldDefToDescMap.GetElement(RidFromToken(token)); } return pResult; From bf3edf0edcc5b8a981aa22a70e9b24d30f9c930e Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 23 Jan 2026 12:15:21 -0800 Subject: [PATCH 3/6] Revert "Fix token memory ordering issue reading from the MethodDef and FieldDef token tables" This reverts commit e793f3b40b8ee8ecb92a2309618313f0a9c0ee66. --- src/coreclr/vm/ceeload.cpp | 7 +++++++ src/coreclr/vm/ceeload.h | 11 +++++++++++ src/coreclr/vm/ceeload.inl | 29 +---------------------------- 3 files changed, 19 insertions(+), 28 deletions(-) diff --git a/src/coreclr/vm/ceeload.cpp b/src/coreclr/vm/ceeload.cpp index 0fea8deaf18cb5..1860164c62e19a 100644 --- a/src/coreclr/vm/ceeload.cpp +++ b/src/coreclr/vm/ceeload.cpp @@ -4644,6 +4644,13 @@ void Module::EnumMemoryRegions(CLRDataEnumMemoryFlags flags, #endif // FEATURE_METADATA_UPDATER } +FieldDesc *Module::LookupFieldDef(mdFieldDef token) +{ + WRAPPER_NO_CONTRACT; + _ASSERTE(TypeFromToken(token) == mdtFieldDef); + return m_FieldDefToDescMap.GetElement(RidFromToken(token)); +} + #endif // DACCESS_COMPILE diff --git a/src/coreclr/vm/ceeload.h b/src/coreclr/vm/ceeload.h index 8cf94aa14703ee..b851f2d3275a52 100644 --- a/src/coreclr/vm/ceeload.h +++ b/src/coreclr/vm/ceeload.h @@ -1278,7 +1278,18 @@ class Module : public ModuleBase #endif // !DACCESS_COMPILE #endif // FEATURE_CODE_VERSIONING +#ifndef DACCESS_COMPILE + FieldDesc *LookupFieldDef(mdFieldDef token) + { + WRAPPER_NO_CONTRACT; + + _ASSERTE(TypeFromToken(token) == mdtFieldDef); + return m_FieldDefToDescMap.GetElement(RidFromToken(token)); + } +#else // DACCESS_COMPILE + // FieldDesc isn't defined at this point so PTR_FieldDesc can't work. FieldDesc *LookupFieldDef(mdFieldDef token); +#endif // DACCESS_COMPILE #ifndef DACCESS_COMPILE void EnsureFieldDefCanBeStored(mdFieldDef token) diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index b77d4796d270b8..9989c3e5cf209b 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -271,34 +271,7 @@ inline PTR_MethodDesc Module::LookupMethodDef(mdMethodDef token) CONTRACTL_END _ASSERTE(TypeFromToken(token) == mdtMethodDef); - PTR_MethodDesc pResult = m_MethodDefToDescMap.GetElement(RidFromToken(token)); - if (pResult == NULL) - { - MemoryBarrier(); // m_MethodDefToDescMap is initialized in a specific order with regard to the m_TypeDefToDescMap, and is used - // by reading the TypeDefToDescMap, and then if a value is found in there, reading from this map. - // Since those two reads are not dependent on each other it is possible for the compiler or CPU to reorder them. - // The MemoryBarrier here prevents that reordering. - - pResult = m_MethodDefToDescMap.GetElement(RidFromToken(token)); - } - return pResult; -} - -FieldDesc *Module::LookupFieldDef(mdFieldDef token) -{ - WRAPPER_NO_CONTRACT; - _ASSERTE(TypeFromToken(token) == mdtFieldDef); - FieldDesc* pResult = m_FieldDefToDescMap.GetElement(RidFromToken(token)); - - if (pResult == NULL) - { - MemoryBarrier(); // m_FieldDefToDescMap is initialized in a specific order with regard to the m_TypeDefToDescMap, and is used - // by reading the TypeDefToDescMap, and then if a value is found in there, reading from this map. - // Since those two reads are not dependent on each other it is possible for the compiler or CPU to reorder them. - // The MemoryBarrier here prevents that reordering. - pResult = m_FieldDefToDescMap.GetElement(RidFromToken(token)); - } - return pResult; + return m_MethodDefToDescMap.GetElement(RidFromToken(token)); } #ifdef FEATURE_CODE_VERSIONING From ef8a25ab4760fc83de8c30243827bb8bb27fa214 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 23 Jan 2026 14:30:11 -0800 Subject: [PATCH 4/6] Simpler fix --- src/coreclr/vm/ceeload.inl | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index 9989c3e5cf209b..5296527663486f 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -51,7 +51,13 @@ SIZE_T LookupMap::GetValueAt(PTR_TADDR pValue, TADDR* pFlags, TADDR supp { WRAPPER_NO_CONTRACT; - TADDR value = VolatileLoadWithoutBarrier(pValue); // LookupMap's hold pointers, so we can use a data dependency instead of an explicit barrier here. + // 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); if (pFlags) *pFlags = value & supportedFlags; From 9d9c7746ed12646450fcc0c21de531878bd02b64 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 23 Jan 2026 16:55:33 -0800 Subject: [PATCH 5/6] Update src/coreclr/vm/ceeload.inl Co-authored-by: Aaron R Robinson --- src/coreclr/vm/ceeload.inl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index 5296527663486f..b2da5f123e6407 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -55,7 +55,7 @@ SIZE_T LookupMap::GetValueAt(PTR_TADDR pValue, TADDR* pFlags, TADDR supp // 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 + // access the MethodDesc/FieldDesc. Since those loads are independent, we use // VolatileLoad here to ensure proper ordering. TADDR value = VolatileLoad(pValue); From 0880741d3d7fffd7771d97d04b824c27ebb497b6 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Fri, 23 Jan 2026 16:59:09 -0800 Subject: [PATCH 6/6] Fix the other GetValueAt --- src/coreclr/vm/ceeload.inl | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index 5296527663486f..0a2d07474aa635 100644 --- a/src/coreclr/vm/ceeload.inl +++ b/src/coreclr/vm/ceeload.inl @@ -18,7 +18,13 @@ TYPE LookupMap::GetValueAt(PTR_TADDR pValue, TADDR* pFlags, TADDR supporte WRAPPER_NO_CONTRACT; SUPPORTS_DAC; #ifndef DACCESS_COMPILE - TYPE value = dac_cast(VolatileLoadWithoutBarrier(pValue)); // LookupMap's hold pointers, so we can use a data dependency instead of an explicit barrier here. + // 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, we use + // VolatileLoad here to ensure proper ordering. + TYPE value = dac_cast(VolatileLoad(pValue)); #else TYPE value = dac_cast(*pValue); #endif @@ -55,7 +61,7 @@ SIZE_T LookupMap::GetValueAt(PTR_TADDR pValue, TADDR* pFlags, TADDR supp // 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 + // access the MethodDesc/FieldDesc. Since those loads are independent, we use // VolatileLoad here to ensure proper ordering. TADDR value = VolatileLoad(pValue);