fix(memory): harden read_ptr_unsafe under ASan, stabilize concurrent guarded-read tests

AGENTS.md

@@ -333,7 +333,7 @@ The raw `Scanner::find_pattern(start_address, region_size, pattern)` overloads d
 - **Drift manifest tests:** `tests/test_drift_manifest.cpp` covers `drift_manifest.hpp`: round-trip of a DriftEntry report through serialize/parse (including negative offsets and the owned-name-survives-source-destruction guarantee), the empty-report header-only case, file round-trip, and the fail-closed parse errors (missing header, wrong field count, non-numeric offset, missing file).
 - **Diagnostics tests:** `tests/test_diagnostics.cpp` covers `diagnostics.hpp` (the per-subsystem intentional-leak counters): per-subsystem increment isolation, accumulation, the cross-subsystem total, the out-of-range `LeakSubsystem::Count` no-op, and reset. The instrumented loader-lock teardown sites themselves are not reachable from a normal test run; the counter contract is verified directly.
 - **Coverage gate:** 80% minimum line coverage enforced in CI. All PRs must pass.
-- **Concurrency tests:** Use `std::atomic<bool> stop` flag pattern with multiple threads. See `AsyncMode_ConcurrentLogAndDisable` in `test_logger.cpp` for the reference pattern.
+- **Concurrency tests:** Use `std::atomic<bool> stop` flag pattern with multiple threads. See `AsyncMode_ConcurrentLogAndDisable` in `test_logger.cpp` for the reference pattern. When a concurrent test needs a *non-readable* address (to prove a guarded read faults to 0), hold a committed `PAGE_NOACCESS` page until teardown -- never a `MEM_RELEASE`d region. A released VA can be recycled and remapped by the allocations that spawning threads triggers (stacks/TEBs, more so under AddressSanitizer), so a "must fault" read can land on live memory and flake.
 - **Build flag:** Tests are enabled with `DMK_BUILD_TESTS=ON` (on by default in debug presets).
 
 For detailed coverage analysis, see [docs/tests/README.md](docs/tests/README.md). For hot-reload testing patterns, see [docs/hot-reload/README.md](docs/hot-reload/README.md). For INI hot-reload (filesystem watcher and reload hotkey), see [docs/config-hot-reload/README.md](docs/config-hot-reload/README.md). For AOB signature construction, the Scanner API, and RIP-relative resolution, see [docs/misc/aob-signatures.md](docs/misc/aob-signatures.md). For the MSVC RTTI walker and the typed SEH read primitives it builds on, see [docs/misc/rtti-walker.md](docs/misc/rtti-walker.md). For the reverse RTTI dissector and the self-healing offset resolver built on the same prelude, see [docs/misc/rtti-self-heal.md](docs/misc/rtti-self-heal.md). For the declarative anchor registry that unifies the vtable / cascade / code-constant / manual backends, see [docs/misc/anchors.md](docs/misc/anchors.md).

src/memory.cpp

