From 1d723a06748251e8be083f61af6c7e13070f65ba Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Mon, 5 Sep 2022 17:13:23 -0700 Subject: [PATCH 1/3] Fix race condition in DacEnumerableHashTable::BaseFindNextEntryByHash --- src/coreclr/vm/dacenumerablehash.inl | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/coreclr/vm/dacenumerablehash.inl b/src/coreclr/vm/dacenumerablehash.inl index 02024c229ca7b3..b3f76c6b6bf868 100644 --- a/src/coreclr/vm/dacenumerablehash.inl +++ b/src/coreclr/vm/dacenumerablehash.inl @@ -335,7 +335,7 @@ DPTR(VALUE) DacEnumerableHashTable::BaseFindFirstEntryByHash // in a rare case if resize is in progress, look in the new table as well. // if existing entry is not in the old table, it must be in the new // since we unlink it from old only after linking into the new. - // check for next table must hapen after we looked through the current. + // check for next table must happen after we looked through the current. VolatileLoadBarrier(); curBuckets = GetNext(curBuckets); } while (curBuckets != nullptr); @@ -367,11 +367,9 @@ DPTR(VALUE) DacEnumerableHashTable::BaseFindNextEntryByHash( PTR_VolatileEntry pVolatileEntry = dac_cast(pContext->m_pEntry); iHash = pVolatileEntry->m_iHashValue; - // Iterate over the bucket chain. - while (pVolatileEntry->m_pNextEntry) + // Iterate over the rest ot the bucket chain. + while ((pVolatileEntry = pVolatileEntry->m_pNextEntry) != nullptr) { - // Advance to the next entry. - pVolatileEntry = pVolatileEntry->m_pNextEntry; if (pVolatileEntry->m_iHashValue == iHash) { // Found a match on hash code. Update our find context to indicate where we got to and return @@ -381,7 +379,7 @@ DPTR(VALUE) DacEnumerableHashTable::BaseFindNextEntryByHash( } } - // check for next table must hapen after we looked through the current. + // check for next table must happen after we looked through the current. VolatileLoadBarrier(); // in a case if resize is in progress, look in the new table as well. From b40f79218692e82140c3afdd6b4b66ddc420440e Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Mon, 5 Sep 2022 20:05:10 -0700 Subject: [PATCH 2/3] Use a volatile load --- src/coreclr/vm/dacenumerablehash.inl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/dacenumerablehash.inl b/src/coreclr/vm/dacenumerablehash.inl index b3f76c6b6bf868..a6114d0068eeab 100644 --- a/src/coreclr/vm/dacenumerablehash.inl +++ b/src/coreclr/vm/dacenumerablehash.inl @@ -329,7 +329,7 @@ DPTR(VALUE) DacEnumerableHashTable::BaseFindFirstEntryByHash } // Move to the next entry in the chain. - pEntry = pEntry->m_pNextEntry; + pEntry = VolatileLoadWithoutBarrier(&pEntry->m_pNextEntry); } // in a rare case if resize is in progress, look in the new table as well. @@ -368,7 +368,7 @@ DPTR(VALUE) DacEnumerableHashTable::BaseFindNextEntryByHash( iHash = pVolatileEntry->m_iHashValue; // Iterate over the rest ot the bucket chain. - while ((pVolatileEntry = pVolatileEntry->m_pNextEntry) != nullptr) + while ((pVolatileEntry = VolatileLoadWithoutBarrier(&pVolatileEntry->m_pNextEntry)) != nullptr) { if (pVolatileEntry->m_iHashValue == iHash) { From 84419db28a2b39a0a90a1100e76848effbb7fbb1 Mon Sep 17 00:00:00 2001 From: Anton Lapounov Date: Mon, 5 Sep 2022 23:59:02 -0700 Subject: [PATCH 3/3] One more volatile load --- src/coreclr/vm/dacenumerablehash.inl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/vm/dacenumerablehash.inl b/src/coreclr/vm/dacenumerablehash.inl index a6114d0068eeab..cfee7b8f684be3 100644 --- a/src/coreclr/vm/dacenumerablehash.inl +++ b/src/coreclr/vm/dacenumerablehash.inl @@ -309,8 +309,8 @@ DPTR(VALUE) DacEnumerableHashTable::BaseFindFirstEntryByHash // +2 to skip "length" and "next" slots DWORD dwBucket = iHash % cBuckets + SKIP_SPECIAL_SLOTS; - // Point at the first entry in the bucket chain which would contain any entries with the given hash code. - PTR_VolatileEntry pEntry = curBuckets[dwBucket]; + // Point at the first entry in the bucket chain that stores entries with the given hash code. + PTR_VolatileEntry pEntry = VolatileLoadWithoutBarrier(&curBuckets[dwBucket]); // Walk the bucket chain one entry at a time. while (pEntry)