From 51e2c4f4cdf0f1246b1073bd6ada9b10d94abde5 Mon Sep 17 00:00:00 2001 From: BlackMark Date: Tue, 23 Jun 2026 12:23:06 +0200 Subject: [PATCH] Preserve SafetyHook concurrency finding as a committed upstream repro A multithreaded test that tight-looped InlineHook enable()/disable() while other threads called the hooked function flaked ~1/10. Isolation proved this is a SafetyHook limitation, not our code: with the hook created once (no install race, no trampoline UAF), tight-loop toggling AVs ~1/3 of runs in Debug (0xC0000005, faulting RIP in the target body), while a no-toggle control is clean at ~60M calls. enable()/disable() re-patch the prologue in place under a VEH page-trap that only relocates a thread parked ON the prologue; a thread in the function body faults on the briefly-non-exec page and relies on instruction retry, which under rapid toggling races a half-rewritten prologue. Rather than silently drop the flaky test, preserve the finding: - tools/sh_concurrency_repro/: minimal, committed, non-CI reproducer (coop_sh_concurrency_repro; --callonly is the control). Surfaces 5/16 AVs. - docs/safetyhook-concurrency.md: upstream-ready write-up (mechanism + fix directions + why it does not affect us). - README lessons-learned + memory updated; tests/CMakeLists cross-references it. Our code stays in SafetyHook's safe envelope (install/remove reconciled from a single tick-bounded worker thread, never a tight loop), so the mock_game_test storm is reliably green; the persistent-trampoline contract is covered deterministically by hook_install_test + detour_gate_test. Removes the temp _sh_probe wiring. Co-Authored-By: Claude Opus 4.8 --- CMakeLists.txt | 1 + README.md | 14 +++ docs/safetyhook-concurrency.md | 105 ++++++++++++++++++++++ tests/CMakeLists.txt | 8 ++ tools/sh_concurrency_repro/CMakeLists.txt | 6 ++ tools/sh_concurrency_repro/main.cpp | 79 ++++++++++++++++ 6 files changed, 213 insertions(+) create mode 100644 docs/safetyhook-concurrency.md create mode 100644 tools/sh_concurrency_repro/CMakeLists.txt create mode 100644 tools/sh_concurrency_repro/main.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 20613e6..9d504ab 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -139,6 +139,7 @@ if(COOP_BUILD_HOOK) add_subdirectory(tools/audio_validate) # coop_audio_validate: quantify capture fidelity (pitch/SNR/clicks) add_subdirectory(tools/input_probe) # coop_input_probe: inject + forward synthetic input add_subdirectory(tools/vk_validate) # coop_vk_validate: validate the Vulkan backend vs a real game + add_subdirectory(tools/sh_concurrency_repro) # coop_sh_concurrency_repro: upstream SafetyHook repro (see docs/) add_subdirectory(tests) endif() diff --git a/README.md b/README.md index df4f224..39162e7 100644 --- a/README.md +++ b/README.md @@ -719,6 +719,20 @@ Non-obvious things that cost time and constrain the design: original) — no code patching, pristine stack regardless of prologue. Inline hooking stays fine for `Present`/`SwapBuffers` (clean prologues). Guarded by the x86 hook tests. +- **SafetyHook's `enable()`/`disable()` aren't safe to call in a *tight loop* concurrent with calls to + the hooked function.** They re-patch the prologue in place under a VEH "trap" that reliably relocates + a thread *parked on the prologue*, but a thread executing the function **body** faults when the whole + page is briefly made non-executable during the patch and relies on a retry — and under rapid toggling + that retry races a half-rewritten prologue → AV in the caller. A minimal reproducer (one thread + tight-looping enable/disable, two hammering the target; hook created once, so no install race / UAF) + faults ~1/3 of runs in Debug while the no-toggle control is clean. This is a SafetyHook limitation, + not our code: we reconcile install/remove from a **single, tick-bounded worker thread**, never a + tight loop, so the `mock_game_test` storm (a single enable/disable concurrent with thousands of + calls/s — SafetyHook's designed-safe case) is reliably crash-free. The finding is preserved as a + committed, non-CI reproducer (`tools/sh_concurrency_repro/`) and written up for upstream in + [docs/safetyhook-concurrency.md](docs/safetyhook-concurrency.md); an earlier flaky tight-loop CTest + that hit this was replaced by the deterministic `hook_install_test` + `detour_gate_test` (our + contract) plus the storm (realistic concurrency). - **Steam Input init suppresses XInput.** Initializing Steam Input turns on Steam's in-process XInput interception, which hides controllers from `XInputGetState` unless they're bound to the running appid's action set — defaulting to it diff --git a/docs/safetyhook-concurrency.md b/docs/safetyhook-concurrency.md new file mode 100644 index 0000000..e00fbe2 --- /dev/null +++ b/docs/safetyhook-concurrency.md @@ -0,0 +1,105 @@ +# SafetyHook: `InlineHook::enable()/disable()` are unsafe under rapid concurrent toggling + +**Component:** SafetyHook (vendored as a submodule at `third_party/safetyhook`) +**Version:** `v0.7.0-1-g2f28386` (commit `2f283866189c5c728384ae8b9e7f58c268ae036c`) +**Platform observed:** Windows x64, MSVC, Release & Debug +**Severity:** crash (access violation) in the *caller* of a hooked function +**Reproducer:** `tools/sh_concurrency_repro/` (built target `coop_sh_concurrency_repro`) + +This is a write-up of a limitation we hit in CoopAllTheThings, kept so it can be filed upstream +and so we don't silently rediscover it. Our shipping code does **not** depend on the unsafe pattern +(see "Why this does not affect CoopAllTheThings" below); this documents the underlying behaviour. + +## Summary + +`InlineHook::enable()` and `InlineHook::disable()` rewrite the target function's prologue bytes in +place, guarding concurrently-executing threads with a VEH-based "trap" (`trap_threads` in +`src/os.windows.cpp`). That trap reliably rescues a thread **parked on the prologue bytes being +patched**, but it does *not* safely handle a thread executing in the function **body** that faults +because the whole code page was made non-executable during the patch — when `enable()`/`disable()` +are called in a **tight loop** concurrent with calls to the hooked function. Under that load the +re-patch (a non-atomic byte copy) races a faulting caller's instruction retry, and the process +crashes with an access violation whose faulting RIP is inside the target's body (not on the prologue). + +## Reproducer + +`tools/sh_concurrency_repro/main.cpp`. The hook is created **once** and never recreated/destroyed, so +no install race and no trampoline use-after-free are involved — the only variable is concurrent +`enable()`/`disable()` vs. calls. + +- **default mode:** one thread tight-loops `disable()`/`enable()`; two threads hammer `target_fn`. +- **`--callonly` (control):** the two caller threads hammer; **no** toggling. + +Run it repeatedly: + +```powershell +1..16 | % { & .\coop_sh_concurrency_repro.exe; "exit=$LASTEXITCODE" } +``` + +**Observed** (this repo, ~215k toggle cycles per 3s run): default mode faults with `0xC0000005` +frequently — **5 of 16** runs in a Debug build, ~1 in 8 in earlier Release runs; `--callonly` is +**clean every run** (~60M calls). Under a debugger the faulting instruction pointer is inside the +target function body (e.g. `target_fn+0x4`), i.e. a *caller* thread, not the toggling thread. Note the +toggle mode completes far fewer caller calls (~1M vs ~60M) because the VEH fault storm and the +`trap_threads` mutex throttle the callers — itself evidence of the thundering-herd fault path. + +## Mechanism (why it faults) + +In `enable()` / `disable()` the byte rewrite runs inside `trap_threads(from, to, len, run_fn)` +(`src/os.windows.cpp`): + +1. registers a trap `{from, to, len}` in a global map and installs a VEH (`trap_handler`) once; +2. `VirtualProtect`s **both the `from` and `to` pages to `PAGE_READWRITE`** — removing *execute* + permission from the entire page the function lives on — for the duration of the rewrite; +3. runs `run_fn` (a plain, non-atomic `emit_jmp` on enable / `std::copy` of the original bytes on + disable); +4. restores the page protections. + +While the page is non-executable, any thread executing **anywhere on it** faults. `trap_handler`: + +- if the faulting RIP is exactly within `[from, from+len)` (a thread parked on the prologue), + relocates it to the matching offset in `to` via `fix_ip` — **only** an exact `RIP == from+i` match; +- otherwise, if the fault is merely elsewhere on the trapped page, returns + `EXCEPTION_CONTINUE_EXECUTION` to **retry** the instruction once protections are restored. + +This is correct for a *single, occasional* enable/disable. It breaks under tight-loop toggling: + +- A caller executing the function **body** (past the stolen prologue bytes) is never relocated by + `fix_ip` (its RIP is not `from+i`); it depends entirely on the retry path, which assumes the bytes + at its RIP are **stable** once executable again. +- But under a tight loop one thread is almost continuously inside `trap_threads` flipping page + protection and rewriting the prologue (enable writes the jump, disable restores originals), while + caller threads continuously fault on the non-executable page and re-enter the VEH. The opposing + traps `{target→trampoline}` and `{trampoline→target}` are keyed differently and **never removed** + from the map (`add_trap` only `insert_or_assign`s), so they accumulate and can ping-pong a retrying + thread between them. +- A caller that retries at an instruction boundary while the prologue is **half-rewritten** by a + concurrent toggle decodes a torn instruction → unrecoverable AV in the body. + +In short: the re-patch is not atomic with respect to a concurrently *retrying* caller, and the VEH +relocation only rescues threads parked on the prologue, not threads mid-body caught by the +whole-page non-executable window during a re-patch that another toggle is simultaneously mutating. + +## Suggested directions for an upstream fix + +- Remove a trap from the map once `trap_threads` finishes, so opposing traps can't accumulate and + ping-pong. +- Quiesce in-flight callers for the duration of the rewrite (e.g. suspend other threads, or stage the + patch so no thread can observe a half-written prologue), rather than relying solely on retry. +- Document explicitly that `enable()`/`disable()` are not safe to call in a tight loop concurrent with + calls to the hooked function, and recommend an atomic enable/disable that does not re-patch bytes + (e.g. a gate the detour checks) for callers that toggle frequently. + +## Why this does not affect CoopAllTheThings + +Our hooks are reconciled from a **single worker thread, tick-bounded** (`hook/src/dllmain.cpp`): a +subsystem's `install_*`/`remove_*` (≡ `enable()`/`disable()`) runs at most once per reconcile tick, +never in a tight loop, and never from two threads at once for the same hook. The end-to-end +hook/unhook storm (`tests/mock_game_test.cpp`) toggles every subsystem while the game presents at +thousands of frames/s and is reliably crash-free — that is SafetyHook's designed, safe case (a single +enable/disable concurrent with calls; the VEH relocates in-flight callers). Only the synthetic +tight-loop in the reproducer, which our architecture cannot produce, hits the window above. + +Our contract (persistent trampoline reuse, drain coordination) is covered deterministically by +`tests/hook_install_test.cpp` and `tests/detour_gate_test.cpp`; the integrated concurrent behaviour at +realistic cadence is covered by `tests/mock_game_test.cpp`. diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b8b772e..5ef47ef 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -48,6 +48,14 @@ add_test(NAME detour_gate_test COMMAND detour_gate_test) # Deterministic contract test for the persistent inline-hook model (hook/src/hook_install.hpp): # install_inline creates a hook once and re-enables it on re-install, reusing the trampoline (never # freed) so a stale detour can't UAF. Single-threaded, so it can't flake on enable/disable atomicity. +# +# A multithreaded TIGHT-LOOP toggle test used to live here and flaked ~1/10: it was asserting an +# atomicity that SafetyHook's enable()/disable() don't provide and that our code never needs (we +# reconcile install/remove from a single, tick-bounded worker thread -- never a tight loop). That +# valid contract is now covered deterministically here + by detour_gate_test; the realistic concurrent +# behaviour by mock_game_test's storm; and the SafetyHook limitation itself is captured as a committed +# upstream reproducer at tools/sh_concurrency_repro/ (see docs/safetyhook-concurrency.md) rather than a +# flaky CTest. add_executable(hook_install_test hook_install_test.cpp) target_include_directories(hook_install_test PRIVATE ${CMAKE_SOURCE_DIR}/hook/src) target_link_libraries(hook_install_test PRIVATE safetyhook::safetyhook) diff --git a/tools/sh_concurrency_repro/CMakeLists.txt b/tools/sh_concurrency_repro/CMakeLists.txt new file mode 100644 index 0000000..557fcb9 --- /dev/null +++ b/tools/sh_concurrency_repro/CMakeLists.txt @@ -0,0 +1,6 @@ +# Standalone reproducer for a SafetyHook enable()/disable()-vs-concurrent-call concurrency limitation. +# NOT a CTest (it is probabilistic) -- it documents an upstream issue and validates an upstream fix. +# See docs/safetyhook-concurrency.md. +add_executable(coop_sh_concurrency_repro main.cpp) +target_link_libraries(coop_sh_concurrency_repro PRIVATE safetyhook::safetyhook) +coop_output_subdir(tools coop_sh_concurrency_repro) # dev tool -> bin//tools/ diff --git a/tools/sh_concurrency_repro/main.cpp b/tools/sh_concurrency_repro/main.cpp new file mode 100644 index 0000000..6458605 --- /dev/null +++ b/tools/sh_concurrency_repro/main.cpp @@ -0,0 +1,79 @@ +// Minimal reproducer for a SafetyHook concurrency limitation (see docs/safetyhook-concurrency.md). +// +// Claim: safetyhook::InlineHook::enable()/disable() are NOT safe to call in a tight loop while other +// threads call the hooked function. The hook here is created ONCE (no install race, no create/destroy, +// no trampoline churn), so a crash is purely SafetyHook's enable/disable-vs-concurrent-call behaviour. +// +// Modes: +// (default) one thread tight-loops disable()/enable(); two threads hammer the target. +// --callonly control: the two caller threads hammer, NO toggling. Must never crash. +// +// Observed on Windows x64 (MSVC): default mode faults ~1 in 8 runs (~30M toggle cycles) with the +// faulting RIP inside the target function BODY (e.g. target_fn+0x4) -- a caller thread that the VEH +// trap relocation failed to rescue while the prologue was being re-patched. --callonly is 3/3 clean +// at 60-75M calls. This is a regression REPRODUCER for an upstream report, deliberately excluded from +// the CTest suite (it is probabilistic by nature); run it directly when validating an upstream fix. +// +// Run it in a loop to see the flake, e.g. (PowerShell): +// 1..16 | % { & .\coop_sh_concurrency_repro.exe; "exit=$LASTEXITCODE" } +#include +#include +#include +#include +#include + +#include + +namespace +{ +safetyhook::InlineHook g_hook; + +// A real, relocatable, never-inlined target so SafetyHook steals a genuine prologue. +__declspec(noinline) int target_fn(int x) +{ + volatile int a = x; + a = a * 3 + 7; + a ^= (a >> 2); + a += (a << 1); + return a; +} + +int detour_fn(int x) +{ + return g_hook.call(x) + 100000; // reach the original through the trampoline +} +} // namespace + +int main(int argc, char** argv) +{ + const bool call_only = argc > 1 && std::strcmp(argv[1], "--callonly") == 0; + g_hook = safetyhook::create_inline(reinterpret_cast(&target_fn), + reinterpret_cast(&detour_fn)); // created ONCE, persistent + + std::atomic stop{false}; + std::atomic calls{0}; + std::thread c1([&] { volatile int s = 0; while (!stop.load(std::memory_order_relaxed)) s = target_fn(static_cast(calls.fetch_add(1))); (void)s; }); + std::thread c2([&] { volatile int s = 0; while (!stop.load(std::memory_order_relaxed)) s = target_fn(static_cast(calls.fetch_add(1))); (void)s; }); + std::thread tog; + if (!call_only) + { + tog = std::thread([&] { + long long n = 0; + while (!stop.load(std::memory_order_relaxed)) + { + if (!g_hook.disable()) {} + if (!g_hook.enable()) {} + ++n; + } + std::printf("toggles=%lld\n", n); + }); + } + std::this_thread::sleep_for(std::chrono::seconds(3)); + stop.store(true); + c1.join(); + c2.join(); + if (tog.joinable()) tog.join(); + g_hook = {}; + std::printf("survived %lld calls (mode=%s)\n", calls.load(), call_only ? "callonly" : "toggle"); + return 0; +}