From 3098827d03dbc9465c942d6cba718e8bc3f709ed Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 5 May 2026 16:57:55 -0700 Subject: [PATCH 1/7] Fix a series of possible race conditions in thread static variable initialization - The two critical ones are the one just after the memcpy in the TLSIndexToMethodTableMap, as well as the VolatileStore when setting pIndex. Fixes #127776 --- src/coreclr/vm/threadstatics.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index 23d8efd5be9fc2..167d7f705e9df1 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -230,8 +230,8 @@ void TLSIndexToMethodTableMap::Set(TLSIndex index, PTR_MethodTable pMT, bool isG memcpy(newMap, pMap, m_maxIndex * sizeof(TADDR)); // Don't delete the old map in case some other thread is reading from it, this won't waste significant amounts of memory, since this map cannot grow indefinitely } - pMap = newMap; - m_maxIndex = newSize; + VolatileStore(&pMap, newMap); // Use a volatile store, so that the memcpy is guaranteed to be visible before the new pMap is visible on other threads. + VolatileStore(&m_maxIndex, newSize); // Use a volatile store so that any thread that reads the size will also have a consistent view of a map at least as big as newSize } TADDR rawValue = dac_cast(pMT); @@ -245,7 +245,7 @@ void TLSIndexToMethodTableMap::Set(TLSIndex index, PTR_MethodTable pMT, bool isG m_collectibleEntries++; } _ASSERTE(pMap[index.GetIndexOffset()] == 0 || IsClearedValue(pMap[index.GetIndexOffset()])); - pMap[index.GetIndexOffset()] = rawValue; + VolatileStore(&pMap[index.GetIndexOffset()], rawValue); } void TLSIndexToMethodTableMap::Clear(TLSIndex index, uint8_t whenCleared) @@ -265,7 +265,7 @@ void TLSIndexToMethodTableMap::Clear(TLSIndex index, uint8_t whenCleared) { m_collectibleEntries--; } - pMap[index.GetIndexOffset()] = (whenCleared << 2) | 0x3; + VolatileStore(&pMap[index.GetIndexOffset()], (whenCleared << 2) | 0x3); _ASSERTE(GetClearedMarker(pMap[index.GetIndexOffset()]) == whenCleared); _ASSERTE(IsClearedValue(pMap[index.GetIndexOffset()])); } @@ -749,7 +749,7 @@ void GetTLSIndexForThreadStatic(MethodTable* pMT, bool gcStatic, TLSIndex* pInde usedDirectOnThreadLocalDataPath = true; } if (usedDirectOnThreadLocalDataPath) - VolatileStoreWithoutBarrier(&g_pMethodTablesForDirectThreadLocalData[IndexOffsetToDirectThreadLocalIndex(newTLSIndex.GetIndexOffset())], pMT); + VolatileStore(&g_pMethodTablesForDirectThreadLocalData[IndexOffsetToDirectThreadLocalIndex(newTLSIndex.GetIndexOffset())], pMT); } if (!usedDirectOnThreadLocalDataPath) @@ -772,7 +772,7 @@ void GetTLSIndexForThreadStatic(MethodTable* pMT, bool gcStatic, TLSIndex* pInde pMT->GetLoaderAllocator()->GetTLSIndexList().Append(newTLSIndex); } - *pIndex = newTLSIndex; + VolatileStore(pIndex, newTLSIndex); // Use a volatile store so that any other thread that sees the allocated index will also see the writes throughout this path. } void FreeTLSIndicesForLoaderAllocator(LoaderAllocator *pLoaderAllocator) From ed944214a5f8899761c1025a2373a747062fd5e9 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Wed, 6 May 2026 12:58:06 -0700 Subject: [PATCH 2/7] fix build break --- src/coreclr/vm/threadstatics.cpp | 2 +- src/coreclr/vm/threadstatics.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index 167d7f705e9df1..ddf1508be577d1 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -265,7 +265,7 @@ void TLSIndexToMethodTableMap::Clear(TLSIndex index, uint8_t whenCleared) { m_collectibleEntries--; } - VolatileStore(&pMap[index.GetIndexOffset()], (whenCleared << 2) | 0x3); + VolatileStore(&pMap[index.GetIndexOffset()], (TADDR)((whenCleared << 2) | 0x3)); _ASSERTE(GetClearedMarker(pMap[index.GetIndexOffset()]) == whenCleared); _ASSERTE(IsClearedValue(pMap[index.GetIndexOffset()])); } diff --git a/src/coreclr/vm/threadstatics.h b/src/coreclr/vm/threadstatics.h index afbb3f25039031..db1c90cba453c4 100644 --- a/src/coreclr/vm/threadstatics.h +++ b/src/coreclr/vm/threadstatics.h @@ -63,6 +63,7 @@ struct TLSIndex static TLSIndex Unallocated() { LIMITED_METHOD_DAC_CONTRACT; return TLSIndex(0xFFFFFFFF); } bool operator == (TLSIndex index) const { LIMITED_METHOD_DAC_CONTRACT; return TLSIndexRawIndex == index.TLSIndexRawIndex; } bool operator != (TLSIndex index) const { LIMITED_METHOD_DAC_CONTRACT; return TLSIndexRawIndex != index.TLSIndexRawIndex; } + volatile TLSIndex & operator=(const TLSIndex &other) volatile { LIMITED_METHOD_CONTRACT; VolatileStore((uint32_t*)&TLSIndexRawIndex, other.TLSIndexRawIndex); return *this; } }; // Used to store access to TLS data for a single index when the TLS is accessed while the class constructor is running From a3038bf25e68dd862a2938e4887ca465a4a7ba52 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 7 May 2026 14:57:14 -0700 Subject: [PATCH 3/7] Fix build break in a way that will hopefully work on all platforms --- src/coreclr/vm/threadstatics.cpp | 2 +- src/coreclr/vm/threadstatics.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index ddf1508be577d1..73e682cdbec776 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -772,7 +772,7 @@ void GetTLSIndexForThreadStatic(MethodTable* pMT, bool gcStatic, TLSIndex* pInde pMT->GetLoaderAllocator()->GetTLSIndexList().Append(newTLSIndex); } - VolatileStore(pIndex, newTLSIndex); // Use a volatile store so that any other thread that sees the allocated index will also see the writes throughout this path. + pIndex->VolatileStore(newTLSIndex); // Use a volatile store so that any other thread that sees the allocated index will also see the writes throughout this path. } void FreeTLSIndicesForLoaderAllocator(LoaderAllocator *pLoaderAllocator) diff --git a/src/coreclr/vm/threadstatics.h b/src/coreclr/vm/threadstatics.h index db1c90cba453c4..4d711d246900b5 100644 --- a/src/coreclr/vm/threadstatics.h +++ b/src/coreclr/vm/threadstatics.h @@ -63,7 +63,7 @@ struct TLSIndex static TLSIndex Unallocated() { LIMITED_METHOD_DAC_CONTRACT; return TLSIndex(0xFFFFFFFF); } bool operator == (TLSIndex index) const { LIMITED_METHOD_DAC_CONTRACT; return TLSIndexRawIndex == index.TLSIndexRawIndex; } bool operator != (TLSIndex index) const { LIMITED_METHOD_DAC_CONTRACT; return TLSIndexRawIndex != index.TLSIndexRawIndex; } - volatile TLSIndex & operator=(const TLSIndex &other) volatile { LIMITED_METHOD_CONTRACT; VolatileStore((uint32_t*)&TLSIndexRawIndex, other.TLSIndexRawIndex); return *this; } + void VolatileStore(const TLSIndex &other) { LIMITED_METHOD_CONTRACT; ::VolatileStore((uint32_t*)&TLSIndexRawIndex, other.TLSIndexRawIndex); } }; // Used to store access to TLS data for a single index when the TLS is accessed while the class constructor is running From 13c3840cc5ba8aaa337769d6aa5e43bc1ea0e6da Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Thu, 7 May 2026 16:58:46 -0700 Subject: [PATCH 4/7] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- src/coreclr/vm/threadstatics.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/vm/threadstatics.h b/src/coreclr/vm/threadstatics.h index 4d711d246900b5..255abc0b3f6ed7 100644 --- a/src/coreclr/vm/threadstatics.h +++ b/src/coreclr/vm/threadstatics.h @@ -63,7 +63,7 @@ struct TLSIndex static TLSIndex Unallocated() { LIMITED_METHOD_DAC_CONTRACT; return TLSIndex(0xFFFFFFFF); } bool operator == (TLSIndex index) const { LIMITED_METHOD_DAC_CONTRACT; return TLSIndexRawIndex == index.TLSIndexRawIndex; } bool operator != (TLSIndex index) const { LIMITED_METHOD_DAC_CONTRACT; return TLSIndexRawIndex != index.TLSIndexRawIndex; } - void VolatileStore(const TLSIndex &other) { LIMITED_METHOD_CONTRACT; ::VolatileStore((uint32_t*)&TLSIndexRawIndex, other.TLSIndexRawIndex); } + void VolatileStore(const TLSIndex &other) { LIMITED_METHOD_CONTRACT; ::VolatileStore(&TLSIndexRawIndex, other.TLSIndexRawIndex); } }; // Used to store access to TLS data for a single index when the TLS is accessed while the class constructor is running From a2be2128d9174ee42d17f4f2974ba09ac9d78565 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 11 May 2026 14:48:58 -0700 Subject: [PATCH 5/7] Update comments --- src/coreclr/vm/threadstatics.cpp | 5 +++-- src/coreclr/vm/threadstatics.h | 11 +++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index 73e682cdbec776..3931b87e613d27 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -164,6 +164,7 @@ PTR_MethodTable LookupMethodTableAndFlagForThreadStatic(TLSIndex index, bool *pI } CONTRACTL_END; + // This method is called without the g_TLSCrst lock being held PTR_MethodTable retVal; if (index.GetTLSIndexType() == TLSIndexType::NonCollectible) { @@ -230,8 +231,8 @@ void TLSIndexToMethodTableMap::Set(TLSIndex index, PTR_MethodTable pMT, bool isG memcpy(newMap, pMap, m_maxIndex * sizeof(TADDR)); // Don't delete the old map in case some other thread is reading from it, this won't waste significant amounts of memory, since this map cannot grow indefinitely } - VolatileStore(&pMap, newMap); // Use a volatile store, so that the memcpy is guaranteed to be visible before the new pMap is visible on other threads. - VolatileStore(&m_maxIndex, newSize); // Use a volatile store so that any thread that reads the size will also have a consistent view of a map at least as big as newSize + VolatileStore(&pMap, newMap); // Use a volatile store, so that the memcpy is guaranteed to be visible before the new pMap is visible on other threads. This is paired with VolatileLoad in the Lookup functions although it probably could be a VolatileLoadWithoutBarrier and take advantage of data dependency. + VolatileStore(&m_maxIndex, newSize); // Use a volatile store so that any thread that reads the size will also have a consistent view of a map at least as big as newSize. This is paired with VolatileLoad in the Lookup functions. } TADDR rawValue = dac_cast(pMT); diff --git a/src/coreclr/vm/threadstatics.h b/src/coreclr/vm/threadstatics.h index 255abc0b3f6ed7..57c8b4fd50c84b 100644 --- a/src/coreclr/vm/threadstatics.h +++ b/src/coreclr/vm/threadstatics.h @@ -124,6 +124,7 @@ class TLSIndexToMethodTableMap PTR_MethodTable Lookup(TLSIndex index, bool *isGCStatic, bool *isCollectible) const { LIMITED_METHOD_CONTRACT; + // This method is called without the g_TLSCrst lock being held *isGCStatic = false; *isCollectible = false; if (index.GetIndexOffset() < VolatileLoad(&m_maxIndex)) @@ -143,6 +144,7 @@ class TLSIndexToMethodTableMap PTR_MethodTable LookupTlsIndexKnownToBeAllocated(TLSIndex index) const { LIMITED_METHOD_CONTRACT; + // This method is called without the g_TLSCrst lock being held if (index.GetIndexOffset() < VolatileLoad(&m_maxIndex)) { TADDR rawValue = VolatileLoadWithoutBarrier(&VolatileLoad(&pMap)[index.GetIndexOffset()]); @@ -166,9 +168,14 @@ class TLSIndexToMethodTableMap entry Lookup(TLSIndex index) const { + // This method is called with the g_TLSCrst lock held, so we don't actually + // need all of these volatile loads, but using VolatileLoad is more similar to the other + // paths, and the performance penalty is negligible, so we keep them. + LIMITED_METHOD_CONTRACT; entry e(index); - if (index.GetIndexOffset() < VolatileLoad(&m_maxIndex)) + int32_t maxIndex = VolatileLoad(&m_maxIndex); // This is the VolatileLoad that pairs with + if (index.GetIndexOffset() < maxIndex) { TADDR rawValue = VolatileLoadWithoutBarrier(&VolatileLoad(&pMap)[index.GetIndexOffset()]); if (!IsClearedValue(rawValue)) @@ -185,7 +192,7 @@ class TLSIndexToMethodTableMap } else { - e.TlsIndex = TLSIndex(m_indexType, m_maxIndex); + e.TlsIndex = TLSIndex(m_indexType, maxIndex); } return e; } From 62872bdc8c08d431deef4748c35c7b5b1be73d31 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Mon, 11 May 2026 17:02:25 -0700 Subject: [PATCH 6/7] Add an explanatory comment ... --- src/coreclr/vm/threadstatics.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index 3931b87e613d27..4001419741e5f2 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -246,7 +246,12 @@ void TLSIndexToMethodTableMap::Set(TLSIndex index, PTR_MethodTable pMT, bool isG m_collectibleEntries++; } _ASSERTE(pMap[index.GetIndexOffset()] == 0 || IsClearedValue(pMap[index.GetIndexOffset()])); - VolatileStore(&pMap[index.GetIndexOffset()], rawValue); + // This VolatileStore does not have a clear consumer-producer relationship. Notably, the MethodTable is always considered to have been + // fully published by the time its pointer is stored in the map, so there is no need for a memory barrier to ensure visibility of the MethodTable's contents. + // However, we have had issues with race safety in this codebase, and we fixed them by adding several VolatileStore which we believe are actually critical + // but this is a scenario which AI indicated should also have a VolatileStore. We've chosen to add the VolatileStore as a defensive measure, as this is very + // rarely run code. + VolatileStore(&pMap[index.GetIndexOffset()], rawValue); } void TLSIndexToMethodTableMap::Clear(TLSIndex index, uint8_t whenCleared) From eff1e6cbcafeac71cb0bd7492ba89f71b5701453 Mon Sep 17 00:00:00 2001 From: David Wrighton Date: Tue, 12 May 2026 10:53:36 -0700 Subject: [PATCH 7/7] Fix more copilot feedback --- src/coreclr/vm/threadstatics.cpp | 2 +- src/coreclr/vm/threadstatics.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/threadstatics.cpp b/src/coreclr/vm/threadstatics.cpp index 4001419741e5f2..e4d94eac463453 100644 --- a/src/coreclr/vm/threadstatics.cpp +++ b/src/coreclr/vm/threadstatics.cpp @@ -174,7 +174,7 @@ PTR_MethodTable LookupMethodTableAndFlagForThreadStatic(TLSIndex index, bool *pI { *pIsGCStatic = false; *pIsCollectible = false; - retVal = g_pMethodTablesForDirectThreadLocalData[IndexOffsetToDirectThreadLocalIndex(index.GetIndexOffset())]; + retVal = VolatileLoadWithoutBarrier(&g_pMethodTablesForDirectThreadLocalData[IndexOffsetToDirectThreadLocalIndex(index.GetIndexOffset())]); } else { diff --git a/src/coreclr/vm/threadstatics.h b/src/coreclr/vm/threadstatics.h index 57c8b4fd50c84b..0a77a356e33db4 100644 --- a/src/coreclr/vm/threadstatics.h +++ b/src/coreclr/vm/threadstatics.h @@ -174,7 +174,7 @@ class TLSIndexToMethodTableMap LIMITED_METHOD_CONTRACT; entry e(index); - int32_t maxIndex = VolatileLoad(&m_maxIndex); // This is the VolatileLoad that pairs with + int32_t maxIndex = VolatileLoad(&m_maxIndex); // This VolatileLoad pairs with a VolatileStore in TLSIndexToMethodTableMap::Set to ensure that if we read a maxIndex that is large enough to contain our index if (index.GetIndexOffset() < maxIndex) { TADDR rawValue = VolatileLoadWithoutBarrier(&VolatileLoad(&pMap)[index.GetIndexOffset()]);