From 6f9c71d95948462d94c171eb4872c0dc8109e15a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 4 Jan 2026 00:31:33 +0000 Subject: [PATCH 1/6] Initial plan From c1b95d6faf55e8bf0858bd272762f587b0bd7d38 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 4 Jan 2026 00:38:52 +0000 Subject: [PATCH 2/6] Replace ThreadLocalStorage with lock-per-bucket implementation and add multi-threaded test Co-authored-by: jonwis <18537118+jonwis@users.noreply.github.com> --- include/wil/result.h | 71 ++++++++++++++++++++++--------------------- tests/ResultTests.cpp | 62 +++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 35 deletions(-) diff --git a/include/wil/result.h b/include/wil/result.h index f4de6c6b..dfd38ab1 100644 --- a/include/wil/result.h +++ b/include/wil/result.h @@ -383,65 +383,66 @@ namespace details_abi ~ThreadLocalStorage() WI_NOEXCEPT { - for (auto& entry : m_hashArray) - { - Node* pNode = entry; - while (pNode != nullptr) - { - auto pCurrent = pNode; -#pragma warning(push) -#pragma warning(disable : 6001) // https://github.com/microsoft/wil/issues/164 - pNode = pNode->pNext; -#pragma warning(pop) - pCurrent->~Node(); - ::HeapFree(::GetProcessHeap(), 0, pCurrent); - } - entry = nullptr; - } + // Cleanup is automatic via unique_process_heap_ptr destructors } // Note: Can return nullptr even when (shouldAllocate == true) upon allocation failure T* GetLocal(bool shouldAllocate = false) WI_NOEXCEPT { + // Get the current thread ID DWORD const threadId = ::GetCurrentThreadId(); + + // Determine the appropriate bucket for this thread size_t const index = ((threadId >> 2) % ARRAYSIZE(m_hashArray)); // Reduce hash collisions; thread IDs are even. - for (auto pNode = m_hashArray[index]; pNode != nullptr; pNode = pNode->pNext) + Bucket& bucket = m_hashArray[index]; + + // Lock the bucket and search for an existing entry { - if (pNode->threadId == threadId) + auto lock = bucket.lock.lock_shared(); + for (auto pNode = bucket.head.get(); pNode != nullptr; pNode = pNode->pNext.get()) { - return &pNode->value; + if (pNode->threadId == threadId) + { + return &pNode->value; + } } } - if (shouldAllocate) + if (!shouldAllocate) { - if (auto pNewRaw = details::ProcessHeapAlloc(0, sizeof(Node))) - { - auto pNew = new (pNewRaw) Node{threadId}; - - Node* pFirst; - do - { - pFirst = m_hashArray[index]; - pNew->pNext = pFirst; - } while (::InterlockedCompareExchangePointer(reinterpret_cast(m_hashArray + index), pNew, pFirst) != - pFirst); + return nullptr; + } - return &pNew->value; - } + // No entry for us, make a new one and insert it at the head + wil::unique_process_heap_ptr newNode; + void* newMemory = details::ProcessHeapAlloc(0, sizeof(Node)); + if (!newMemory) + { + return nullptr; } - return nullptr; + newNode.reset(new (newMemory) Node{threadId}); + + auto lock = bucket.lock.lock_exclusive(); + newNode->pNext = wistd::move(bucket.head); + bucket.head = wistd::move(newNode); + return &bucket.head->value; } private: struct Node { DWORD threadId = 0xffffffffU; - Node* pNext = nullptr; + wil::unique_process_heap_ptr pNext; T value{}; }; - Node* volatile m_hashArray[10]{}; + struct Bucket + { + wil::srwlock lock; + wil::unique_process_heap_ptr head; + }; + + Bucket m_hashArray[10]{}; }; struct ThreadLocalFailureInfo diff --git a/tests/ResultTests.cpp b/tests/ResultTests.cpp index 8939c4a8..7f701536 100644 --- a/tests/ResultTests.cpp +++ b/tests/ResultTests.cpp @@ -12,6 +12,10 @@ #include "common.h" +#include +#include +#include + static volatile long objectCount = 0; struct SharedObject { @@ -773,3 +777,61 @@ TEST_CASE("ResultTests::ReportDoesNotChangeLastError", "[result]") LOG_IF_WIN32_BOOL_FALSE(FALSE); REQUIRE(::GetLastError() == ERROR_ABIOS_ERROR); } + +TEST_CASE("ResultTests::ThreadLocalStorage", "[result]") +{ + constexpr int NUM_THREADS = 10; + constexpr int ITERATIONS = 1000; + + wil::details_abi::ThreadLocalStorage storage; + std::atomic errors{0}; + std::vector threads; + + // Create multiple threads that will access the thread local storage concurrently + for (int i = 0; i < NUM_THREADS; ++i) + { + threads.emplace_back([&storage, &errors, i]() { + for (int j = 0; j < ITERATIONS; ++j) + { + // Get or create thread local value + int* pValue = storage.GetLocal(true); + if (!pValue) + { + errors.fetch_add(1); + continue; + } + + // First time should be zero-initialized + if (j == 0 && *pValue != 0) + { + errors.fetch_add(1); + } + + // Set a thread-specific value + *pValue = i * 1000 + j; + + // Verify we get the same pointer on subsequent calls + int* pValue2 = storage.GetLocal(false); + if (pValue != pValue2) + { + errors.fetch_add(1); + } + + // Verify the value is correct + if (*pValue2 != i * 1000 + j) + { + errors.fetch_add(1); + } + } + }); + } + + // Wait for all threads to complete + for (auto& thread : threads) + { + thread.join(); + } + + // Verify no errors occurred + REQUIRE(errors.load() == 0); +} From 7852833204affd0b1e1fe4797ca06e12190c24b1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 4 Jan 2026 00:40:29 +0000 Subject: [PATCH 3/6] Fix race condition in ThreadLocalStorage by rechecking after acquiring exclusive lock Co-authored-by: jonwis <18537118+jonwis@users.noreply.github.com> --- include/wil/result.h | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/include/wil/result.h b/include/wil/result.h index dfd38ab1..7a940996 100644 --- a/include/wil/result.h +++ b/include/wil/result.h @@ -423,6 +423,17 @@ namespace details_abi newNode.reset(new (newMemory) Node{threadId}); auto lock = bucket.lock.lock_exclusive(); + + // Check again if an entry was created by another thread while we were waiting for the exclusive lock + for (auto pNode = bucket.head.get(); pNode != nullptr; pNode = pNode->pNext.get()) + { + if (pNode->threadId == threadId) + { + return &pNode->value; + } + } + + // No duplicate entry, safe to insert newNode->pNext = wistd::move(bucket.head); bucket.head = wistd::move(newNode); return &bucket.head->value; From e426334b3bba0374097a1e5f1c7d70d944ea0c45 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sun, 4 Jan 2026 00:42:49 +0000 Subject: [PATCH 4/6] Complete implementation - code review and security analysis passed Co-authored-by: jonwis <18537118+jonwis@users.noreply.github.com> --- _codeql_detected_source_root | 1 + 1 file changed, 1 insertion(+) create mode 120000 _codeql_detected_source_root diff --git a/_codeql_detected_source_root b/_codeql_detected_source_root new file mode 120000 index 00000000..945c9b46 --- /dev/null +++ b/_codeql_detected_source_root @@ -0,0 +1 @@ +. \ No newline at end of file From 55a6a55427dd42baa54989a3bab33ce49a370577 Mon Sep 17 00:00:00 2001 From: Jon Wiswall Date: Thu, 8 Jan 2026 11:00:52 -0800 Subject: [PATCH 5/6] Go back to stone age memory management (and bring in cmake fixes) --- CMakePresets.json | 29 ++++++++--------- include/wil/result.h | 74 ++++++++++++++++++++++++-------------------- 2 files changed, 55 insertions(+), 48 deletions(-) diff --git a/CMakePresets.json b/CMakePresets.json index 61f23932..836f5905 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -14,6 +14,7 @@ "installDir": "${sourceDir}/build/install/${presetName}", "toolchainFile": "$env{VCPKG_ROOT}/scripts/buildsystems/vcpkg.cmake", "cacheVariables": { + "WIL_ENABLE_ASAN": true, "CMAKE_CONFIGURATION_TYPES": "Debug;RelWithDebInfo;Release;MinSizeRel", "CMAKE_CXX_COMPILER": "cl", "CMAKE_C_COMPILER": "cl" @@ -29,9 +30,19 @@ } }, "cacheVariables": { + "WIL_ENABLE_ASAN": false, "CMAKE_CXX_COMPILER": "clang-cl", "CMAKE_C_COMPILER": "clang-cl" } + }, + { + "name": "clang-release", + "inherits": "clang", + "hidden": false, + "cacheVariables": { + "WIL_ENABLE_ASAN": true, + "WIL_ENABLE_UBSAN": true + } } ], "buildPresets": [ @@ -39,19 +50,13 @@ "name": "msvc-debug", "displayName": "MSVC Debug", "configurePreset": "msvc", - "configuration": "Debug", - "cacheVariables": { - "WIL_ENABLE_ASAN": true - } + "configuration": "Debug" }, { "name": "msvc-release", "displayName": "MSVC Release (debuggable)", "configurePreset": "msvc", - "configuration": "RelWithDebInfo", - "cacheVariables": { - "WIL_ENABLE_ASAN": true - } + "configuration": "RelWithDebInfo" }, { "name": "clang-debug", @@ -62,12 +67,8 @@ { "name": "clang-release", "displayName": "clang Release (debuggable)", - "configurePreset": "clang", - "configuration": "RelWithDebInfo", - "cacheVariables": { - "WIL_ENABLE_ASAN": true, - "WIL_ENABLE_UBSAN": true - } + "configurePreset": "clang-release", + "configuration": "RelWithDebInfo" } ], "testPresets": [ diff --git a/include/wil/result.h b/include/wil/result.h index 7a940996..cf6435b5 100644 --- a/include/wil/result.h +++ b/include/wil/result.h @@ -375,16 +375,39 @@ namespace details_abi template class ThreadLocalStorage { + struct Node + { + Node* next{nullptr}; + DWORD threadId = 0xffffffffU; + T value{}; + }; + + struct Bucket + { + wil::srwlock lock; + Node* head{nullptr}; + + ~Bucket() WI_NOEXCEPT + { + // Cleanup in a loop rather than recursively + while (head) + { + auto tmp = head; + head = tmp->next; + tmp->~Node(); + details::FreeProcessHeap(tmp); + } + } + }; + + Bucket m_hashArray[13]{}; + public: ThreadLocalStorage(const ThreadLocalStorage&) = delete; ThreadLocalStorage& operator=(const ThreadLocalStorage&) = delete; ThreadLocalStorage() = default; - - ~ThreadLocalStorage() WI_NOEXCEPT - { - // Cleanup is automatic via unique_process_heap_ptr destructors - } + ~ThreadLocalStorage() WI_NOEXCEPT = default; // Note: Can return nullptr even when (shouldAllocate == true) upon allocation failure T* GetLocal(bool shouldAllocate = false) WI_NOEXCEPT @@ -399,7 +422,7 @@ namespace details_abi // Lock the bucket and search for an existing entry { auto lock = bucket.lock.lock_shared(); - for (auto pNode = bucket.head.get(); pNode != nullptr; pNode = pNode->pNext.get()) + for (auto pNode = bucket.head; pNode != nullptr; pNode = pNode->next) { if (pNode->threadId == threadId) { @@ -414,46 +437,29 @@ namespace details_abi } // No entry for us, make a new one and insert it at the head - wil::unique_process_heap_ptr newNode; - void* newMemory = details::ProcessHeapAlloc(0, sizeof(Node)); - if (!newMemory) + void* newNodeStore = details::ProcessHeapAlloc(0, sizeof(Node)); + if (!newNodeStore) { return nullptr; } - newNode.reset(new (newMemory) Node{threadId}); + auto node = new (newNodeStore) Node{nullptr, threadId}; - auto lock = bucket.lock.lock_exclusive(); - - // Check again if an entry was created by another thread while we were waiting for the exclusive lock - for (auto pNode = bucket.head.get(); pNode != nullptr; pNode = pNode->pNext.get()) + // Look again and insert the new node + auto lock = bucket.lock.lock_exclusive(); + for (auto pNode = bucket.head; pNode != nullptr; pNode = pNode->next) { if (pNode->threadId == threadId) { + node->~Node(); + details::FreeProcessHeap(node); return &pNode->value; } } - - // No duplicate entry, safe to insert - newNode->pNext = wistd::move(bucket.head); - bucket.head = wistd::move(newNode); + + node->next = bucket.head; + bucket.head = node; return &bucket.head->value; } - - private: - struct Node - { - DWORD threadId = 0xffffffffU; - wil::unique_process_heap_ptr pNext; - T value{}; - }; - - struct Bucket - { - wil::srwlock lock; - wil::unique_process_heap_ptr head; - }; - - Bucket m_hashArray[10]{}; }; struct ThreadLocalFailureInfo From 7f07b5baa11a74710ea4b68b61084391c45f9417 Mon Sep 17 00:00:00 2001 From: Jon Wiswall Date: Thu, 8 Jan 2026 13:27:51 -0800 Subject: [PATCH 6/6] Formatting changes --- include/wil/result.h | 4 ++-- tests/ResultTests.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/wil/result.h b/include/wil/result.h index cf6435b5..15ee33c8 100644 --- a/include/wil/result.h +++ b/include/wil/result.h @@ -445,7 +445,7 @@ namespace details_abi auto node = new (newNodeStore) Node{nullptr, threadId}; // Look again and insert the new node - auto lock = bucket.lock.lock_exclusive(); + auto lock = bucket.lock.lock_exclusive(); for (auto pNode = bucket.head; pNode != nullptr; pNode = pNode->next) { if (pNode->threadId == threadId) @@ -457,7 +457,7 @@ namespace details_abi } node->next = bucket.head; - bucket.head = node; + bucket.head = node; return &bucket.head->value; } }; diff --git a/tests/ResultTests.cpp b/tests/ResultTests.cpp index 7f701536..ed10c83d 100644 --- a/tests/ResultTests.cpp +++ b/tests/ResultTests.cpp @@ -782,7 +782,7 @@ TEST_CASE("ResultTests::ThreadLocalStorage", "[result]") { constexpr int NUM_THREADS = 10; constexpr int ITERATIONS = 1000; - + wil::details_abi::ThreadLocalStorage storage; std::atomic errors{0}; std::vector threads;