@@ -1679,34 +1679,17 @@ Memory::ReadableStatus DetourModKit::Memory::is_readable_nonblocking(const void
 
 uintptr_t DetourModKit::Memory::read_ptr_unsafe(uintptr_t base, ptrdiff_t offset) noexcept
 {
-#ifdef _MSC_VER
-    __try
-    {
-        // memcpy, not a *reinterpret_cast deref: the foreign address may be misaligned, and a cast-deref of a
-        // misaligned pointer is formal undefined behavior. memcpy has identical codegen here (a single mov on x86-64)
-        // and matches the idiom seh_read_bytes already uses for the same reason.
-        uintptr_t value;
-        std::memcpy(&value, reinterpret_cast<const void *>(base + offset), sizeof(value));
-        return value;
-    }
-    __except (Memory::detail::is_guarded_read_fault(GetExceptionCode()) ? EXCEPTION_EXECUTE_HANDLER
-                                                                        : EXCEPTION_CONTINUE_SEARCH)
-    {
-        return 0;
-    }
-#else
-    // MinGW/GCC lacks __try/__except. Read the pointer-sized value through the process-wide vectored fault guard (see
-    // veh_read_bytes): a fault -- unmapped, PAGE_NOACCESS/guard, or a page reprotected out from under a stale cache
-    // entry -- is swallowed and reported as failure. This matches the MSVC path's "0 on fault" contract without a
-    // per-call VirtualQuery and without trusting cached protection data for an unguarded dereference. memcpy semantics
-    // are preserved (the guarded copy is byte-wise), so a misaligned foreign pointer is read without the undefined
-    // behavior of dereferencing it.
-    const uintptr_t src = base + static_cast<uintptr_t>(offset);
+    // Route the pointer-sized read through the shared guarded-read entry point rather than a private __try/memcpy so a
+    // single implementation owns every safety property: the low/null source floor, the (src + size) wrap rejection, and
+    // -- under AddressSanitizer on MSVC -- the __movsb copy that seh_read_bytes uses because the compiler otherwise
+    // routes std::memcpy through the ASan interceptor, which both false-positives on the foreign mapped memory these
+    // probes legitimately read and aborts on a wrapping source range before the SEH guard can turn the fault into a 0
+    // return. seh_read_bytes' byte-wise copy also keeps a misaligned foreign pointer free of cast-deref undefined
+    // behavior. A rejected or faulting read yields 0; on a hit, value holds the bytes.
     uintptr_t value = 0;
-    if (!veh_read_bytes(src, &value, sizeof(value)))
+    if (!seh_read_bytes(base + static_cast<uintptr_t>(offset), &value, sizeof(value)))
         return 0;
     return value;
-#endif
 }
 
 bool DetourModKit::Memory::seh_read_bytes(uintptr_t addr, void *out, size_t bytes) noexcept

tests/test_memory.cpp

@@ -2350,11 +2350,14 @@ TEST_F(MemoryTest, GuardedReadsAreThreadIsolatedUnderConcurrency)
     constexpr uintptr_t kSentinel = 0x5151515151515151ULL;
     *reinterpret_cast<uintptr_t *>(good) = kSentinel;
 
-    void *freed = VirtualAlloc(nullptr, 4096, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
-    ASSERT_NE(freed, nullptr);
-
-    // Now unmapped; reads of it must fault through the guard to 0.
-    VirtualFree(freed, 0, MEM_RELEASE);
+    // A deliberately non-readable page the test owns until teardown. This used to MEM_RELEASE a PAGE_READWRITE region
+    // to get an unmapped address, but a released VA can be recycled and remapped by the allocations that spawning the
+    // reader threads triggers (thread stacks/TEBs, more so under AddressSanitizer); a read then lands on live memory
+    // instead of faulting, so the "must fault to 0" assertion flaked on about one read in many thousands. A committed
+    // PAGE_NOACCESS page the test holds faults deterministically and cannot be recycled, so it exercises the guard's
+    // per-thread fault isolation without that artifact.
+    void *unreadable = VirtualAlloc(nullptr, 4096, MEM_COMMIT | MEM_RESERVE, PAGE_NOACCESS);
+    ASSERT_NE(unreadable, nullptr);
 
     std::atomic<bool> all_correct{true};
     std::vector<std::thread> threads;
@@ -2370,7 +2373,7 @@ TEST_F(MemoryTest, GuardedReadsAreThreadIsolatedUnderConcurrency)
                         if (Memory::read_ptr_unsafe(reinterpret_cast<uintptr_t>(good), 0) != kSentinel)
                             all_correct.store(false, std::memory_order_relaxed);
                     }
-                    else if (Memory::read_ptr_unsafe(reinterpret_cast<uintptr_t>(freed), 0) != 0u)
+                    else if (Memory::read_ptr_unsafe(reinterpret_cast<uintptr_t>(unreadable), 0) != 0u)
                     {
                         all_correct.store(false, std::memory_order_relaxed);
                     }
@@ -2381,6 +2384,7 @@ TEST_F(MemoryTest, GuardedReadsAreThreadIsolatedUnderConcurrency)
         th.join();
 
     EXPECT_TRUE(all_correct.load());
+    VirtualFree(unreadable, 0, MEM_RELEASE);
     VirtualFree(good, 0, MEM_RELEASE);
 }
 
@@ -2395,9 +2399,11 @@ TEST_F(MemoryTest, GuardedReadsSurviveConcurrentShutdown)
     constexpr uintptr_t kSentinel = 0xA5A5A5A5A5A5A5A5ULL;
     *reinterpret_cast<uintptr_t *>(good) = kSentinel;
 
-    void *freed = VirtualAlloc(nullptr, 4096, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE);
-    ASSERT_NE(freed, nullptr);
-    VirtualFree(freed, 0, MEM_RELEASE);
+    // A deliberately non-readable page the test owns until teardown: a committed PAGE_NOACCESS page faults on every
+    // read and cannot be recycled, unlike a MEM_RELEASE'd VA, which the allocations from spawning the reader threads
+    // can remap so a "faulting" read lands on live memory.
+    void *unreadable = VirtualAlloc(nullptr, 4096, MEM_COMMIT | MEM_RESERVE, PAGE_NOACCESS);
+    ASSERT_NE(unreadable, nullptr);
 
     std::atomic<bool> stop{false};
     std::atomic<int> seen_good{0};
@@ -2408,7 +2414,7 @@ TEST_F(MemoryTest, GuardedReadsSurviveConcurrentShutdown)
         readers.emplace_back(
             [&, t]()
             {
-                void *const target = (t % 2) ? freed : good;
+                void *const target = (t % 2) ? unreadable : good;
                 while (!stop.load(std::memory_order_acquire))
                 {
                     const uintptr_t v = Memory::read_ptr_unsafe(reinterpret_cast<uintptr_t>(target), 0);
@@ -2442,6 +2448,7 @@ TEST_F(MemoryTest, GuardedReadsSurviveConcurrentShutdown)
         th.join();
 
     EXPECT_EQ(Memory::read_ptr_unsafe(reinterpret_cast<uintptr_t>(good), 0), kSentinel);
+    VirtualFree(unreadable, 0, MEM_RELEASE);
     VirtualFree(good, 0, MEM_RELEASE);
 }