fix(memory,scanner): guard region reads against TOCTOU faults (#113)

Author: tkhquang

AGENTS.md

@@ -301,7 +301,7 @@ Do not add `Memory::is_readable()` or `Memory::is_writable()` before every field
 
 ### Scanning process memory
 
-The raw `Scanner::find_pattern(start_address, region_size, pattern)` overloads do no page filtering: they read the whole span with `memchr`/SIMD, so the caller must guarantee `[start_address, start_address + region_size)` is committed and readable, or the host faults. Use them only on byte buffers or module sections whose readability is already known. To scan arbitrary process or module memory, prefer the page-filtered helpers (`scan_executable_regions`, `scan_readable_regions`), which walk `VirtualQuery` and skip guard, no-access, and non-readable pages.
+The raw `Scanner::find_pattern(start_address, region_size, pattern)` overloads do no page filtering: they read the whole span with `memchr`/SIMD, so the caller must guarantee `[start_address, start_address + region_size)` is committed and readable, or the host faults. Use them only on byte buffers or module sections whose readability is already known. To scan arbitrary process or module memory, prefer the page-filtered helpers (`scan_executable_regions`, `scan_readable_regions`), which walk `VirtualQuery` and skip guard, no-access, and non-readable pages. The per-region `VirtualQuery` gate proves readability only at gate time; on MSVC each region read additionally runs inside a structured-exception guard, so a region decommitted or reprotected concurrently between the gate and the read is skipped (and counted at `Debug`) rather than faulting the host. On MinGW the gate is the only guard, so that narrow TOCTOU window can still fault until the VEH-based handler lands. The same guard wraps the per-window reads behind `find_string_xref`.
 
 ## Testing
 
@@ -380,7 +380,7 @@ These are called at 60+ fps from game hook callbacks. Never add allocations, exc
 - `Memory::is_readable_nonblocking()` -- try_lock_shared + cache lookup (returns Unknown on contention)
 - `Memory::read_ptr_unsafe()` -- SEH-protected raw dereference (MSVC), cache-accelerated with VirtualQuery fallback (MinGW)
 - `Memory::read_ptr_unchecked()` -- inline pointer dereference with source and result user-mode range guards (a low-address floor plus a `USERSPACE_PTR_MAX` ceiling that rejects kernel-range and non-canonical values, which also subsumes pointer-arithmetic wraparound), no SEH (caller must guarantee structural pointer validity); debug builds add an `is_readable` assert that catches a stale or unmapped source pointer, compiled out in release so the hot path stays a single guarded memcpy
-- `Memory::seh_read<T>()` / `seh_read_bytes()` -- typed and raw SEH-guarded reads; single `__try` frame on MSVC, VirtualQuery loop across regions on MinGW. Used by `Rtti` for chained RTTI walks
+- `Memory::seh_read<T>()` / `seh_read_bytes()` -- typed and raw SEH-guarded reads; single `__try` frame on MSVC, VirtualQuery loop across regions on MinGW. The MSVC `__except` filter swallows the foreign-read fault set -- `EXCEPTION_ACCESS_VIOLATION`, `STATUS_GUARD_PAGE_VIOLATION`, and `EXCEPTION_IN_PAGE_ERROR` (a file-backed or image-mapped page failing to page in, e.g. during an RTTI / section walk) -- and lets any other code continue the handler search; the predicate (`Memory::detail::is_guarded_read_fault`) is shared by every guarded read so the set stays uniform. Used by `Rtti` for chained RTTI walks
 - `Memory::seh_resolve_chain()` / `seh_read_chain<T>()` -- resolves or reads a whole multi-level pointer chain under one fault guard: one out-of-line call instead of N separate `seh_read` calls, with each intermediate link kept in a register and pre-screened by `plausible_userspace_ptr` (a faulting or implausible link aborts the walk and returns nullopt/false). VirtualQuery-guarded per link on MinGW
 - `Memory::plausible_userspace_ptr(p)` -- `inline constexpr` user-mode pointer plausibility test; pure arithmetic with no syscall and no memory access (early-rejects stale/sentinel/torn pointers before an SEH-guarded read)
 - `Memory::contains(range, p)` -- constexpr point-in-range test for module bounds checks

README.md

@@ -41,7 +41,7 @@ DetourModKit is a full-featured C++23 toolkit designed to simplify common tasks
 - `|` offset markers for targeting a specific instruction within a wider pattern (e.g., `"48 8B 88 B8 00 00 00 | 48 89 4C 24 68"` sets the offset to byte 7)
 - Nth-occurrence matching (1-based) for patterns that hit multiple locations
 - RIP-relative instruction resolution for extracting absolute addresses from x86-64 code (returns `std::expected` with typed `RipResolveError` for actionable diagnostics)
-- `scan_executable_regions()` for scanning all committed executable pages in the process - useful for games with packed or protected binaries that unpack code into anonymous memory outside any loaded module (pure-execute pages without a read bit are skipped to avoid access violations)
+- `scan_executable_regions()` for scanning all committed executable pages in the process - useful for games with packed or protected binaries that unpack code into anonymous memory outside any loaded module (pure-execute pages without a read bit are skipped to avoid access violations; a region decommitted or reprotected concurrently mid-sweep is skipped rather than faulting the host on MSVC, where each region read runs inside a structured-exception guard)
 - `scan_readable_regions()` -- the data-section sibling of `scan_executable_regions()` -- sweeps every committed readable page (`.rdata` / `.data`, read-only heaps) to reach C++ vtables, RTTI type descriptors, and read-only metadata the executable-only sweep cannot see (guard / no-access / uncommitted pages are skipped); opt a cascade into it with `resolve_cascade(..., ScannerKind::Readable)`
 - `resolve_cascade()` and the module-scoped `resolve_cascade_in_module()` -- ordered multi-candidate resolution (try signatures in priority order, return the first that resolves), with an optional hooked-prologue recovery pass; the `_in_module` variants confine the scan to one mapped image `[base, end)` and reject out-of-module resolutions, so a generic signature that also matches inside another injected module (a graphics overlay, a sibling mod) cannot shadow the correct in-module target. By default each candidate must match uniquely in the scanned scope: an ambiguous signature (more than one match) falls through to the next candidate instead of silently committing to an arbitrary match, so a too-loose pattern surfaces as a clean failure to fix rather than a wrong hook. Set `require_unique = false` per candidate only to opt out a deliberately non-unique, separately-verified candidate
 - `resolve_cascade_in_host_module()` / `resolve_cascade_in_host_module_with_prologue_fallback()` -- one-line convenience overloads that scope a cascade to the host EXE (`host_module_range()`), removing the boilerplate of building the range at every call site. They return `ResolveError::InvalidRange` if the host range cannot be determined. Use them only when the target lives in the host EXE; for a game whose logic is in a separate module (an engine DLL), resolve that module's range and call `resolve_cascade_in_module()` instead

include/DetourModKit/memory.hpp

@@ -342,12 +342,12 @@ namespace DetourModKit
 
         /**
          * @brief SEH-guarded raw memory copy from @p addr into @p out.
-         * @details On MSVC, the copy runs inside a __try / __except
-         *          (EXCEPTION_ACCESS_VIOLATION | STATUS_GUARD_PAGE_VIOLATION)
-         *          frame, so any access violation in the middle of the copy unwinds cleanly and the function returns
-         *          false. On MinGW the implementation falls back to VirtualQuery-based validation of every region the
-         *          read spans before issuing the memcpy; this is race-prone against concurrent
-         *          VirtualProtect but is the best a non-SEH toolchain can do.
+         * @details On MSVC, the copy runs inside a __try / __except frame whose filter accepts the foreign-read fault
+         *          set (EXCEPTION_ACCESS_VIOLATION, STATUS_GUARD_PAGE_VIOLATION, and EXCEPTION_IN_PAGE_ERROR for a
+         *          file-backed page that fails to page in), so any such fault in the middle of the copy unwinds cleanly
+         *          and the function returns false. On MinGW the implementation falls back to VirtualQuery-based
+         *          validation of every region the read spans before issuing the memcpy; this is race-prone against
+         *          concurrent VirtualProtect but is the best a non-SEH toolchain can do.
          *
          *          The function is the underlying primitive for the typed @ref seh_read template and is exposed
          *          directly for callers that need to read a contiguous buffer of bytes (for example NUL-terminated

src/memory.cpp

@@ -15,6 +15,7 @@
 #include "DetourModKit/logger.hpp"
 #include "DetourModKit/srw_shared_mutex.hpp"
 #include "platform.hpp"
+#include "memory_internal.hpp"
 
 #include <windows.h>
 #if defined(_MSC_VER) && defined(__SANITIZE_ADDRESS__)
@@ -668,8 +669,10 @@ namespace
 
         constexpr size_t MAX_INVALIDATION_RETRIES = 3;
 
+        size_t skipped_shards = 0;
         for (size_t shard_idx = 0; shard_idx < shard_count; ++shard_idx)
         {
+            bool invalidated = false;
             for (size_t retry = 0; retry < MAX_INVALIDATION_RETRIES; ++retry)
             {
                 std::unique_lock<SrwSharedMutex> lock(*s_shard_mutexes[shard_idx], std::try_to_lock);
@@ -684,8 +687,25 @@ namespace
                 }
 
                 evict_overlapping_entries_in_shard(s_cache_shards[shard_idx], address, end_address);
+                invalidated = true;
                 break;
             }
+            if (!invalidated)
+            {
+                ++skipped_shards;
+            }
+        }
+
+        if (skipped_shards > 0)
+        {
+            // A shard that stayed contended across every retry keeps its entries in place, so they answer from the
+            // pre-write protection state until the configured expiry sweeps them. This matters chiefly when the caller
+            // could not restore protection (write_bytes restores on success), so surface it for diagnosis instead of
+            // skipping silently. try_log keeps this noexcept path honest when the level is enabled.
+            (void)Logger::get_instance().try_log(LogLevel::Debug,
+                                                 "MemoryCache: invalidate_range left {} contended shard(s) unswept; "
+                                                 "stale entries persist until expiry.",
+                                                 skipped_shards);
         }
     }
 
@@ -1114,13 +1134,24 @@ namespace
         if (!address || size == 0)
             return false;
 
-        // Construct reader guard BEFORE checking s_cache_initialized to prevent shutdown_cache from destroying data
+        // Construct reader guard BEFORE loading s_cache_initialized to prevent shutdown_cache from destroying data
         // structures between the check and access.
         ActiveReaderGuard reader_guard;
 
-        if (!s_cache_initialized.load(std::memory_order_seq_cst))
+        // Snapshot cache readiness once, under the guard. The guard's seq_cst increment is ordered before this seq_cst
+        // load of s_cache_initialized (the Dekker protocol that lets shutdown_cache drain readers safely). shard_count
+        // is loaded only when initialized, so the cache path below operates on a single consistent value.
+        const bool cache_initialized = s_cache_initialized.load(std::memory_order_seq_cst);
+        const size_t shard_count = cache_initialized ? s_shard_count.load(std::memory_order_acquire) : 0;
+
+        // Fall back to a direct VirtualQuery whenever the cache is unavailable: never initialized, observed in the
+        // brief init publication window where s_cache_initialized is already true but s_shard_count is still 0
+        // (init_cache publishes the flag before perform_cache_initialization stores the count), or a concurrent
+        // shutdown that has already zeroed the count. Treating shard_count == 0 as "use the direct query" -- matching
+        // read_ptr_unsafe and invalidate_range -- avoids wrongly reporting a readable region as non-readable during
+        // that window.
+        if (shard_count == 0)
         {
-            // Cache not initialized -- fall back to direct VirtualQuery
             MEMORY_BASIC_INFORMATION mbi;
             if (!VirtualQuery(address, &mbi, sizeof(mbi)))
                 return false;
@@ -1136,12 +1167,7 @@ namespace
             return query_addr_val >= region_start && query_end <= region_start + mbi.RegionSize;
         }
 
-        // Reader guard already active -- safe to access cache data structures
-
-        const size_t shard_count = s_shard_count.load(std::memory_order_acquire);
-        if (shard_count == 0)
-            return false;
-
+        // Cache is live and shard_count is non-zero -- the reader guard keeps the shard vectors alive for the access.
         const uintptr_t query_addr_val = reinterpret_cast<uintptr_t>(address);
         const size_t shard_idx = compute_shard_index(query_addr_val, shard_count);
         const uint64_t now_ns = current_time_ns();
@@ -1325,11 +1351,15 @@ uintptr_t DetourModKit::Memory::read_ptr_unsafe(uintptr_t base, ptrdiff_t offset
 #ifdef _MSC_VER
     __try
     {
-        return *reinterpret_cast<const uintptr_t *>(base + offset);
+        // 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 ((GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION || GetExceptionCode() == STATUS_GUARD_PAGE_VIOLATION)
-                  ? EXCEPTION_EXECUTE_HANDLER
-                  : EXCEPTION_CONTINUE_SEARCH)
+    __except (Memory::detail::is_guarded_read_fault(GetExceptionCode()) ? EXCEPTION_EXECUTE_HANDLER
+                                                                        : EXCEPTION_CONTINUE_SEARCH)
     {
         return 0;
     }
@@ -1358,7 +1388,12 @@ uintptr_t DetourModKit::Memory::read_ptr_unsafe(uintptr_t base, ptrdiff_t offset
                     if (cached)
                     {
                         if (check_read_permission(cached->protection))
-                            return *reinterpret_cast<const uintptr_t *>(src);
+                        {
+                            // memcpy for the same misalignment reason as the MSVC path above.
+                            uintptr_t value;
+                            std::memcpy(&value, reinterpret_cast<const void *>(src), sizeof(value));
+                            return value;
+                        }
                         return 0;
                     }
                 }
@@ -1381,7 +1416,10 @@ uintptr_t DetourModKit::Memory::read_ptr_unsafe(uintptr_t base, ptrdiff_t offset
     const uintptr_t read_end = src + sizeof(uintptr_t);
     if (read_end < src || src < region_start || read_end > region_end)
         return 0;
-    return *reinterpret_cast<const uintptr_t *>(src);
+    // memcpy for the same misalignment reason as the MSVC path above.
+    uintptr_t value;
+    std::memcpy(&value, reinterpret_cast<const void *>(src), sizeof(value));
+    return value;
 #endif
 }
 
@@ -1418,9 +1456,8 @@ bool DetourModKit::Memory::seh_read_bytes(uintptr_t addr, void *out, size_t byte
 #endif
         return true;
     }
-    __except ((GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION || GetExceptionCode() == STATUS_GUARD_PAGE_VIOLATION)
-                  ? EXCEPTION_EXECUTE_HANDLER
-                  : EXCEPTION_CONTINUE_SEARCH)
+    __except (Memory::detail::is_guarded_read_fault(GetExceptionCode()) ? EXCEPTION_EXECUTE_HANDLER
+                                                                        : EXCEPTION_CONTINUE_SEARCH)
     {
         return false;
     }
@@ -1482,10 +1519,8 @@ namespace
             out_addr = (count == 0) ? cur : cur + static_cast<uintptr_t>(offsets[count - 1]);
             return true;
         }
-        __except (
-            (GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION || GetExceptionCode() == STATUS_GUARD_PAGE_VIOLATION)
-                ? EXCEPTION_EXECUTE_HANDLER
-                : EXCEPTION_CONTINUE_SEARCH)
+        __except (Memory::detail::is_guarded_read_fault(GetExceptionCode()) ? EXCEPTION_EXECUTE_HANDLER
+                                                                            : EXCEPTION_CONTINUE_SEARCH)
         {
             return false;
         }
@@ -1550,9 +1585,8 @@ bool DetourModKit::Memory::seh_read_chain_bytes(uintptr_t base, std::span<const
         std::memcpy(out, reinterpret_cast<const void *>(final_addr), bytes);
         return true;
     }
-    __except ((GetExceptionCode() == EXCEPTION_ACCESS_VIOLATION || GetExceptionCode() == STATUS_GUARD_PAGE_VIOLATION)
-                  ? EXCEPTION_EXECUTE_HANDLER
-                  : EXCEPTION_CONTINUE_SEARCH)
+    __except (Memory::detail::is_guarded_read_fault(GetExceptionCode()) ? EXCEPTION_EXECUTE_HANDLER
+                                                                        : EXCEPTION_CONTINUE_SEARCH)
     {
         return false;
     }