From 50270dc488f522453f2cb907119598d0ac862570 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Sat, 24 Jan 2026 04:18:23 +0000 Subject: [PATCH] Fix token memory ordering issue reading from the MethodDef and FieldDef token tables (#123557) 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 --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Aaron R Robinson --- src/coreclr/vm/ceeload.inl | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/ceeload.inl b/src/coreclr/vm/ceeload.inl index 2067de38088fd9..a9d414dd30c62d 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 @@ -51,7 +57,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, we use + // VolatileLoad here to ensure proper ordering. + TADDR value = VolatileLoad(pValue); if (pFlags) *pFlags = value & supportedFlags;