CoopAllTheThings/docs/safetyhook-concurrency.md

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:

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. VirtualProtects 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_assigns), 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.