Make inline-hook install AND remove safe to spam
The uncapped, input-polling mock_game_test storm (thousands of presents/s, now
also driving the input/focus/MKB hooks) drove out a family of install/remove races
the slow vsync'd mock had masked. Fixes (hook/src/hook_install.hpp + hook_guard.hpp):
- Persistent hooks. The old model created a hook on install and DESTROYED it on
remove (= {}), freeing the trampoline; a detour about to call it (.stdcall) then
hit freed memory -> 0xC0000005. drain() can't fully close that window (a thread
can be inside the detour but not past its Guard ctor). So hooks are now created
ONCE and only enable()/disable()d across install/remove cycles -- never destroyed
during the session -- so a stale detour always calls a live trampoline (disabled,
it just runs the original). Reused, so no churn and no leak. remove_* therefore
disable()s + drain()s but does not destroy; install guards check .enabled().
- Install race. create_inline() enables the hook before the result is move-assigned
into the global the detour reads; a call landing in the detour mid-assign reads a
torn hook -> AV. install_inline() creates StartDisabled, assigns, then enable()s.
- drain() Sleep(1)s BEFORE each zero-check, so a thread that entered the detour but
hasn't reached its Guard registers before we conclude zero.
- Focus: publish g_orig_proc before SetWindowLongPtr activates the subclass (and
subclass_proc falls back to DefWindowProc if null); and disable the focus-query
hooks in reverse install order, because GetForegroundWindow shares user32 code
with GetActiveWindow (keep GFW hooked until GAW is unhooked).
- disable()/enable() [[nodiscard]] results are handled (logged), not (void)-discarded.
Storm now survives on every backend across repeated runs.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
60
README.md
60
README.md
@@ -661,32 +661,40 @@ Non-obvious things that cost time and constrain the design:
|
||||
churn from re-creating the probe each toggle (build it once, keep it, only swap vtable
|
||||
slots). **Silently silencing/zeroing a buffer whose true size you only guessed is an
|
||||
over-write, not just an over-read** — clamp the read, but don't write what you can't size.
|
||||
- **Every removable hook needs the same safe-unhook drain, not just audio.** The audio hooks
|
||||
learned to restore the vtable slot first and *drain in-flight detours* before tearing down the
|
||||
shared state they read; the video (Present / D3D9 / D3D10 / OpenGL / Vulkan) and the
|
||||
XInput / focus / MKB hooks did not — `remove_*` freed the hook's shared D3D device / keyed-mutex
|
||||
texture (or the Vulkan read-back resources, or the IPC pointer) *immediately*. So spamming a
|
||||
subsystem toggle (the "Mirror video" button) freed that state while a capture detour was still
|
||||
mid-flight on the game's render thread → use-after-free → the game crashed (Brotato, on its
|
||||
OpenGL path; reproduced across every backend by the `mock_game_test` storm). The generalised fix
|
||||
(`hook/src/hook_guard.hpp`, `DetourGate`): every detour wraps its body in an RAII active-count
|
||||
`Guard`; `remove_*` (1) **disables** the hook so **no new detour can start** — for a SafetyHook
|
||||
inline hook that's `disable()` (restore the original bytes under thread suspension) **not** `= {}`,
|
||||
because destroying frees the trampoline immediately and an in-flight detour about to call it
|
||||
(`.stdcall()`) then uses freed memory; or, for the focus WNDPROC subclass, restore the window proc
|
||||
— then (2) `drain()`s the active count to zero, and only **then** (3) destroys the hook (frees the
|
||||
trampoline) and frees the shared state. Two subtleties the *uncapped* mock storm (thousands of
|
||||
presents/s) exposed that the old vsync'd one (tens/s) masked: **(a)** the original code did `= {}`
|
||||
before draining → trampoline UAF (now disable → drain → destroy, keeping the trampoline alive
|
||||
across the drain); **(b)** `drain()` returned the instant the count read zero, but a thread can be
|
||||
*inside* the detour yet not have reached its `Guard` constructor (the few-instruction prologue is
|
||||
unguarded), so it `Sleep(1)`s **before** each zero-check to let such a thread register. **Vulkan is
|
||||
the exception**: the game caches our `hk_vkQueuePresentKHR` pointer at resolution time and keeps
|
||||
calling it even after the GPA hook is reset, so a reset can't stop new detours — instead removal
|
||||
closes an atomic **capture gate** first (the detour then passes straight through to the real present
|
||||
without touching the read-back state), drains, and only then frees. The drain is bounded (~400 ms)
|
||||
so a wedged game thread can't hang the worker; detours are micro- to milliseconds, so it returns
|
||||
almost immediately.
|
||||
- **Spamming a subsystem on/off must always be safe — install *and* remove.** Toggling a subsystem
|
||||
install/removes its hooks while the game keeps calling the hooked API; the `mock_game_test` storm
|
||||
(uncapped backends presenting at thousands/s, the mock now polling XInput / GetAsyncKeyState /
|
||||
GetKeyboardState / GetForegroundWindow so the input/focus/MKB hooks are exercised too) drove out a
|
||||
whole family of races. The fixes:
|
||||
- **Removal — drain in-flight detours, never free the trampoline.** Each detour wraps its body in an
|
||||
RAII `DetourGate::Guard` (active-count). `remove_*` `disable()`s the SafetyHook inline hook
|
||||
(restores the original bytes under thread suspension; **not** `= {}`, which frees the trampoline a
|
||||
detour may be about to call), then `drain()`s the count to zero, then frees the shared state. Two
|
||||
drain subtleties: it `Sleep(1)`s **before** each zero-check, because a thread can be *inside* the
|
||||
detour but not yet past its `Guard` constructor (the prologue is unguarded); and even after the
|
||||
drain we **keep the hooks alive (disabled), never destroying them during the session** — they're
|
||||
*persistent*, re-enabled on re-install (`hook/src/hook_install.hpp`) — so a stale detour can never
|
||||
jump through a freed trampoline (it runs the original instead). The trampoline is allocated once
|
||||
and reused, so no churn and no leak.
|
||||
- **Install — populate the global before the detour can fire.** `create_inline()` enables the hook
|
||||
(patches the bytes) *before* the result is move-assigned into the global the detour reads to reach
|
||||
the trampoline; a call landing in the detour during that assign reads a torn hook → AV.
|
||||
`install_inline()` instead creates `StartDisabled`, lets the global be assigned, and only then
|
||||
`enable()`s it (and reuses the existing hook on re-install — the persistent model).
|
||||
- **Focus is special twice.** The WNDPROC subclass publishes `g_orig_proc` *before*
|
||||
`SetWindowLongPtr` activates it (and `subclass_proc` falls back to `DefWindowProc` if it's still
|
||||
null), and `GetForegroundWindow`/`GetActiveWindow` share user32 code — so the focus query hooks
|
||||
are **disabled in reverse install order** (GetForegroundWindow stays hooked until GetActiveWindow
|
||||
is unhooked), keeping the invariant "GetActiveWindow hooked ⇒ GetForegroundWindow hooked" so a
|
||||
call never lands in a half-patched shared region.
|
||||
- **Vulkan is the exception to disable-stops-new-detours.** The game caches our
|
||||
`hk_vkQueuePresentKHR` pointer at resolution time and keeps calling it after the GPA hook is
|
||||
disabled, so removal closes an atomic **capture gate** first (the detour then passes straight
|
||||
through to the saved real present without touching the read-back state), drains, then frees.
|
||||
The drain is bounded (~400 ms) so a wedged game thread can't hang the worker. Guarded by
|
||||
`mock_game_test`'s storm (every backend) and the deterministic `detour_gate_test`. (This started as
|
||||
the audio hooks' restore-then-drain idea; the video / XInput / focus / MKB hooks needed all of the
|
||||
above to be spam-safe — the original "Mirror video" spam crashed Brotato's OpenGL path.)
|
||||
- **Capturing at `Present` decouples the mirror from DWM composition.** The hook copies
|
||||
the backbuffer inside the game's `Present`, which the game issues at its true render
|
||||
rate regardless of how DWM composites that *window*. So an unfocused game window can
|
||||
|
||||
Reference in New Issue
Block a user