From dab5573a9270e834022575afaddb6cecef597667 Mon Sep 17 00:00:00 2001 From: Quang Trinh Date: Fri, 12 Jun 2026 00:56:14 +0700 Subject: [PATCH 1/2] fix: back reader/writer locks with a shared SRWLOCK wrapper --- AGENTS.md | 10 +- README.md | 1 + include/DetourModKit/hook_manager.hpp | 105 ++++++++++-------- include/DetourModKit/input.hpp | 12 +- include/DetourModKit/srw_shared_mutex.hpp | 61 +++++++++++ src/hook_manager.cpp | 80 +++++++------- src/input.cpp | 21 ++-- src/memory.cpp | 36 +----- src/srw_shared_mutex.cpp | 69 ++++++++++++ tests/test_hook_integration.cpp | 4 +- tests/test_input.cpp | 77 +++++++++++++ tests/test_srw_shared_mutex.cpp | 127 ++++++++++++++++++++++ 12 files changed, 467 insertions(+), 136 deletions(-) create mode 100644 include/DetourModKit/srw_shared_mutex.hpp create mode 100644 src/srw_shared_mutex.cpp create mode 100644 tests/test_srw_shared_mutex.cpp diff --git a/AGENTS.md b/AGENTS.md index fe20ac5..d113de8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -133,6 +133,7 @@ include/DetourModKit/ # Public headers -- one per module config_watcher.hpp # Filesystem watcher (ReadDirectoryChangesW) for INI hot-reload input.hpp # Input polling (keyboard/mouse/XInput) + opt-in mouse-wheel capture and gamepad passthrough suppression input_codes.hpp # Unified InputCode type and named key tables (incl. WheelUp/Down/Left/Right) + srw_shared_mutex.hpp # Opaque Windows SRWLOCK SharedLockable wrapper; use for reader/writer locks that need native Windows semantics without exposing Windows headers memory.hpp # Memory read/write, sharded region cache, seh_read, seh_resolve_chain/seh_read_chain, plausible_userspace_ptr, PE module range rtti.hpp # MSVC RTTI walker (type_name_of, vtable_is_type, find_in_pointer_table) + reverse name-to-vtable (vtable_for_type, vtables_for_type, TypeIdentity) rtti_dissect.hpp # Reverse RTTI dissection + self-healing offsets (identify_pointee_type, reverse_scan_block, heal_landmark/heal_offset, solve_fingerprint) + drift-telemetry report (heal_report, DriftEntry) @@ -310,6 +311,7 @@ The raw `Scanner::find_pattern(start_address, region_size, pattern)` overloads d - **Decoder tests:** `tests/test_x86_decode.cpp` tests the internal header `src/x86_decode.hpp` (RIP-relative E9 / EB / FF25 resolvers consumed by `Scanner`). The test file adds `src/` to its include path and drives each decoder with hand-crafted byte buffers. - **Input interception tests:** `tests/test_input_intercept.cpp` tests the internal header `src/input_intercept.hpp` (same `src/`-on-include-path pattern as the decoder tests). It covers the two pure state machines that back the active-input layer: the wheel-pulse stepper (one notch maps to one Press edge with a forced low cycle) and the gamepad consume-until-release latch (modifier-released-before-trigger does not leak, release grace expiry, re-press during grace). The live hooks are exercised by integration tests against a throwaway top-level window the test process creates: the window-procedure subclass install/uninstall, per-direction wheel-notch capture, the consume-swallow versus forward decision (observed through a recording predecessor procedure), and the `WM_NCDESTROY` self-heal that lets a recreated window be re-subclassed. The XInput inline hook is exercised against a loaded `xinput` runtime (install, trampoline publish, idempotent re-install, teardown), and a standalone `InputPoller` test drives the full hot-reload disarm path (a consume wheel binding arms the swallow flag; `clear_bindings(false)` lets the poll loop disarm it on a later cycle). These live-hook tests skip themselves when the host has no window station or no XInput runtime, so a headless runner stays green. The one path still validated manually is the gamepad button masking itself, which requires a physically connected controller. - **Worker tests:** `tests/test_worker.cpp` covers the `StoppableWorker` RAII `std::jthread` wrapper, including the empty-body early return, swallowed `std::exception` and unknown-exception paths, and idempotent `request_stop()` / `shutdown()`. The loader-lock detach arm is untestable from user code (only reached under DllMain) and is accepted as such. +- **SRW shared mutex tests:** `tests/test_srw_shared_mutex.cpp` covers the opaque Windows SRWLOCK SharedLockable wrapper used by DetourModKit reader/writer locks: shared-reader overlap, exclusive try-lock failure while readers are held, shared try-lock failure while a writer is held, a single-threaded try-lock/unlock sequence that proves both unlock paths actually release, and a blocked-writer handoff (a writer blocked in `lock()` cannot complete while a shared owner is live and acquires once the reader releases). File-scope static_asserts pin the deleted copy/move members (SRWLOCK owners and waiters reference the lock by address). - **Pointer-chain tests:** `tests/test_memory_chain.cpp` is a deliberate second suite for the public `memory.hpp` surface, kept separate from `test_memory.cpp` because the single-fault-frame pointer-chain primitives (`plausible_userspace_ptr`, `seh_resolve_chain`, `seh_read_chain`, `seh_read_chain_bytes`) walk in-process pointer chains and need no cache or game-memory state, whereas `test_memory.cpp` drives the sharded cache, read/write, and module-range paths. Both suites bind to the same header; this is the only same-module split and is intentional for state isolation. - **Reverse-RTTI / self-heal tests:** `tests/test_rtti_dissect.cpp` covers the `rtti_dissect.hpp` surface (L1 `identify_pointee_type` through L4 `solve_fingerprint`). It rebuilds the `SyntheticVtable` COL/TD/vtable fixture in the test exe's PE range (so the shared prelude's module bound-check accepts it) and adds a `syn_heap_object` helper that `VirtualAlloc`s an object outside every module range, exercising the pointer-to-object branch and the cross-DLL resolvability case. The suite replaces the throwing global `operator new`/`delete` with a malloc/free pair plus a counter so the `_AllocatesNothing` tests can assert the heal and fingerprint paths are allocation-free (each warms the module-range cache first, then measures a second call; the heal case is measured on the window-scan path, not just the nominal short-circuit); the aligned `operator new` forms are left at their defaults so over-aligned allocations never cross-free. This replacement is process-wide for the test binary but harmless to other suites (malloc/free is a valid implementation and only this suite reads the counter). - **Reverse-RTTI by-name tests:** `tests/test_rtti_reverse.cpp` covers `rtti.hpp`'s reverse resolvers (`vtable_for_type`, `vtables_for_type`, `TypeIdentity`). It builds synthetic COL/TypeDescriptor/vtable fixtures in the test exe's data segment (so the prelude's module bound-check accepts them) and resolves them through a tight pool range for speed, with one case driving the real PE-section walk via `host_module_range()`. It exercises single- and multiple-inheritance (one name -> many sub-object vtables), the `COL.offset == 0` primary selection, ambiguous-name fail-closed, and the warm-cache identity path. @@ -349,11 +351,11 @@ PATH="/c/msys64/mingw64/bin:$PATH" ./build/mingw-debug/tests/DetourModKit_tests. | Module | Thread safety | Hot-path mechanism | |--------|--------------|-------------------| | Scanner | Stateless -- inherently safe | N/A (startup only) | -| HookManager | `shared_mutex` (readers) / `unique_lock` (writers); two-phase shutdown (disable under shared lock, clear under exclusive lock); `m_mutator_gate` (shared_mutex) blocks new mutators (including all VMT operations) during teardown; CAS on `m_shutdown_called` serializes shutdown/remove_all_hooks; double-checked fast-fail on `m_shutdown_called` in all mutators; destructor fallback (when `DMK_Shutdown()` was not called) acquires `m_mutator_gate` exclusively, flips `m_shutdown_called`, drains readers via exclusive `m_hooks_mutex`, then clears the maps -- under loader lock it pins the module and swaps each map's contents into heap storage allocated via `new (std::nothrow)` so the storage outlives the destructor without ever draining, mirroring the leak-on-loader-lock discipline used in `Logger::shutdown_internal` and `ConfigWatcher::~ConfigWatcher` | `shared_lock` for `with_inline_hook()` | +| HookManager | SRWLOCK-backed reader/writer locks; two-phase shutdown (disable under shared lock, clear under exclusive lock); `m_mutator_gate` blocks new mutators (including all VMT operations) during teardown; CAS on `m_shutdown_called` serializes shutdown/remove_all_hooks; double-checked fast-fail on `m_shutdown_called` in all mutators; destructor fallback (when `DMK_Shutdown()` was not called) acquires `m_mutator_gate` exclusively, flips `m_shutdown_called`, drains readers via exclusive `m_hooks_mutex`, then clears the maps -- under loader lock it pins the module and swaps each map's contents into heap storage allocated via `new (std::nothrow)` so the storage outlives the destructor without ever draining, mirroring the leak-on-loader-lock discipline used in `Logger::shutdown_internal` and `ConfigWatcher::~ConfigWatcher` | `shared_lock` SRWLOCK reader for `with_inline_hook()` | | Logger | `atomic` for lock-free async reads; `shutdown_internal` is safe across repeated shutdown / enable_async_mode cycles: when the writer thread has to be detached under loader lock, the module is pinned and the `shared_ptr` is moved into a per-call heap cell allocated via `new (std::nothrow)` rather than appended to a static `std::vector`, so the leak path keeps the noexcept destructor honest under OOM (returns nullptr instead of throwing `bad_alloc`) and prior handles are never dropped while their writer threads may still be running | Single atomic load on log level check | | AsyncLogger | Lock-free MPMC queue (Vyukov-style); post-join drain on shutdown (at most one message per producer can be lost in the nanosecond race between drain and force-zero -- accepted trade-off to avoid atomic overhead on every enqueue); timestamp caching in write batches | Atomic sequence numbers per slot | | InputPoller | Atomic `m_active_states[]` array | `shared_lock` (uncontended SRWLOCK reader) guarding the `m_active_states` pointer swap + `memory_order_relaxed` load per binding | -| InputManager | `mutex` for lifecycle, `atomic` for reads | `atomic` acquire-load, then the poller's `shared_lock` + relaxed load (not lock-free) | +| InputManager | `mutex` for lifecycle, `atomic>` for reads | `atomic>` acquire-load, then the poller's `shared_lock` + relaxed load (not lock-free) | | InputIntercept (internal `src/input_intercept.*`) | File-scope atomics shared between the poll thread and the game's threads (XInput callers, window message thread); owns its safetyhook InlineHooks directly (not via HookManager) because the poll thread reads the trampoline and the hook lifetime is coupled to the poll thread; consume-until-release latch and wheel-pulse state are poll-thread-private; teardown skipped under loader lock (detours left installed against the pinned module) | Lock-free atomic loads in each detour; allocation-free, non-throwing detour bodies | | Memory cache | Sharded `SRWLOCK` + epoch-based shutdown | Shared reader locks per shard | | Config | `mutex` for registration; deferred setter invocation outside lock (no reentrancy guard needed -- setters may call back into Config); `reload()` re-runs the registered items against the stashed INI path using the same deferred pattern and short-circuits on FNV-1a 64 hash match of the on-disk bytes to skip no-op reloads; bytes are read once per load/reload and fed to `CSimpleIniA::LoadData`, so the cached hash and the parsed INI state are guaranteed to reflect the same file snapshot (no TOCTOU between hash and parse); `enable_auto_reload()` owns a `ConfigWatcher` behind a separate `std::mutex` so start/stop transitions do not contend with registration traffic; setters invoked by the watcher run on the watcher thread, setters invoked by the reload hotkey run on a dedicated `ReloadServicer` thread (lazily started on first `register_reload_hotkey`, torn down in `clear_registered_items()`) so the `InputManager` poll thread never blocks on INI parsing; the servicer's press-request path takes its internal `m_mutex` around the predicate store before `cv.notify_one` to close the lost-wakeup window; all setters must be reentrant and thread-safe | N/A (startup only) | @@ -367,7 +369,7 @@ These are called at 60+ fps from game hook callbacks. Never add allocations, exc - `InputPoller::is_binding_active(index)` -- `shared_lock` (uncontended SRWLOCK reader, guards the `m_active_states` pointer swap on reshape) + single `memory_order_relaxed` load - `InputPoller::is_binding_active(name)` -- `shared_lock` + hash lookup + `memory_order_relaxed` load per matching binding (typically 1-3) -- `HookManager::with_inline_hook()` -- shared_lock read +- `HookManager::with_inline_hook()` -- `shared_lock` SRWLOCK reader - `Logger::log()` level check -- single atomic load - `Logger::log()` async enqueue -- atomic shared_ptr load + lock-free queue push - `Memory::is_readable()` -- sharded SRWLOCK reader + cache lookup @@ -410,7 +412,7 @@ These are called at 60+ fps from game hook callbacks. Never add allocations, exc - **Do not use** `EventDispatcher::emit()` from hook callbacks -- use `emit_safe()` instead to prevent unhandled handler exceptions from crashing the host process. - **Do not return** from a memory-writing helper before its post-write cache maintenance (instruction-cache flush and cache-range invalidation) once bytes have been modified -- run the cleanup on every exit path, even when a later step such as restoring page protection fails. - **Do not let** a public doc comment describe behavior the implementation no longer has -- lifecycle and ordering claims must match the code, and are best pinned by a test. -- **Do not take** a shared lock in a const query/accessor that can be called from inside a `with_*`/`try_with_*` callback without first making it reentrancy-guard-aware (skip the lock when the per-thread guard is non-zero, since the callback already holds it). Recursive shared acquisition of the non-recursive `shared_mutex`/SRWLOCK on one thread is undefined behavior and deadlocks if a writer is queued between the two acquisitions (see `HookManager::lock_hooks_shared_reentrant`). +- **Do not take** a shared lock in a const query/accessor that can be called from inside a `with_*`/`try_with_*` callback without first making it reentrancy-guard-aware (skip the lock when the per-thread guard is non-zero, since the callback already holds it). Recursive shared acquisition of a non-recursive reader/writer lock on one thread is undefined behavior and deadlocks if a writer is queued between the two acquisitions (see `HookManager::lock_hooks_shared_reentrant`). - **Do not key** a cache store and its invalidation/eviction by different addresses or shard-selection functions. Eviction must use the same canonical key and the same containment lookup as insertion and read, or entries silently survive invalidation (see `Memory::invalidate_range`, which scans every shard because storage is sharded by query address, not region base). - **Do not let** a queue or backlog fed at an external event rate grow without bound -- clamp the pending count to a documented ceiling (see `Input` wheel-notch `MAX_WHEEL_PENDING`). - **Do not let** an async/deferred sink diverge from its synchronous counterpart's configuration (timestamp format, level, etc.) -- carry the same settings through, as `Logger::enable_async_mode` does for the timestamp format. diff --git a/README.md b/README.md index 7243fe1..181f92f 100644 --- a/README.md +++ b/README.md @@ -255,6 +255,7 @@ See the [Config Hot-Reload Guide](docs/config-hot-reload/README.md) for the thre **Performance:** - Hash-map-backed `is_binding_active()` query for low-overhead cross-thread state reads (e.g., from render hooks at 60+ fps) +- SRWLOCK-backed reader/writer synchronization for live binding reshapes, using the same native Windows lock primitive as hook registries and memory caches - Multiple bindings per name for multi-combo hotkeys - Lock-free `is_running()` via atomic flag - O(1) reverse name lookup for `input_code_to_name()` diff --git a/include/DetourModKit/hook_manager.hpp b/include/DetourModKit/hook_manager.hpp index 362a083..2d53402 100644 --- a/include/DetourModKit/hook_manager.hpp +++ b/include/DetourModKit/hook_manager.hpp @@ -1,26 +1,29 @@ #ifndef DETOURMODKIT_HOOK_MANAGER_HPP #define DETOURMODKIT_HOOK_MANAGER_HPP -#include -#include +#include "DetourModKit/format.hpp" +#include "DetourModKit/logger.hpp" +#include "DetourModKit/scanner.hpp" +#include "DetourModKit/srw_shared_mutex.hpp" + +#include "safetyhook.hpp" + +#include +#include #include +#include +#include #include #include -#include +#include #include -#include -#include +#include #include +#include +#include #include -#include -#include +#include #include -#include - -#include "safetyhook.hpp" -#include "DetourModKit/logger.hpp" -#include "DetourModKit/scanner.hpp" -#include "DetourModKit/format.hpp" namespace DetourModKit { @@ -566,8 +569,8 @@ namespace DetourModKit auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { @@ -680,7 +683,7 @@ namespace DetourModKit /** * @brief Safely accesses a VmHook (method hook) within a named VMT hook. - * @details The callback is invoked while the shared_mutex is held as a reader. + * @details The callback is invoked while the hook registry is held under a reader lock. * @tparam F Callable type accepting (safetyhook::VmHook&) and returning a value. * @param vmt_name The name of the VMT hook. * @param method_index The vtable index of the method hook. @@ -700,7 +703,7 @@ namespace DetourModKit method_index); return std::nullopt; } - std::shared_lock lock(m_hooks_mutex); + std::shared_lock lock(m_hooks_mutex); ReentrancyGuard guard(get_reentrancy_guard()); auto vmt_it = m_vmt_hooks.find(vmt_name); if (vmt_it != m_vmt_hooks.end()) @@ -733,7 +736,7 @@ namespace DetourModKit method_index); return false; } - std::shared_lock lock(m_hooks_mutex); + std::shared_lock lock(m_hooks_mutex); ReentrancyGuard guard(get_reentrancy_guard()); auto vmt_it = m_vmt_hooks.find(vmt_name); if (vmt_it != m_vmt_hooks.end()) @@ -857,19 +860,21 @@ namespace DetourModKit */ std::vector get_hook_ids(std::optional status_filter = std::nullopt) const; + // clang-format off /** * @brief Safely accesses an InlineHook by its ID while holding the internal lock. - * @details The callback is invoked with a reference to the InlineHook while the shared_mutex is held as a - * reader, preventing concurrent removal. - * @warning DANGER: Any callback holding m_hooks_mutex must NOT call methods that - * acquire a unique_lock (remove_hook, enable_hook, disable_hook, create_*_hook) because those calls - * will deadlock. Perform such mutations outside the callback or use an asynchronous/posted operation - * that does not hold m_hooks_mutex. + * @details The callback receives an InlineHook reference while the hook registry is held under a reader lock. + * @warning Do not call HookManager mutators from the callback. The callback runs while m_hooks_mutex is held + * shared: remove_hook and create_*_hook acquire that lock exclusively and self-deadlock, while + * enable_hook/disable_hook re-acquire it shared, which is undefined behavior on a non-recursive lock + * and deadlocks when a writer is queued between the two acquisitions. Queue mutations and apply them + * after the callback returns. * @tparam F Callable type accepting (InlineHook&) and returning a value. * @param hook_id The name of the inline hook. * @param fn The callback to invoke with the hook reference. - * @return std::optional The callback's return value, or std::nullopt if hook not found. + * @return The callback's return value, or std::nullopt if the hook was not found. */ + // clang-format on template requires std::invocable && (!std::is_void_v>) && (!std::is_reference_v>) @@ -886,7 +891,7 @@ namespace DetourModKit hook_id); return std::nullopt; } - std::shared_lock lock(m_hooks_mutex); + std::shared_lock lock(m_hooks_mutex); ReentrancyGuard guard(get_reentrancy_guard()); auto it = m_hooks.find(hook_id); if (it != m_hooks.end() && it->second->get_type() == HookType::Inline) @@ -917,7 +922,7 @@ namespace DetourModKit hook_id); return false; } - std::shared_lock lock(m_hooks_mutex); + std::shared_lock lock(m_hooks_mutex); ReentrancyGuard guard(get_reentrancy_guard()); auto it = m_hooks.find(hook_id); if (it != m_hooks.end() && it->second->get_type() == HookType::Inline) @@ -958,7 +963,7 @@ namespace DetourModKit hook_id); return std::nullopt; } - std::shared_lock lock(m_hooks_mutex, std::try_to_lock); + std::shared_lock lock(m_hooks_mutex, std::try_to_lock); if (!lock.owns_lock()) { return std::nullopt; @@ -972,19 +977,21 @@ namespace DetourModKit return std::nullopt; } + // clang-format off /** * @brief Safely accesses a MidHook by its ID while holding the internal lock. - * @details The callback is invoked with a reference to the MidHook while the shared_mutex is held as a reader, - * preventing concurrent removal. - * @warning DANGER: Any callback holding m_hooks_mutex must NOT call methods that - * acquire a unique_lock (remove_hook, enable_hook, disable_hook, create_*_hook) because those calls - * will deadlock. Perform such mutations outside the callback or use an asynchronous/posted operation - * that does not hold m_hooks_mutex. + * @details The callback receives a MidHook reference while the hook registry is held under a reader lock. + * @warning Do not call HookManager mutators from the callback. The callback runs while m_hooks_mutex is held + * shared: remove_hook and create_*_hook acquire that lock exclusively and self-deadlock, while + * enable_hook/disable_hook re-acquire it shared, which is undefined behavior on a non-recursive lock + * and deadlocks when a writer is queued between the two acquisitions. Queue mutations and apply them + * after the callback returns. * @tparam F Callable type accepting (MidHook&) and returning a value. * @param hook_id The name of the mid hook. * @param fn The callback to invoke with the hook reference. - * @return std::optional The callback's return value, or std::nullopt if hook not found. + * @return The callback's return value, or std::nullopt if the hook was not found. */ + // clang-format on template requires std::invocable && (!std::is_void_v>) && (!std::is_reference_v>) @@ -1001,7 +1008,7 @@ namespace DetourModKit hook_id); return std::nullopt; } - std::shared_lock lock(m_hooks_mutex); + std::shared_lock lock(m_hooks_mutex); ReentrancyGuard guard(get_reentrancy_guard()); auto it = m_hooks.find(hook_id); if (it != m_hooks.end() && it->second->get_type() == HookType::Mid) @@ -1032,7 +1039,7 @@ namespace DetourModKit hook_id); return false; } - std::shared_lock lock(m_hooks_mutex); + std::shared_lock lock(m_hooks_mutex); ReentrancyGuard guard(get_reentrancy_guard()); auto it = m_hooks.find(hook_id); if (it != m_hooks.end() && it->second->get_type() == HookType::Mid) @@ -1073,7 +1080,7 @@ namespace DetourModKit hook_id); return std::nullopt; } - std::shared_lock lock(m_hooks_mutex, std::try_to_lock); + std::shared_lock lock(m_hooks_mutex, std::try_to_lock); if (!lock.owns_lock()) { return std::nullopt; @@ -1096,7 +1103,7 @@ namespace DetourModKit }; explicit HookManager(Logger &logger = Logger::get_instance()); - mutable std::shared_mutex m_hooks_mutex; + mutable detail::SrwSharedMutex m_hooks_mutex; detail::HookMap m_hooks; detail::VmtHookMap m_vmt_hooks; Logger &m_logger; @@ -1108,7 +1115,7 @@ namespace DetourModKit * to block new work. Teardown serialization uses compare_exchange_strong on * m_shutdown_called rather than a separate mutex. */ - mutable std::shared_mutex m_mutator_gate; + mutable detail::SrwSharedMutex m_mutator_gate; /** * @brief Returns the thread-local reentrancy depth counter. @@ -1132,23 +1139,25 @@ namespace DetourModKit ReentrancyGuard &operator=(ReentrancyGuard &&) = delete; }; + // clang-format off /** * @brief Acquires m_hooks_mutex shared, or returns a disengaged lock when reentrant. * @details A with_* or try_with_* callback already holds m_hooks_mutex shared on this thread and has bumped the - * reentrancy guard. Re-locking a non-recursive std::shared_mutex from the same thread is undefined - * behavior (and can deadlock if a writer is queued between the two acquisitions), so a const query - * accessor invoked from inside such a callback must read under the lock the callback already holds - * instead of taking a second shared lock. When not reentrant (guard == 0) the returned lock owns - * m_hooks_mutex for the caller's scope exactly as a direct shared_lock would. + * reentrancy guard. Re-locking the non-recursive reader/writer mutex from the same thread is undefined + * behavior and can deadlock if a writer is queued between the two acquisitions. A const query accessor + * invoked from inside such a callback must read under the lock the callback already holds instead of + * taking a second shared lock. When not reentrant, the returned lock owns m_hooks_mutex for the + * caller's scope exactly as a direct shared_lock would. * @return An engaged shared_lock when guard == 0, otherwise a disengaged one. */ - [[nodiscard]] std::shared_lock lock_hooks_shared_reentrant() const + // clang-format on + [[nodiscard]] std::shared_lock lock_hooks_shared_reentrant() const { if (get_reentrancy_guard() > 0) { - return std::shared_lock{}; + return std::shared_lock{}; } - return std::shared_lock(m_hooks_mutex); + return std::shared_lock(m_hooks_mutex); } std::string error_to_string(const safetyhook::InlineHook::Error &err) const; diff --git a/include/DetourModKit/input.hpp b/include/DetourModKit/input.hpp index 9608b03..dc85a1a 100644 --- a/include/DetourModKit/input.hpp +++ b/include/DetourModKit/input.hpp @@ -3,6 +3,7 @@ #include "DetourModKit/input_codes.hpp" #include "DetourModKit/config.hpp" +#include "DetourModKit/srw_shared_mutex.hpp" #include #include @@ -11,7 +12,6 @@ #include #include #include -#include #include #include #include @@ -167,7 +167,8 @@ namespace DetourModKit [[nodiscard]] bool is_running() const noexcept; /** - * @brief Returns the number of registered bindings. + * @brief Returns the number of registered bindings under the binding reader lock. + * * @return size_t Number of bindings. */ [[nodiscard]] size_t binding_count() const noexcept; @@ -312,11 +313,12 @@ namespace DetourModKit }; // m_bindings_rw_mutex protects m_bindings, m_name_index, m_known_modifiers, and m_has_gamepad_bindings when a - // live update is in flight. The poll - // loop holds a shared lock for the duration of one polling cycle; + // live update is in flight. The poll loop holds a shared lock across the binding-evaluation pass of each + // cycle and releases it before dispatching user callbacks, so callbacks may call binding_count(), + // is_binding_active(), or update_binding_combos() without re-acquiring the non-recursive lock; // update_combos() holds an exclusive lock across the swap. m_active_states entries are always accessed via // atomic ops and need no further guard. - mutable std::shared_mutex m_bindings_rw_mutex; + mutable detail::SrwSharedMutex m_bindings_rw_mutex; std::vector m_bindings; std::unordered_map, StringHash, std::equal_to<>> m_name_index; std::vector m_known_modifiers; diff --git a/include/DetourModKit/srw_shared_mutex.hpp b/include/DetourModKit/srw_shared_mutex.hpp new file mode 100644 index 0000000..b8105e8 --- /dev/null +++ b/include/DetourModKit/srw_shared_mutex.hpp @@ -0,0 +1,61 @@ +#ifndef DETOURMODKIT_SRW_SHARED_MUTEX_HPP +#define DETOURMODKIT_SRW_SHARED_MUTEX_HPP + +#include + +namespace DetourModKit +{ + namespace detail + { + /** + * @class SrwSharedMutex + * @brief Shared mutex backed by Windows SRWLOCK. + * @details Provides the BasicLockable and SharedLockable operations used by std::unique_lock and + * std::shared_lock without including Windows headers from DetourModKit public headers. Exists + * because std::shared_mutex cannot back these locks on every supported toolchain: + * MinGW/winpthreads' pthread_rwlock_t corrupts internal state under high reader contention, + * causing assertion failures in lock_shared(), while SRWLOCK is lock-free in the uncontended case + * and does not suffer from that bug. Use this for DetourModKit reader/writer locks that guard + * runtime state. Prefer std::mutex when no shared-reader path is needed. + */ + class SrwSharedMutex + { + public: + /// Creates an unlocked reader/writer lock. + SrwSharedMutex() noexcept; + + /// Trivially destructible: SRWLOCK has no teardown API, so releasing the storage ends its lifetime. + ~SrwSharedMutex() noexcept = default; + + SrwSharedMutex(const SrwSharedMutex &) = delete; + SrwSharedMutex &operator=(const SrwSharedMutex &) = delete; + SrwSharedMutex(SrwSharedMutex &&) = delete; + SrwSharedMutex &operator=(SrwSharedMutex &&) = delete; + + /// Acquires exclusive ownership, blocking until the lock is available. + void lock() noexcept; + + /// Attempts to acquire exclusive ownership without blocking. + [[nodiscard]] bool try_lock() noexcept; + + /// Releases exclusive ownership held by the current thread. + void unlock() noexcept; + + /// Acquires shared ownership, blocking until the lock is available. + void lock_shared() noexcept; + + /// Attempts to acquire shared ownership without blocking. + [[nodiscard]] bool try_lock_shared() noexcept; + + /// Releases shared ownership held by the current thread. + void unlock_shared() noexcept; + + private: + static constexpr std::size_t SRW_STORAGE_BYTES = sizeof(void *); + + alignas(void *) std::byte m_srw_storage[SRW_STORAGE_BYTES]{}; + }; + } // namespace detail +} // namespace DetourModKit + +#endif // DETOURMODKIT_SRW_SHARED_MUTEX_HPP diff --git a/src/hook_manager.cpp b/src/hook_manager.cpp index 1c5704a..964023f 100644 --- a/src/hook_manager.cpp +++ b/src/hook_manager.cpp @@ -188,18 +188,18 @@ HookManager::~HookManager() noexcept return; } - std::unique_lock mutator_gate(m_mutator_gate); + std::unique_lock mutator_gate(m_mutator_gate); m_shutdown_called.store(true, std::memory_order_release); { - std::shared_lock shared(m_hooks_mutex); + std::shared_lock shared(m_hooks_mutex); for (auto &[name, hook] : m_hooks) { (void)hook->disable(); } } - std::unique_lock lock(m_hooks_mutex); + std::unique_lock lock(m_hooks_mutex); m_vmt_hooks.clear(); m_hooks.clear(); } @@ -213,21 +213,21 @@ void HookManager::shutdown() // Block all mutators (create_*_hook, enable, disable, remove) before entering phase 1. They hold shared on // m_mutator_gate, so acquiring exclusive here waits for active mutators and blocks new ones. - std::unique_lock mutator_gate(m_mutator_gate); + std::unique_lock mutator_gate(m_mutator_gate); // Two-phase shutdown: disable hooks under a shared lock first, then clear the maps under the exclusive lock. The // shared phase lets the kit's own with_* readers (shared_lock holders) finish; SafetyHook::disable() relocates only // threads caught in the patched prologue and does not drain threads already in the detour or trampoline body, so // the caller must quiesce the hooked function during teardown to close the residual window. { - std::shared_lock shared(m_hooks_mutex); + std::shared_lock shared(m_hooks_mutex); for (auto &[name, hook] : m_hooks) { (void)hook->disable(); } } { - std::unique_lock lock(m_hooks_mutex); + std::unique_lock lock(m_hooks_mutex); m_vmt_hooks.clear(); m_hooks.clear(); @@ -310,8 +310,8 @@ std::expected HookManager::create_inline_hook(std::strin auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); // Re-check after acquiring locks (double-checked pattern). if (m_shutdown_called.load(std::memory_order_acquire)) @@ -497,8 +497,8 @@ std::expected HookManager::create_mid_hook(std::string_v auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { @@ -657,7 +657,7 @@ std::expected HookManager::remove_hook(std::string_view hook_id auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); + std::shared_lock mutator_gate(m_mutator_gate); // Re-check after acquiring the gate. A thread can observe shutdown as false above, then block here behind // remove_all_hooks()'s exclusive gate; once that releases (with m_shutdown_called reset to false) this would @@ -677,7 +677,7 @@ std::expected HookManager::remove_hook(std::string_view hook_id // thread-suspend teardown off the exclusive lock. The caller must ensure no thread is executing the hooked // function during removal to close the residual narrow window. { - std::shared_lock shared(m_hooks_mutex); + std::shared_lock shared(m_hooks_mutex); auto it = m_hooks.find(hook_id); if (it == m_hooks.end()) { @@ -689,7 +689,7 @@ std::expected HookManager::remove_hook(std::string_view hook_id (void)it->second->disable(); } - std::unique_lock lock(m_hooks_mutex); + std::unique_lock lock(m_hooks_mutex); std::vector logs; auto it = m_hooks.find(hook_id); if (it == m_hooks.end()) @@ -732,7 +732,7 @@ void HookManager::remove_all_hooks() { // Block all mutators (create_*_hook, enable, disable, remove) before entering phase 1. They hold shared on // m_mutator_gate, so acquiring exclusive here waits for active mutators and blocks new ones. - std::unique_lock mutator_gate(m_mutator_gate); + std::unique_lock mutator_gate(m_mutator_gate); // Two-phase removal: disable hooks under the shared lock first, then take the exclusive lock to erase. The // shared phase lets the kit's own with_inline_hook readers (shared_lock holders) finish before the Hook is @@ -740,14 +740,14 @@ void HookManager::remove_all_hooks() // detour or trampoline body, so the caller must quiesce the hooked function during teardown to close the // residual narrow window. { - std::shared_lock shared(m_hooks_mutex); + std::shared_lock shared(m_hooks_mutex); for (auto &[name, hook] : m_hooks) { (void)hook->disable(); } } - std::unique_lock lock(m_hooks_mutex); + std::unique_lock lock(m_hooks_mutex); num_vmt = m_vmt_hooks.size(); m_vmt_hooks.clear(); @@ -787,8 +787,8 @@ std::expected HookManager::enable_hook(std::string_view hook_id auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::shared_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::shared_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { return {std::unexpected(HookError::ShutdownInProgress), @@ -842,8 +842,8 @@ std::expected HookManager::disable_hook(std::string_view hook_i auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::shared_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::shared_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { return {std::unexpected(HookError::ShutdownInProgress), @@ -919,8 +919,8 @@ std::size_t HookManager::enable_hooks(std::span hook_ids return 0; } - std::shared_lock mutator_gate(m_mutator_gate); - std::shared_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::shared_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { m_logger.warning("HookManager: Shutdown in progress. Cannot enable {} hook(s).", hook_ids.size()); @@ -952,8 +952,8 @@ std::size_t HookManager::disable_hooks(std::span hook_id return 0; } - std::shared_lock mutator_gate(m_mutator_gate); - std::shared_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::shared_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { m_logger.warning("HookManager: Shutdown in progress. Cannot disable {} hook(s).", hook_ids.size()); @@ -985,8 +985,8 @@ std::size_t HookManager::enable_all_hooks() return 0; } - std::shared_lock mutator_gate(m_mutator_gate); - std::shared_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::shared_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { m_logger.warning("HookManager: Shutdown in progress. Cannot enable all hooks."); @@ -1012,8 +1012,8 @@ std::size_t HookManager::disable_all_hooks() return 0; } - std::shared_lock mutator_gate(m_mutator_gate); - std::shared_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::shared_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { m_logger.warning("HookManager: Shutdown in progress. Cannot disable all hooks."); @@ -1081,8 +1081,8 @@ std::expected HookManager::create_vmt_hook(std::string_v auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { @@ -1157,8 +1157,8 @@ std::expected HookManager::remove_vmt_hook(std::string_view vmt auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); // Re-check after acquiring the gate so a teardown that flipped m_shutdown_called while we waited is not raced // (uniform with the create/enable/disable mutators). if (m_shutdown_called.load(std::memory_order_acquire)) @@ -1197,8 +1197,8 @@ std::expected HookManager::remove_vmt_method(std::string_view v auto [result, deferred_logs] = [&]() -> std::pair, std::vector> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { return {std::unexpected(HookError::ShutdownInProgress), @@ -1246,8 +1246,8 @@ bool HookManager::apply_vmt_hook(std::string_view vmt_name, void *object) auto [result, deferred_logs] = [&]() -> std::pair> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { return {false, @@ -1310,8 +1310,8 @@ bool HookManager::remove_vmt_from_object(std::string_view vmt_name, void *object auto [result, deferred_logs] = [&]() -> std::pair> { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); if (m_shutdown_called.load(std::memory_order_acquire)) { return { @@ -1348,8 +1348,8 @@ void HookManager::remove_all_vmt_hooks() std::vector deferred_logs = [&]() { - std::shared_lock mutator_gate(m_mutator_gate); - std::unique_lock lock(m_hooks_mutex); + std::shared_lock mutator_gate(m_mutator_gate); + std::unique_lock lock(m_hooks_mutex); std::vector logs; if (m_shutdown_called.load(std::memory_order_acquire)) { diff --git a/src/input.cpp b/src/input.cpp index d3a8871..be82246 100644 --- a/src/input.cpp +++ b/src/input.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -426,6 +427,7 @@ namespace DetourModKit size_t InputPoller::binding_count() const noexcept { + std::shared_lock lock(m_bindings_rw_mutex); return m_bindings.size(); } @@ -831,9 +833,10 @@ namespace DetourModKit } // Cardinality-preserving fast path: in-place rewrite of keys and modifiers leaves m_bindings and - // m_active_states in lockstep. The poll thread holds a shared_lock for the duration of one tick, so the - // unique_lock here serializes against it; concurrent is_binding_active(size_t) reads stay valid because the - // binding count and array sizes do not change. + // m_active_states in lockstep. The poll thread's binding-evaluation pass and every other reader + // (is_binding_active, binding_count) hold the shared lock, so the unique_lock here serializes against + // them; concurrent is_binding_active(size_t) reads stay valid because the binding count and array + // sizes do not change. if (indices.size() == combos.size()) { std::vector replacements; @@ -1424,12 +1427,16 @@ namespace DetourModKit size_t InputManager::binding_count() const noexcept { - std::lock_guard lock(m_mutex); - if (m_poller) + std::shared_ptr live_poller; { - return m_poller->binding_count(); + std::lock_guard lock(m_mutex); + if (!m_poller) + { + return m_pending_bindings.size(); + } + live_poller = m_poller; } - return m_pending_bindings.size(); + return live_poller->binding_count(); } bool InputManager::is_binding_active(std::string_view name) const noexcept diff --git a/src/memory.cpp b/src/memory.cpp index 9609669..fef8cb6 100644 --- a/src/memory.cpp +++ b/src/memory.cpp @@ -13,6 +13,7 @@ #include "DetourModKit/diagnostics.hpp" #include "DetourModKit/format.hpp" #include "DetourModKit/logger.hpp" +#include "DetourModKit/srw_shared_mutex.hpp" #include "platform.hpp" #include @@ -25,6 +26,7 @@ #include #include #include +#include #include #include #include @@ -38,6 +40,7 @@ using namespace DetourModKit; using DetourModKit::detail::is_loader_lock_held; using DetourModKit::detail::pin_current_module; +using DetourModKit::detail::SrwSharedMutex; // Anonymous namespace for internal helpers and storage namespace @@ -55,33 +58,6 @@ namespace static constexpr DWORD NOACCESS_GUARD_FLAGS = PAGE_NOACCESS | PAGE_GUARD; }; - /** - * @class SrwSharedMutex - * @brief Shared mutex backed by Windows SRWLOCK instead of pthread_rwlock_t. - * @details MinGW/winpthreads' pthread_rwlock_t corrupts internal state under high reader contention, causing - * assertion failures in lock_shared(). SRWLOCK is kernel-level, lock-free for uncontended cases, and does - * not suffer from this bug. - */ - class SrwSharedMutex - { - public: - SrwSharedMutex() noexcept { InitializeSRWLock(&m_srw); } - - SrwSharedMutex(const SrwSharedMutex &) = delete; - SrwSharedMutex &operator=(const SrwSharedMutex &) = delete; - - void lock() noexcept { AcquireSRWLockExclusive(&m_srw); } - bool try_lock() noexcept { return TryAcquireSRWLockExclusive(&m_srw) != 0; } - void unlock() noexcept { ReleaseSRWLockExclusive(&m_srw); } - - void lock_shared() noexcept { AcquireSRWLockShared(&m_srw); } - bool try_lock_shared() noexcept { return TryAcquireSRWLockShared(&m_srw) != 0; } - void unlock_shared() noexcept { ReleaseSRWLockShared(&m_srw); } - - private: - SRWLOCK m_srw; - }; - /** * @struct CachedMemoryRegionInfo * @brief Structure to hold cached memory region information. @@ -1622,7 +1598,7 @@ namespace // DMK_Shutdown() (callers do not query ranges from atexit handlers). struct ModuleRangeCache { - std::shared_mutex mtx; + SrwSharedMutex mtx; std::unordered_map entries; }; @@ -1648,7 +1624,7 @@ std::optional DetourModKit::Memory::module_ra auto &cache = get_module_range_cache(); { - std::shared_lock lock(cache.mtx); + std::shared_lock lock(cache.mtx); const auto it = cache.entries.find(mod); if (it != cache.entries.end()) return it->second; @@ -1659,7 +1635,7 @@ std::optional DetourModKit::Memory::module_ra return std::nullopt; { - std::unique_lock lock(cache.mtx); + std::unique_lock lock(cache.mtx); // Another thread may have inserted between our shared/unique transition; // emplace skips on collision so we keep the first-resolved entry. cache.entries.emplace(mod, range); diff --git a/src/srw_shared_mutex.cpp b/src/srw_shared_mutex.cpp new file mode 100644 index 0000000..17bbd7a --- /dev/null +++ b/src/srw_shared_mutex.cpp @@ -0,0 +1,69 @@ +#include "DetourModKit/srw_shared_mutex.hpp" + +#include +#include +#include + +namespace DetourModKit +{ + namespace detail + { + static_assert(sizeof(SrwSharedMutex) == sizeof(SRWLOCK)); + static_assert(alignof(SrwSharedMutex) == alignof(SRWLOCK)); + // The defaulted destructor never runs ~SRWLOCK on the placement-new'd object; that is only correct while + // SRWLOCK stays trivially destructible (releasing the storage then legally ends its lifetime). + static_assert(std::is_trivially_destructible_v); + + namespace + { + /** + * @brief Converts SrwSharedMutex opaque storage back to the native Windows lock. + * @details std::launder is required: the byte array only provides storage for the SRWLOCK created by + * placement new in the constructor, and an array element is not pointer-interconvertible with + * the object nested in it, so the bare reinterpret_cast would still designate the byte element. + * @param storage Storage owned by a live SrwSharedMutex instance. + * @return Pointer to the SRWLOCK object living in the byte buffer. + */ + [[nodiscard]] SRWLOCK *native_lock(std::byte *storage) noexcept + { + return std::launder(reinterpret_cast(storage)); + } + } // namespace + + SrwSharedMutex::SrwSharedMutex() noexcept + { + auto *lock = ::new (static_cast(m_srw_storage)) SRWLOCK{}; + InitializeSRWLock(lock); + } + + void SrwSharedMutex::lock() noexcept + { + AcquireSRWLockExclusive(native_lock(m_srw_storage)); + } + + bool SrwSharedMutex::try_lock() noexcept + { + return TryAcquireSRWLockExclusive(native_lock(m_srw_storage)) != 0; + } + + void SrwSharedMutex::unlock() noexcept + { + ReleaseSRWLockExclusive(native_lock(m_srw_storage)); + } + + void SrwSharedMutex::lock_shared() noexcept + { + AcquireSRWLockShared(native_lock(m_srw_storage)); + } + + bool SrwSharedMutex::try_lock_shared() noexcept + { + return TryAcquireSRWLockShared(native_lock(m_srw_storage)) != 0; + } + + void SrwSharedMutex::unlock_shared() noexcept + { + ReleaseSRWLockShared(native_lock(m_srw_storage)); + } + } // namespace detail +} // namespace DetourModKit diff --git a/tests/test_hook_integration.cpp b/tests/test_hook_integration.cpp index 009b006..9c42a15 100644 --- a/tests/test_hook_integration.cpp +++ b/tests/test_hook_integration.cpp @@ -162,8 +162,8 @@ TEST_F(HookIntegrationTest, QueryAccessorsAreReentrantFromCallback) // The callback runs while m_hooks_mutex is held shared. The read-only query accessors are reentrancy-aware: invoked // from inside a with_* callback they read under the lock the callback already holds instead of taking a second - // shared_lock on the non-recursive shared_mutex (undefined behavior, and deadlock-prone when a writer is queued - // between the two acquisitions). The callback must complete and return the correct values. + // shared_lock on the non-recursive reader/writer mutex (undefined behavior, and deadlock-prone when a writer is + // queued between the two acquisitions). The callback must complete and return the correct values. auto status = m_hook_manager->with_inline_hook( "ReentrantQueryHook", [&](InlineHook &) -> HookStatus diff --git a/tests/test_input.cpp b/tests/test_input.cpp index 5c676b9..f204f02 100644 --- a/tests/test_input.cpp +++ b/tests/test_input.cpp @@ -1942,6 +1942,83 @@ TEST(InputManagerUpdateCombos, ConcurrentUpdateWhilePollerRunning) SUCCEED(); } +TEST(InputManagerUpdateCombos, ConcurrentQueriesAndCardinalityUpdatesWhilePollerRunning) +{ + auto &im = InputManager::get_instance(); + im.shutdown(); + im.set_require_focus(false); + + Config::KeyComboList initial; + initial.push_back({{keyboard_key(0x41)}, {}}); // 'A' + im.register_press("update-query-stress", initial, []() {}); + im.start(std::chrono::milliseconds(1)); + + constexpr int READER_THREADS = 4; + constexpr int ITERATIONS = 1000; + std::atomic start{false}; + std::atomic stop{false}; + std::atomic invalid_counts{0}; + + std::vector readers; + readers.reserve(READER_THREADS); + for (int i = 0; i < READER_THREADS; ++i) + { + readers.emplace_back( + [&im, &start, &stop, &invalid_counts]() + { + while (!start.load(std::memory_order_acquire)) + { + std::this_thread::yield(); + } + + while (!stop.load(std::memory_order_acquire)) + { + const size_t count = im.binding_count(); + if (count < 1 || count > 2) + { + invalid_counts.fetch_add(1, std::memory_order_relaxed); + } + (void)im.is_binding_active("update-query-stress"); + } + }); + } + + start.store(true, std::memory_order_release); + for (int i = 0; i < ITERATIONS; ++i) + { + Config::KeyComboList replacement; + switch (i % 3) + { + case 0: + replacement.push_back({{keyboard_key(0x41)}, {}}); // 'A' + break; + case 1: + replacement.push_back({{keyboard_key(0x5A)}, {}}); // 'Z' + replacement.push_back({{keyboard_key(0x58)}, {}}); // 'X' + break; + default: + break; + } + im.update_binding_combos("update-query-stress", replacement); + if ((i % 32) == 0) + { + std::this_thread::yield(); + } + } + + stop.store(true, std::memory_order_release); + for (auto &reader : readers) + { + reader.join(); + } + + EXPECT_EQ(invalid_counts.load(std::memory_order_relaxed), static_cast(0)); + EXPECT_TRUE(im.is_running()); + + im.shutdown(); + im.set_require_focus(true); +} + TEST(InputManagerHotReload, RegisterPressWhilePollerRunning) { auto &im = InputManager::get_instance(); diff --git a/tests/test_srw_shared_mutex.cpp b/tests/test_srw_shared_mutex.cpp new file mode 100644 index 0000000..99eb291 --- /dev/null +++ b/tests/test_srw_shared_mutex.cpp @@ -0,0 +1,127 @@ +#include "DetourModKit/srw_shared_mutex.hpp" + +#include + +#include +#include +#include +#include +#include + +using DetourModKit::detail::SrwSharedMutex; + +// SRWLOCK owners and waiters reference the lock by address, so an SrwSharedMutex must keep a stable address for its +// entire lifetime. All four copy/move special members are deleted; these guards keep that contract from regressing. +static_assert(!std::is_copy_constructible_v, + "SrwSharedMutex must remain non-copyable: SRWLOCK state cannot be duplicated while owners or waiters " + "reference its address."); +static_assert(!std::is_copy_assignable_v, + "SrwSharedMutex must remain non-copyable: SRWLOCK state cannot be duplicated while owners or waiters " + "reference its address."); +static_assert(!std::is_move_constructible_v, + "SrwSharedMutex must remain non-movable: waiters block on the SRWLOCK's address, which must stay " + "stable."); +static_assert(!std::is_move_assignable_v, + "SrwSharedMutex must remain non-movable: waiters block on the SRWLOCK's address, which must stay " + "stable."); + +TEST(SrwSharedMutexTest, SharedReadersCanOverlap) +{ + SrwSharedMutex mutex; + std::shared_lock first_reader(mutex); + + std::atomic acquired{false}; + std::thread reader( + [&]() -> void + { + std::shared_lock second_reader(mutex, std::try_to_lock); + acquired.store(second_reader.owns_lock(), std::memory_order_release); + }); + + reader.join(); + + EXPECT_TRUE(acquired.load(std::memory_order_acquire)); +} + +TEST(SrwSharedMutexTest, ExclusiveTryLockFailsWhileReaderHeld) +{ + SrwSharedMutex mutex; + std::shared_lock reader(mutex); + + std::atomic acquired{true}; + std::thread writer( + [&]() -> void + { + std::unique_lock writer_lock(mutex, std::try_to_lock); + acquired.store(writer_lock.owns_lock(), std::memory_order_release); + }); + + writer.join(); + + EXPECT_FALSE(acquired.load(std::memory_order_acquire)); +} + +TEST(SrwSharedMutexTest, SharedTryLockFailsWhileWriterHeld) +{ + SrwSharedMutex mutex; + std::unique_lock writer(mutex); + + std::atomic acquired{true}; + std::thread reader( + [&]() -> void + { + std::shared_lock reader_lock(mutex, std::try_to_lock); + acquired.store(reader_lock.owns_lock(), std::memory_order_release); + }); + + reader.join(); + + EXPECT_FALSE(acquired.load(std::memory_order_acquire)); +} + +TEST(SrwSharedMutexTest, TryLockSucceedsWhenFreeAndUnlockReleases) +{ + SrwSharedMutex mutex; + + // After the first acquisition, each subsequent one can only succeed if the preceding unlock genuinely released + // ownership, so this sequence pins the success mapping of both try paths and the release mapping of both unlock + // paths in one pass. + ASSERT_TRUE(mutex.try_lock()); + mutex.unlock(); + ASSERT_TRUE(mutex.try_lock_shared()); + mutex.unlock_shared(); + EXPECT_TRUE(mutex.try_lock()); + mutex.unlock(); +} + +TEST(SrwSharedMutexTest, BlockedWriterAcquiresAfterReaderReleases) +{ + SrwSharedMutex mutex; + std::shared_lock reader(mutex); + + std::atomic writer_started{false}; + std::atomic writer_acquired{false}; + std::thread writer( + [&]() -> void + { + writer_started.store(true, std::memory_order_release); + std::unique_lock writer_lock(mutex); + writer_acquired.store(true, std::memory_order_release); + }); + + while (!writer_started.load(std::memory_order_acquire)) + { + std::this_thread::yield(); + } + + // The writer's blocking lock() cannot return while the shared owner is live, so the flag must still be clear + // here regardless of scheduling. + EXPECT_FALSE(writer_acquired.load(std::memory_order_acquire)); + + // Releasing the shared owner must wake the blocked writer. A broken release manifests as a deterministic hang + // (test timeout), never a flaky pass. + reader.unlock(); + writer.join(); + + EXPECT_TRUE(writer_acquired.load(std::memory_order_acquire)); +} From d0ab93df5251c81cfe7a8bab97ce0d5baa75e8fb Mon Sep 17 00:00:00 2001 From: Quang Trinh Date: Fri, 12 Jun 2026 01:18:26 +0700 Subject: [PATCH 2/2] fix: disable all MSVC STL ASan annotation families in sanitizer builds --- CMakeLists.txt | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index dfc30a3..c2327f4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -527,15 +527,20 @@ if(DMK_ENABLE_SANITIZERS) string(REGEX REPLACE "/RTC[1csu]+" "" CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG}") # The vendored SafetyHook is not instrumented, so the MSVC STL ASan container - # annotations (annotate_string / annotate_vector) would differ across the link - # and fail with LNK2038. Disable those annotations on every instrumented target - # so all objects agree (value 0); the core ASan checks (heap/stack/use-after- - # free) stay on. The library is instrumented explicitly because it was created + # annotations (annotate_string / annotate_vector / annotate_optional) would + # differ across the link and fail with LNK2038. Disable those annotations on + # every instrumented target so all objects agree (value 0); the core ASan + # checks (heap/stack/use-after-free) stay on. _DISABLE_STL_ANNOTATION is the + # STL's umbrella switch and auto-defines every per-family disable the toolset + # knows, so annotation families added in future STL versions stay covered; + # the explicit string/vector macros remain for VS 2022 toolsets that predate + # the umbrella. The library is instrumented explicitly because it was created # before this block; the tests (added afterwards) pick up the directory options. target_compile_options(DetourModKit PRIVATE /fsanitize=address) - target_compile_definitions(DetourModKit PRIVATE _DISABLE_STRING_ANNOTATION=1 _DISABLE_VECTOR_ANNOTATION=1) + target_compile_definitions(DetourModKit PRIVATE + _DISABLE_STL_ANNOTATION=1 _DISABLE_STRING_ANNOTATION=1 _DISABLE_VECTOR_ANNOTATION=1) add_compile_options(/fsanitize=address) - add_compile_definitions(_DISABLE_STRING_ANNOTATION=1 _DISABLE_VECTOR_ANNOTATION=1) + add_compile_definitions(_DISABLE_STL_ANNOTATION=1 _DISABLE_STRING_ANNOTATION=1 _DISABLE_VECTOR_ANNOTATION=1) add_link_options(/INCREMENTAL:NO) message(STATUS "Sanitizers enabled: ASan (MSVC)") elseif(WIN32)