# 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`.