diff --git a/README.md b/README.md index 1c5b4ef..e26621b 100644 --- a/README.md +++ b/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 diff --git a/hook/src/d3d9_hook.cpp b/hook/src/d3d9_hook.cpp index 846a89f..3cf6a6a 100644 --- a/hook/src/d3d9_hook.cpp +++ b/hook/src/d3d9_hook.cpp @@ -15,6 +15,7 @@ #include "coop/shared_memory.hpp" #include "debug_log.hpp" #include "hook_guard.hpp" +#include "hook_install.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -345,9 +346,9 @@ bool install_d3d9_hooks(IpcClient& ipc) { g_ipc = &ipc; g_pid = GetCurrentProcessId(); - if (g_hk_present9) + if (g_hk_present9.enabled()) { - return true; // already installed + return true; // already installed (persistent hook; re-install below re-enables it) } g_id_present9 = hook_register("IDirect3DDevice9::Present", HookSubsys_Video); g_unsupported_logged = false; @@ -358,7 +359,7 @@ bool install_d3d9_hooks(IpcClient& ipc) hook_set_installed(g_id_present9, false); // not a D3D9 game (or no probe device) return false; } - g_hk_present9 = safetyhook::create_inline(present, reinterpret_cast(&hk_Present9)); + install_inline(g_hk_present9, present, &hk_Present9); hook_set_installed(g_id_present9, static_cast(g_hk_present9)); logf("install_d3d9_hooks: Present=%p hooked=%d", present, static_cast(g_hk_present9) ? 1 : 0); return static_cast(g_hk_present9); @@ -371,10 +372,11 @@ void remove_d3d9_hooks() // (the trampoline) doesn't have it freed under it. Destroying (= {}) before the drain frees the // trampoline immediately -- a UAF the uncapped mock-game storm (thousands of presents/s) hits // reliably (0xC0000005). Disable -> drain -> only then destroy. - (void)g_hk_present9.disable(); + disable_for_removal(g_hk_present9); hook_set_installed(g_id_present9, false); g_gate.drain(); - g_hk_present9 = {}; // no detour in-flight or able to start now -> safe to free the trampoline + // Persistent hook: keep g_hk_present9 ALIVE (disabled) so a stale detour's trampoline call is + // never freed -- re-install re-enables it (see hook_install.hpp). release_shared(); release_sysmem(); if (g_ctx != nullptr) diff --git a/hook/src/focus_spoof.cpp b/hook/src/focus_spoof.cpp index a653465..0fcf6d9 100644 --- a/hook/src/focus_spoof.cpp +++ b/hook/src/focus_spoof.cpp @@ -7,6 +7,7 @@ #include #include "hook_guard.hpp" +#include "hook_install.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -101,8 +102,15 @@ LRESULT CALLBACK subclass_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam default: break; } - return g_unicode ? CallWindowProcW(g_orig_proc, hwnd, msg, wparam, lparam) - : CallWindowProcA(g_orig_proc, hwnd, msg, wparam, lparam); + // Read g_orig_proc once; if the subclass is live but the original isn't published yet (the tiny + // install/remove window), fall back to DefWindowProc rather than call through a null pointer. + const WNDPROC orig = g_orig_proc; + if (orig == nullptr) + { + return g_unicode ? DefWindowProcW(hwnd, msg, wparam, lparam) : DefWindowProcA(hwnd, msg, wparam, lparam); + } + return g_unicode ? CallWindowProcW(orig, hwnd, msg, wparam, lparam) + : CallWindowProcA(orig, hwnd, msg, wparam, lparam); } HWND WINAPI hk_GetForegroundWindow() @@ -166,7 +174,8 @@ void hook_export(HMODULE module, const char* name, void* detour, int registry_id { if (void* target = reinterpret_cast(GetProcAddress(module, name))) { - g_focus_hooks.emplace_back(safetyhook::create_inline(target, detour)); + g_focus_hooks.emplace_back(); + install_inline(g_focus_hooks.back(), target, detour); // assign-then-enable (no install race) hook_set_installed(registry_id, true); } } @@ -197,12 +206,20 @@ bool install_focus_spoof(IpcClient& ipc) g_game_hwnd = hwnd; g_unicode = IsWindowUnicode(hwnd) != FALSE; - // Replacing GWLP_WNDPROC from another thread is safe (the new proc runs on - // the window's own thread); match A/W so CallWindowProc translates correctly. - const LONG_PTR replaced = g_unicode - ? SetWindowLongPtrW(hwnd, GWLP_WNDPROC, reinterpret_cast(&subclass_proc)) - : SetWindowLongPtrA(hwnd, GWLP_WNDPROC, reinterpret_cast(&subclass_proc)); - g_orig_proc = reinterpret_cast(replaced); + // Publish g_orig_proc BEFORE activating the subclass, so a message that dispatches the instant the + // subclass goes live finds a valid original (not the null/stale value from a prior install cycle) + // -- the WNDPROC analogue of the inline-hook install race. Replacing GWLP_WNDPROC from another + // thread is safe (the new proc runs on the window's own thread); match A/W for CallWindowProc. + g_orig_proc = g_unicode ? reinterpret_cast(GetWindowLongPtrW(hwnd, GWLP_WNDPROC)) + : reinterpret_cast(GetWindowLongPtrA(hwnd, GWLP_WNDPROC)); + if (g_unicode) + { + SetWindowLongPtrW(hwnd, GWLP_WNDPROC, reinterpret_cast(&subclass_proc)); + } + else + { + SetWindowLongPtrA(hwnd, GWLP_WNDPROC, reinterpret_cast(&subclass_proc)); + } hook_set_installed(g_id_wndproc, true); if (HMODULE user32 = GetModuleHandleW(L"user32.dll")) @@ -214,12 +231,12 @@ bool install_focus_spoof(IpcClient& ipc) if (void* clip = reinterpret_cast(GetProcAddress(user32, "ClipCursor"))) { - g_hk_clipcursor = safetyhook::create_inline(clip, reinterpret_cast(&hk_ClipCursor)); + install_inline(g_hk_clipcursor, clip, &hk_ClipCursor); hook_set_installed(g_id_clipcursor, static_cast(g_hk_clipcursor)); } if (void* setpos = reinterpret_cast(GetProcAddress(user32, "SetCursorPos"))) { - g_hk_setcursorpos = safetyhook::create_inline(setpos, reinterpret_cast(&hk_SetCursorPos)); + install_inline(g_hk_setcursorpos, setpos, &hk_SetCursorPos); hook_set_installed(g_id_setcursorpos, static_cast(g_hk_setcursorpos)); } } @@ -289,17 +306,21 @@ void remove_focus_spoof() SetWindowLongPtrA(g_game_hwnd, GWLP_WNDPROC, reinterpret_cast(g_orig_proc)); } } - // Disable (restore original bytes) the inline focus hooks first so no new detour starts, but KEEP - // the trampolines alive for any in-flight detour calling its trampoline; destroy only after the - // drain. Destroying before the drain frees a trampoline under a detour about to call it (the same - // UAF class as the present-storm crash). The WNDPROC subclass uses a saved g_orig_proc pointer - // (restored above), not a trampoline, so it has nothing to free early. - for (auto& h : g_focus_hooks) + // Disable the inline focus hooks in REVERSE install order. GetForegroundWindow/GetActiveWindow + // share user32 code (the real GetForegroundWindow's path runs through GetActiveWindow's body), so + // GetActiveWindow's inline patch corrupts that shared code. GetForegroundWindow is installed + // first, so disabling in reverse unhooks GetActiveWindow (restoring the shared bytes) while + // GetForegroundWindow is still hooked -- its detour returns g_game_hwnd and never reaches the + // patched bytes. The reverse of the enable order (GFW first) keeps the invariant "GetActiveWindow + // hooked => GetForegroundWindow hooked" across the whole install/remove cycle, so a call never + // lands in a half-patched shared region (the intermittent storm crash: GetActiveWindow+0x8). + // Disable (not destroy) keeps the trampolines alive for any in-flight detour; destroy after drain. + for (auto it = g_focus_hooks.rbegin(); it != g_focus_hooks.rend(); ++it) { - (void)h.disable(); + disable_for_removal(*it); } - (void)g_hk_clipcursor.disable(); - (void)g_hk_setcursorpos.disable(); + disable_for_removal(g_hk_clipcursor); + disable_for_removal(g_hk_setcursorpos); ClipCursor(nullptr); // leave the cursor free when the spoof is removed hook_set_installed(g_id_foreground, false); hook_set_installed(g_id_active, false); @@ -312,9 +333,11 @@ void remove_focus_spoof() // state they read (g_orig_proc / g_game_hwnd / g_focus_ipc) -- otherwise a dispatch mid-flight // could call a null original WNDPROC or a dangling IPC pointer. g_gate.drain(); - g_focus_hooks.clear(); // no detour in-flight or able to start now -> safe to free the trampolines - g_hk_clipcursor = {}; - g_hk_setcursorpos = {}; + // The focus-query hooks (GetForegroundWindow/GetActiveWindow/GetFocus) return g_game_hwnd and + // never call the trampoline, so destroying them is safe; recreate on re-install. The cursor hooks + // DO call the trampoline, so keep them ALIVE (disabled) -- persistent, re-enabled on re-install + // (see hook_install.hpp) -- so a stale detour never hits a freed trampoline. + g_focus_hooks.clear(); if (g_focus_ipc != nullptr) { g_focus_ipc->mark_focus_spoof(false, 0); diff --git a/hook/src/hook_guard.hpp b/hook/src/hook_guard.hpp index 67a8631..0dd5bb8 100644 --- a/hook/src/hook_guard.hpp +++ b/hook/src/hook_guard.hpp @@ -82,4 +82,19 @@ private: std::atomic m_active{0}; }; +// Disable a SafetyHook inline hook as the first step of removal (restore the original bytes, keep the +// trampoline alive for the drain). disable() returns a [[nodiscard]] std::expected: a failure leaves +// the function patched while removal goes on to free the trampoline -- a use-after-free -- so it must +// not be silently discarded. There's no clean recovery mid-unhook, but surface it so it's +// diagnosable. Templated so this header needn't depend on SafetyHook (the type is deduced at the +// call site, where it's already included). +template +void disable_for_removal(InlineHook& hook) +{ + if (!hook.disable()) + { + OutputDebugStringA("coop: SafetyHook InlineHook::disable() failed during removal -- unhook may be unsafe\n"); + } +} + } // namespace coop::hook diff --git a/hook/src/hook_install.hpp b/hook/src/hook_install.hpp new file mode 100644 index 0000000..3e9133a --- /dev/null +++ b/hook/src/hook_install.hpp @@ -0,0 +1,51 @@ +// Safe INSTALL of a SafetyHook inline hook -- the symmetric partner of hook_guard.hpp's safe removal. +// +// Two problems this avoids, both surfaced by mock_game_test's hook/unhook storm (a game polling a +// hooked API at thousands/s while the subsystem is toggled on/off every 60 ms): +// +// 1. Enable-before-assign. create_inline() builds the hook AND enables it (patches the target's bytes +// to jump to the detour), THEN 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 global -> AV. +// So we create StartDisabled, let dst be populated, and only THEN enable(). +// +// 2. Trampoline use-after-free on re-install. The old model recreated the hook on every install and +// DESTROYED it on every remove (= {}), which frees the trampoline. A detour that has entered but +// not yet reached its DetourGate::Guard (the unguarded prologue) can then call a freed trampoline +// -> AV at a garbage address. drain() narrows that window but can't fully close it under heavy +// preemption. So instead we treat hooks as PERSISTENT: create each once and thereafter only +// enable()/disable() it across install/remove cycles. The trampoline is allocated once and never +// freed during the session, so a stale detour always calls a live trampoline (which, when +// disabled, simply runs the original). No churn, no leak (it's reused), no UAF. Removal therefore +// disable()s the hook (and drains) but does NOT destroy it (see hook_guard.hpp). +#pragma once + +#include + +#include + +namespace coop::hook +{ + +// Arm `detour` over `target` in `dst`: create it once (StartDisabled) if empty, then enable. Calling +// this again after a remove just re-enables the SAME hook (no recreate -> the trampoline is never +// freed). enable()'s [[nodiscard]] result is surfaced, not discarded; enabling an already-enabled +// hook is a no-op success. +inline void install_inline(safetyhook::InlineHook& dst, void* target, void* detour) +{ + if (!dst) // create only the first time; reuse across enable/disable cycles + { + dst = safetyhook::create_inline(target, detour, safetyhook::InlineHook::StartDisabled); + } + if (dst && !dst.enable()) + { + OutputDebugStringA("coop: SafetyHook InlineHook::enable() failed during install\n"); + } +} + +template +void install_inline(safetyhook::InlineHook& dst, T target, D detour) +{ + install_inline(dst, reinterpret_cast(target), reinterpret_cast(detour)); +} + +} // namespace coop::hook diff --git a/hook/src/mkb_hook.cpp b/hook/src/mkb_hook.cpp index f3d063b..54c39ab 100644 --- a/hook/src/mkb_hook.cpp +++ b/hook/src/mkb_hook.cpp @@ -10,6 +10,7 @@ #include #include "hook_guard.hpp" +#include "hook_install.hpp" #include "hook_registry.hpp" #include "vtable_hook.hpp" @@ -406,7 +407,7 @@ void install_user32_hook(HMODULE user32, const char* name, void* detour, safetyh } if (void* target = reinterpret_cast(GetProcAddress(user32, name))) { - slot = safetyhook::create_inline(target, detour); + install_inline(slot, target, detour); // StartDisabled -> assign -> enable (no install race) if (slot) { hook_set_installed(id, true); @@ -519,16 +520,15 @@ void remove_mkb_hooks() // drain. Destroying before the drain frees the trampoline under a detour about to call it -- the // same UAF class as the present-storm crash. (The DI hook is a vtable swap: remove() restores the // slot and keeps m_original valid, so it has no trampoline to free early.) - (void)g_hk_async.disable(); - (void)g_hk_kbstate.disable(); - (void)g_hk_cursor.disable(); - (void)g_hk_getrawinputdata.disable(); + disable_for_removal(g_hk_async); + disable_for_removal(g_hk_kbstate); + disable_for_removal(g_hk_cursor); + disable_for_removal(g_hk_getrawinputdata); g_vh_di_getstate.remove(); // restore the DI GetDeviceState slot (probe kept alive for re-enable) g_gate.drain(); // wait for any in-flight polling / DI / raw detour before clearing state - g_hk_async = {}; // no detour in-flight or able to start now -> safe to free the trampolines - g_hk_kbstate = {}; - g_hk_cursor = {}; - g_hk_getrawinputdata = {}; + // Persistent hooks: keep g_hk_async/kbstate/cursor/getrawinputdata ALIVE (disabled), so a stale + // detour's trampoline call (these detours DO call the trampoline) is never freed -- re-install + // re-enables them (see hook_install.hpp). hook_set_installed(g_id_di_getstate, false); hook_set_installed(g_id_rawinput, false); for (int vk = 0; vk < 256; ++vk) diff --git a/hook/src/opengl_hook.cpp b/hook/src/opengl_hook.cpp index 0938e0f..417982a 100644 --- a/hook/src/opengl_hook.cpp +++ b/hook/src/opengl_hook.cpp @@ -14,6 +14,7 @@ #include "coop/shared_memory.hpp" #include "debug_log.hpp" #include "hook_guard.hpp" +#include "hook_install.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -287,9 +288,9 @@ bool install_opengl_hooks(IpcClient& ipc) { g_ipc = &ipc; g_pid = GetCurrentProcessId(); - if (g_hk_swapbuffers || g_hk_wglswap) + if (g_hk_swapbuffers.enabled() || g_hk_wglswap.enabled()) { - return true; // already installed + return true; // already installed (persistent hooks; re-install below re-enables them) } g_id_swapbuffers = hook_register("SwapBuffers", HookSubsys_Video); @@ -301,7 +302,7 @@ bool install_opengl_hooks(IpcClient& ipc) { if (void* fn = reinterpret_cast(GetProcAddress(gdi, "SwapBuffers"))) { - g_hk_swapbuffers = safetyhook::create_inline(fn, reinterpret_cast(&hk_SwapBuffers)); + install_inline(g_hk_swapbuffers, fn, &hk_SwapBuffers); } } // opengl32!wglSwapBuffers if OpenGL is already loaded. @@ -309,7 +310,7 @@ bool install_opengl_hooks(IpcClient& ipc) { if (void* fn = reinterpret_cast(GetProcAddress(gl, "wglSwapBuffers"))) { - g_hk_wglswap = safetyhook::create_inline(fn, reinterpret_cast(&hk_wglSwapBuffers)); + install_inline(g_hk_wglswap, fn, &hk_wglSwapBuffers); } } @@ -327,13 +328,13 @@ void remove_opengl_hooks() // trampoline) doesn't have it freed under it. Destroying (= {}) before the drain frees the // trampoline immediately -- a UAF the uncapped mock-game storm (thousands of swaps/s) can hit. // Disable -> drain -> only then destroy. - (void)g_hk_swapbuffers.disable(); - (void)g_hk_wglswap.disable(); + disable_for_removal(g_hk_swapbuffers); + disable_for_removal(g_hk_wglswap); hook_set_installed(g_id_swapbuffers, false); hook_set_installed(g_id_wglswap, false); g_gate.drain(); - g_hk_swapbuffers = {}; // no detour in-flight or able to start now -> safe to free the trampolines - g_hk_wglswap = {}; + // Persistent hooks: keep them ALIVE (disabled) so a stale detour's trampoline call is never freed + // -- re-install re-enables them (see hook_install.hpp). release_shared(); if (g_ctx != nullptr) { diff --git a/hook/src/present_hook.cpp b/hook/src/present_hook.cpp index 5073b65..f689bc2 100644 --- a/hook/src/present_hook.cpp +++ b/hook/src/present_hook.cpp @@ -17,6 +17,7 @@ #include "coop/shared_memory.hpp" #include "debug_log.hpp" #include "hook_guard.hpp" +#include "hook_install.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -946,9 +947,9 @@ bool install_present_hooks(IpcClient& ipc) { g_ipc = &ipc; g_pid = GetCurrentProcessId(); - if (g_hk_present) + if (g_hk_present.enabled()) { - return true; // already installed + return true; // already installed (persistent hook; the re-install path below re-enables it) } g_id_present = hook_register("IDXGISwapChain::Present", HookSubsys_Video); @@ -963,10 +964,10 @@ bool install_present_hooks(IpcClient& ipc) hook_set_installed(g_id_present1, false); return false; } - g_hk_present = safetyhook::create_inline(present, reinterpret_cast(&hk_Present)); + install_inline(g_hk_present, present, &hk_Present); if (present1 != nullptr) { - g_hk_present1 = safetyhook::create_inline(present1, reinterpret_cast(&hk_Present1)); + install_inline(g_hk_present1, present1, &hk_Present1); } g_unsupported_logged = false; hook_set_installed(g_id_present, static_cast(g_hk_present)); @@ -980,7 +981,7 @@ bool install_present_hooks(IpcClient& ipc) void* ecl = grab_execute_command_lists_address(); if (ecl != nullptr) { - g_hk_ecl = safetyhook::create_inline(ecl, reinterpret_cast(&hk_ExecuteCommandLists)); + install_inline(g_hk_ecl, ecl, &hk_ExecuteCommandLists); hook_set_installed(g_id_ecl, static_cast(g_hk_ecl)); logf("install_present_hooks: d3d12 ExecuteCommandLists=%p hooked=%d", ecl, static_cast(g_hk_ecl) ? 1 : 0); @@ -1000,16 +1001,15 @@ void remove_present_hooks() // Destroying here (= {}) frees the trampoline immediately; at a few hundred presents/s that race // was rarely hit, but the uncapped mock-game storm (thousands/s) hits it reliably (0xC0000005). // So: disable -> drain (in-flight detours finish on the live trampoline) -> only THEN destroy. - (void)g_hk_present.disable(); - (void)g_hk_present1.disable(); - (void)g_hk_ecl.disable(); + disable_for_removal(g_hk_present); + disable_for_removal(g_hk_present1); + disable_for_removal(g_hk_ecl); hook_set_installed(g_id_present, false); hook_set_installed(g_id_present1, false); hook_set_installed(g_id_ecl, false); g_gate.drain(); - g_hk_present = {}; // no detour is in-flight or can start now -> freeing the trampoline is safe - g_hk_present1 = {}; - g_hk_ecl = {}; + // Persistent hooks: keep g_hk_present/present1/ecl ALIVE (disabled), so the trampoline a stale + // detour may still call is never freed -- re-install re-enables them (see hook_install.hpp). g_present_queue.store(nullptr, std::memory_order_relaxed); g_logged_presents_n = 0; // let a fresh injection re-log the present pattern g_logged_swapchains_n = 0; diff --git a/hook/src/vk_hook.cpp b/hook/src/vk_hook.cpp index e350de5..a11b99f 100644 --- a/hook/src/vk_hook.cpp +++ b/hook/src/vk_hook.cpp @@ -18,6 +18,7 @@ #include "coop/shared_memory.hpp" #include "debug_log.hpp" #include "hook_guard.hpp" +#include "hook_install.hpp" #include "hook_registry.hpp" #include "vk_capture.hpp" @@ -305,9 +306,9 @@ bool install_vk_hooks(IpcClient& ipc) { g_ipc = &ipc; g_pid = GetCurrentProcessId(); - if (g_hk_gipa) + if (g_hk_gipa.enabled()) { - return true; // already installed + return true; // already installed (persistent hook; re-install below re-enables it) } if (g_id_present < 0) { @@ -320,7 +321,7 @@ bool install_vk_hooks(IpcClient& ipc) return false; // vulkan-1.dll not loaded; caller can retry once the game loads it } g_unsupported_logged = false; - g_hk_gipa = safetyhook::create_inline(gipa, reinterpret_cast(&hk_vkGetInstanceProcAddr)); + install_inline(g_hk_gipa, gipa, &hk_vkGetInstanceProcAddr); g_install_tick = GetTickCount64(); hook_set_installed(g_id_present, static_cast(g_hk_gipa)); logf("install_vk_hooks: vkGetInstanceProcAddr=%p hooked=%d", gipa, static_cast(g_hk_gipa) ? 1 : 0); @@ -334,12 +335,12 @@ void remove_vk_hooks() // game's thread BEFORE shutting the capture down. The game keeps calling our cached present // detour, but with the gate closed it now passes straight through to the real present. g_capture_enabled.store(false, std::memory_order_release); - // Disable (not destroy) the GIPA hook first so the trampoline stays alive for any in-flight - // resolution detour calling real_gipa() (the trampoline); destroy only after the drain. (The - // present detour already passes through to the saved g_real_present, so it's unaffected.) - (void)g_hk_gipa.disable(); + // Disable (not destroy) the GIPA hook: keep the trampoline alive for any in-flight resolution + // detour calling real_gipa(), and -- persistent model -- keep it alive after the drain too so a + // stale detour never hits a freed trampoline; re-install re-enables it. (The present detour passes + // through to the saved g_real_present, so it's unaffected either way.) + disable_for_removal(g_hk_gipa); g_gate.drain(); - g_hk_gipa = {}; g_cap.shutdown(); // joins the reaper, drains the device, frees the read-back resources diff --git a/hook/src/xinput_hook.cpp b/hook/src/xinput_hook.cpp index 67db30c..5e75168 100644 --- a/hook/src/xinput_hook.cpp +++ b/hook/src/xinput_hook.cpp @@ -10,6 +10,7 @@ #include #include "hook_guard.hpp" +#include "hook_install.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -174,7 +175,8 @@ void hook_export(HMODULE module, const char* name, void* detour, int registry_id } if (void* target = reinterpret_cast(GetProcAddress(module, name))) { - g_hooks.emplace_back(safetyhook::create_inline(target, detour)); + g_hooks.emplace_back(); + install_inline(g_hooks.back(), target, detour); // assign-then-enable (no install race) hook_set_installed(registry_id, true); } } @@ -187,7 +189,8 @@ void hook_ordinal(HMODULE module, WORD ordinal, void* detour, int registry_id) } if (void* target = reinterpret_cast(GetProcAddress(module, MAKEINTRESOURCEA(ordinal)))) { - g_hooks.emplace_back(safetyhook::create_inline(target, detour)); + g_hooks.emplace_back(); + install_inline(g_hooks.back(), target, detour); // assign-then-enable (no install race) hook_set_installed(registry_id, true); } } @@ -240,7 +243,7 @@ void remove_xinput_hooks() // rate could hit (the same class as the mock-game present-storm crash). for (auto& h : g_hooks) { - (void)h.disable(); + disable_for_removal(h); } hook_set_installed(g_id_getstate, false); hook_set_installed(g_id_getstateex, false);