diff --git a/README.md b/README.md index fe77723..3f59cf2 100644 --- a/README.md +++ b/README.md @@ -109,20 +109,6 @@ default** and covers anything the hooked path doesn't. ### Current Tasks -- **Injection hardening — provoke, then fix, the hook install/remove races.** Rapidly - toggling the **"Mirror video"** button has crashed a real game (Brotato): flipping the - Present-hook video subsystem on/off in quick succession races hook install/remove against - the game's Present thread and the host's capture path. Per the test-first rule, *first make - the crash reproducible in a test* — make `mock_game_test`'s hook/unhook stress against - `coop_mock_game` far more aggressive: tight, repeated subsystem toggles (especially video) - while the game is actively presenting, driven from a separate thread so the toggles interleave - with the Present detour, sustained long enough that an unguarded race is near-certain to - fault, and across all backends (DX11/12/9/10/GL/Vulkan). Then fix what it surfaces — the - install/remove path is the same safe-unhook problem the **audio** hooks already solved (bump - an epoch on each toggle, restore the vtable/inline first so no new detour starts, then drain - in-flight detours before tearing down shared state); apply the equivalent guard to the - Present / D3D9 / D3D10 / OpenGL / Vulkan hooks. - - **Determine a pre-existing stream's audio format by *correlating* the two capture paths, instead of guessing.** When we attach to an already-running game we never saw its `IAudioClient::Initialize`, so the render-hook assumes the device mix format and measures @@ -297,9 +283,13 @@ ctest --test-dir build -C Debug --output-on-failure relaunch banner). It launches the game at several **audio formats** (44100/48000/96000, PCM + float) and asserts the hook measures each one's rate through the full inject path, then injects with audio + video, checks both stream, cycles the audio subsystem - off/on (hook/unhook stress), and confirms the game never crashes and capture resumes. This suite - drove out five real audio races (see Lessons learned). Skips cleanly without a D3D11 / Vulkan - device. + off/on (hook/unhook stress), and confirms the game never crashes and capture resumes. It then + runs an **aggressive hook/unhook storm** — a separate thread thrashes *every* subsystem on/off as + fast as the worker reconciles while the game is actively presenting, across **all** backends — to + catch an install/remove race that frees a hook's shared D3D / Vulkan state under a live capture + detour (the use-after-free that crashed a real game when its "Mirror video" button was spammed). + This suite drove out five real audio races plus the cross-backend unhook race (see Lessons + learned). Skips cleanly without a D3D11 / Vulkan device. ### Debugging the hooks against a real game @@ -588,6 +578,24 @@ 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) restores the hook so **no new detour can start** — reset the inline hook + (SafetyHook's mutex-guarded call wrappers make any in-flight trampoline call safe), or, for the + focus WNDPROC subclass, restore the window proc — then (2) `drain()`s the active count to zero, + and only **then** (3) frees the shared state. **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. - **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 fb17cc9..d4227c3 100644 --- a/hook/src/d3d9_hook.cpp +++ b/hook/src/d3d9_hook.cpp @@ -14,6 +14,7 @@ #include "coop/protocol.hpp" #include "coop/shared_memory.hpp" #include "debug_log.hpp" +#include "hook_guard.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -22,6 +23,8 @@ namespace coop::hook namespace { +DetourGate g_gate; // drains in-flight Present detours before remove frees the shared D3D state + // IDirect3DDevice9 vtable: IUnknown 0-2, then the device methods. Present is index 17 // (TestCooperativeLevel 3, GetAvailableTextureMem 4, EvictManagedResources 5, GetDirect3D 6, // GetDeviceCaps 7, GetDisplayMode 8, GetCreationParameters 9, SetCursorProperties 10, @@ -260,6 +263,7 @@ void capture_d3d9(IDirect3DDevice9* dev) HRESULT STDMETHODCALLTYPE hk_Present9(IDirect3DDevice9* dev, const RECT* src, const RECT* dst, HWND wnd, const RGNDATA* dirty) { + DetourGate::Guard guard(g_gate); // keep the shared D3D state alive for this whole detour hook_note_call(g_id_present9); g_presents.fetch_add(1, std::memory_order_relaxed); if (g_ipc != nullptr) @@ -362,8 +366,11 @@ bool install_d3d9_hooks(IpcClient& ipc) void remove_d3d9_hooks() { + // Reset the hook first (restores Present's bytes under thread suspension -> no new detour), + // then drain any Present detour still in-flight before freeing the D3D state it reads (UAF). g_hk_present9 = {}; hook_set_installed(g_id_present9, false); + g_gate.drain(); release_shared(); release_sysmem(); if (g_ctx != nullptr) diff --git a/hook/src/focus_spoof.cpp b/hook/src/focus_spoof.cpp index b38a6c7..688de3f 100644 --- a/hook/src/focus_spoof.cpp +++ b/hook/src/focus_spoof.cpp @@ -6,6 +6,7 @@ #include +#include "hook_guard.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -14,6 +15,8 @@ namespace coop::hook namespace { +DetourGate g_gate; // drains in-flight focus / WNDPROC detours before remove nulls their state + HWND g_game_hwnd = nullptr; WNDPROC g_orig_proc = nullptr; bool g_unicode = true; @@ -74,6 +77,7 @@ HWND find_main_window(DWORD pid) // Replacement window procedure: convince the game it is never deactivated. LRESULT CALLBACK subclass_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam) { + DetourGate::Guard guard(g_gate); // keep g_orig_proc / g_unicode valid for this whole dispatch switch (msg) { case WM_ACTIVATE: @@ -103,6 +107,7 @@ LRESULT CALLBACK subclass_proc(HWND hwnd, UINT msg, WPARAM wparam, LPARAM lparam HWND WINAPI hk_GetForegroundWindow() { + DetourGate::Guard guard(g_gate); hook_note_call(g_id_foreground); if (g_focus_ipc != nullptr) { @@ -113,6 +118,7 @@ HWND WINAPI hk_GetForegroundWindow() HWND WINAPI hk_GetActiveWindow() { + DetourGate::Guard guard(g_gate); hook_note_call(g_id_active); if (g_focus_ipc != nullptr) { @@ -123,6 +129,7 @@ HWND WINAPI hk_GetActiveWindow() HWND WINAPI hk_GetFocus() { + DetourGate::Guard guard(g_gate); hook_note_call(g_id_focus); if (g_focus_ipc != nullptr) { @@ -135,6 +142,7 @@ HWND WINAPI hk_GetFocus() // the operator's mouse isn't trapped; otherwise honor the game's clip. BOOL WINAPI hk_ClipCursor(const RECT* rect) { + DetourGate::Guard guard(g_gate); hook_note_call(g_id_clipcursor); const bool allow = g_focus_ipc != nullptr && g_focus_ipc->cursor_clip_allowed(); return g_hk_clipcursor.stdcall(allow ? rect : nullptr); @@ -144,6 +152,7 @@ BOOL WINAPI hk_ClipCursor(const RECT* rect) // mouse can move freely (e.g. to reach the overlay); pass it through when clipping. BOOL WINAPI hk_SetCursorPos(int x, int y) { + DetourGate::Guard guard(g_gate); hook_note_call(g_id_setcursorpos); const bool allow = g_focus_ipc != nullptr && g_focus_ipc->cursor_clip_allowed(); if (!allow) @@ -290,6 +299,11 @@ void remove_focus_spoof() hook_set_installed(g_id_wndproc, false); hook_set_installed(g_id_clipcursor, false); hook_set_installed(g_id_setcursorpos, false); + // The WNDPROC is restored and the inline hooks reset above, so no NEW detour can start. Drain + // any focus / WNDPROC detour still in-flight on the game's window thread before nulling the + // 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(); 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 new file mode 100644 index 0000000..ef6eb4b --- /dev/null +++ b/hook/src/hook_guard.hpp @@ -0,0 +1,77 @@ +// Safe-unhook coordination between a hook's removal (worker thread) and the detours still +// running on the game's own threads (render / audio / window / input). +// +// The hazard: remove_*_hooks restores the hook and then frees the shared state the detour +// touches (D3D device/context, the keyed-mutex texture, Vulkan read-back resources, the IPC +// pointer). A capture detour mid-flight on the game's render thread then uses freed memory -> +// use-after-free -> the game crashes (the "spamming Mirror video crashed Brotato" bug). +// +// The fix mirrors the audio hooks' epoch+drain pattern, generalised for inline hooks: +// 1. Restore/disable the hook FIRST so no NEW detour can start. For a SafetyHook inline hook +// that's `hook = {}` (reset): it restores the original bytes under thread suspension, and +// its mutex-guarded call wrappers make any in-flight trampoline call safe. For a hook the +// game reaches by a cached pointer (Vulkan present, the WNDPROC subclass) it's clearing an +// atomic gate / restoring the window proc. +// 2. drain() -- wait (bounded) for detour BODIES already running to finish, since the reset +// above does NOT wait for the part of the detour that runs before it calls the trampoline. +// 3. Only THEN free the shared state the detour was reading. +// +// Each detour wraps its whole body in a DetourGate::Guard (an RAII active-count). drain() spins +// until that count reaches zero. Detours are microseconds to a few milliseconds, so this returns +// almost immediately; the bound keeps a wedged game thread from hanging the worker. +#pragma once + +#include + +#include + +namespace coop::hook +{ + +class DetourGate +{ +public: + // RAII: marks a detour body as in-flight for as long as it's on the stack. + class Guard + { + public: + explicit Guard(DetourGate& gate) : m_gate(gate) + { + m_gate.m_active.fetch_add(1, std::memory_order_acq_rel); + } + ~Guard() + { + m_gate.m_active.fetch_sub(1, std::memory_order_acq_rel); + } + Guard(const Guard&) = delete; + Guard& operator=(const Guard&) = delete; + + private: + DetourGate& m_gate; + }; + + // Wait (bounded ~400 ms) for all in-flight detours to finish. Call AFTER the hook is + // restored / the gate is closed, so no new detour can start -- otherwise this may never + // reach zero on a busy render thread. + void drain() + { + for (int spins = 0; spins < 400; ++spins) + { + if (m_active.load(std::memory_order_acquire) == 0) + { + return; + } + Sleep(1); + } + } + + int active() const + { + return m_active.load(std::memory_order_acquire); + } + +private: + std::atomic m_active{0}; +}; + +} // namespace coop::hook diff --git a/hook/src/mkb_hook.cpp b/hook/src/mkb_hook.cpp index ab8aff6..5fbdf2c 100644 --- a/hook/src/mkb_hook.cpp +++ b/hook/src/mkb_hook.cpp @@ -6,6 +6,7 @@ #include +#include "hook_guard.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -14,6 +15,8 @@ namespace coop::hook namespace { +DetourGate g_gate; // drains in-flight polling detours before remove tears the hooks down + // Synthesized input state the polling hooks report. Written by the worker thread // (mkb_pump), read by the game's thread inside the detours -> all atomic. std::atomic g_active{false}; @@ -37,6 +40,7 @@ bool g_installed = false; SHORT WINAPI hk_GetAsyncKeyState(int vkey) { + DetourGate::Guard guard(g_gate); const SHORT orig = g_hk_async.stdcall(vkey); if (g_active.load(std::memory_order_relaxed) && vkey >= 0 && vkey < 256 && g_key_down[vkey].load(std::memory_order_relaxed)) @@ -48,6 +52,7 @@ SHORT WINAPI hk_GetAsyncKeyState(int vkey) BOOL WINAPI hk_GetKeyboardState(PBYTE state) { + DetourGate::Guard guard(g_gate); const BOOL r = g_hk_kbstate.stdcall(state); if (r && state != nullptr && g_active.load(std::memory_order_relaxed)) { @@ -64,6 +69,7 @@ BOOL WINAPI hk_GetKeyboardState(PBYTE state) BOOL WINAPI hk_GetCursorPos(LPPOINT pt) { + DetourGate::Guard guard(g_gate); const BOOL r = g_hk_cursor.stdcall(pt); if (g_active.load(std::memory_order_relaxed) && g_have_cursor.load(std::memory_order_relaxed) && pt != nullptr) { @@ -257,9 +263,10 @@ void remove_mkb_hooks() return; } g_active.store(false, std::memory_order_release); - g_hk_async = {}; // InlineHook destructor restores the original bytes + g_hk_async = {}; // InlineHook destructor restores the original bytes -> no new detour starts g_hk_kbstate = {}; g_hk_cursor = {}; + g_gate.drain(); // wait for any in-flight polling detour before clearing the synth state for (int vk = 0; vk < 256; ++vk) { g_key_down[vk].store(false, std::memory_order_relaxed); // no stuck keys diff --git a/hook/src/opengl_hook.cpp b/hook/src/opengl_hook.cpp index 40473c5..532b22e 100644 --- a/hook/src/opengl_hook.cpp +++ b/hook/src/opengl_hook.cpp @@ -13,6 +13,7 @@ #include "coop/protocol.hpp" #include "coop/shared_memory.hpp" #include "debug_log.hpp" +#include "hook_guard.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -21,6 +22,8 @@ namespace coop::hook namespace { +DetourGate g_gate; // drains in-flight swap detours before remove frees the shared D3D state + // OpenGL enums (avoid pulling in / linking opengl32 at load time). constexpr unsigned GL_RGBA = 0x1908; constexpr unsigned GL_UNSIGNED_BYTE = 0x1401; @@ -234,6 +237,7 @@ void capture_gl(HDC hdc) BOOL WINAPI hk_SwapBuffers(HDC hdc) { + DetourGate::Guard guard(g_gate); // keep the shared D3D state alive for this whole detour hook_note_call(g_id_swapbuffers); g_swaps.fetch_add(1, std::memory_order_relaxed); const bool outer = !t_in_swap; @@ -256,6 +260,7 @@ BOOL WINAPI hk_SwapBuffers(HDC hdc) BOOL WINAPI hk_wglSwapBuffers(HDC hdc) { + DetourGate::Guard guard(g_gate); // keep the shared D3D state alive for this whole detour hook_note_call(g_id_wglswap); g_swaps.fetch_add(1, std::memory_order_relaxed); const bool outer = !t_in_swap; @@ -317,10 +322,14 @@ bool install_opengl_hooks(IpcClient& ipc) void remove_opengl_hooks() { + // Reset the hooks first (restores the original SwapBuffers bytes under thread suspension, so + // no NEW detour starts), then drain any swap detour still in-flight on the render thread + // BEFORE freeing the D3D state it reads -- otherwise the detour uses freed memory (UAF). g_hk_swapbuffers = {}; g_hk_wglswap = {}; hook_set_installed(g_id_swapbuffers, false); hook_set_installed(g_id_wglswap, false); + g_gate.drain(); release_shared(); if (g_ctx != nullptr) { diff --git a/hook/src/present_hook.cpp b/hook/src/present_hook.cpp index c053440..1d30e07 100644 --- a/hook/src/present_hook.cpp +++ b/hook/src/present_hook.cpp @@ -16,6 +16,7 @@ #include "coop/shared_memory.hpp" #include "debug_log.hpp" +#include "hook_guard.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -24,6 +25,8 @@ namespace coop::hook namespace { +DetourGate g_gate; // drains in-flight Present/ECL detours before remove frees the shared state + // IDXGISwapChain vtable layout (frozen ABI). IUnknown 0..2, IDXGIObject 3..6, // IDXGIDeviceSubObject 7, then IDXGISwapChain: Present = 8, GetBuffer = 9. // IDXGISwapChain1 adds methods after IDXGISwapChain (18 methods, 0..17), so @@ -755,6 +758,7 @@ void capture_backbuffer(IDXGISwapChain* sc) void STDMETHODCALLTYPE hk_ExecuteCommandLists(ID3D12CommandQueue* queue, UINT num_lists, ID3D12CommandList* const* lists) { + DetourGate::Guard guard(g_gate); // keep g_present_queue/g_hk_ecl alive for this detour // Record the graphics queue; compute/copy queues never present, so skip them and // keep the last DIRECT one (the present queue on single-graphics-queue engines). if (queue != nullptr && queue->GetDesc().Type == D3D12_COMMAND_LIST_TYPE_DIRECT) @@ -806,6 +810,7 @@ void* grab_execute_command_lists_address() HRESULT STDMETHODCALLTYPE hk_Present(IDXGISwapChain* sc, UINT sync_interval, UINT flags) { + DetourGate::Guard guard(g_gate); // keep the shared texture / On12 bridge alive for this detour hook_note_call(g_id_present); g_present_calls.fetch_add(1, std::memory_order_relaxed); if (g_ipc != nullptr) @@ -836,6 +841,7 @@ HRESULT STDMETHODCALLTYPE hk_Present(IDXGISwapChain* sc, UINT sync_interval, UIN HRESULT STDMETHODCALLTYPE hk_Present1(IDXGISwapChain1* sc, UINT sync_interval, UINT flags, const DXGI_PRESENT_PARAMETERS* params) { + DetourGate::Guard guard(g_gate); // keep the shared texture / On12 bridge alive for this detour hook_note_call(g_id_present1); g_present_calls.fetch_add(1, std::memory_order_relaxed); if (g_ipc != nullptr) @@ -978,12 +984,16 @@ bool install_present_hooks(IpcClient& ipc) void remove_present_hooks() { + // Reset the hooks first (restores Present/Present1/ECL bytes under thread suspension, so no + // NEW detour starts), then drain any detour still in-flight on the render thread BEFORE + // freeing the shared texture / On12 bridge / present queue it reads -- otherwise UAF. g_hk_present = {}; g_hk_present1 = {}; 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_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 fa09a38..416a65c 100644 --- a/hook/src/vk_hook.cpp +++ b/hook/src/vk_hook.cpp @@ -18,6 +18,7 @@ #include "coop/protocol.hpp" #include "coop/shared_memory.hpp" #include "debug_log.hpp" +#include "hook_guard.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -26,6 +27,13 @@ namespace coop::hook namespace { +DetourGate g_gate; // drains in-flight present/create detours before remove frees the Vulkan/D3D state +// Capture gate. Unlike the other backends, the game caches our hk_vkQueuePresentKHR pointer at +// resolution time, so it keeps calling our detour even after the GPA hook is reset -- resetting the +// hook can't stop new detours. So removal instead closes this gate (detours then pass straight +// through to the real present), drains in-flight detours, and only THEN frees the read-back state. +std::atomic g_capture_enabled{false}; + IpcClient* g_ipc = nullptr; unsigned long g_pid = 0; @@ -447,6 +455,7 @@ const SwapInfo* find_swap(VkSwapchainKHR sc) VKAPI_ATTR VkResult VKAPI_CALL hk_vkQueuePresentKHR(VkQueue queue, const VkPresentInfoKHR* pPresentInfo) { + DetourGate::Guard guard(g_gate); // keep the read-back resources alive for this whole detour hook_note_call(g_id_present); g_presents.fetch_add(1, std::memory_order_relaxed); if (g_ipc != nullptr) @@ -454,8 +463,11 @@ VKAPI_ATTR VkResult VKAPI_CALL hk_vkQueuePresentKHR(VkQueue queue, const VkPrese g_ipc->note_present(); } - // Capture only the simple, common single-swapchain present; pass anything else through. - if (g_device != VK_NULL_HANDLE && pPresentInfo != nullptr && pPresentInfo->swapchainCount == 1) + // Capture only the simple, common single-swapchain present; pass anything else through. The + // gate lets removal stop capture (and pass through to the real present) before it frees the + // read-back state, even though the game keeps calling this cached detour pointer. + if (g_capture_enabled.load(std::memory_order_acquire) && g_device != VK_NULL_HANDLE && + pPresentInfo != nullptr && pPresentInfo->swapchainCount == 1) { const SwapInfo* s = find_swap(pPresentInfo->pSwapchains[0]); if (s != nullptr && pPresentInfo->pImageIndices[0] < s->images.size()) @@ -479,6 +491,7 @@ VKAPI_ATTR VkResult VKAPI_CALL hk_vkQueuePresentKHR(VkQueue queue, const VkPrese VKAPI_ATTR VkResult VKAPI_CALL hk_vkCreateSwapchainKHR(VkDevice device, const VkSwapchainCreateInfoKHR* ci, const VkAllocationCallbacks* alloc, VkSwapchainKHR* out) { + DetourGate::Guard guard(g_gate); // keep g_swaps stable while remove may be clearing it const VkResult r = g_real_create_swapchain(device, ci, alloc, out); if (r == VK_SUCCESS && out != nullptr && g_fns.GetSwapchainImagesKHR != nullptr) { @@ -537,6 +550,7 @@ void load_device_fns(VkDevice device) VKAPI_ATTR VkResult VKAPI_CALL hk_vkCreateDevice(VkPhysicalDevice phys, const VkDeviceCreateInfo* ci, const VkAllocationCallbacks* alloc, VkDevice* out) { + DetourGate::Guard guard(g_gate); const VkResult r = g_real_create_device(phys, ci, alloc, out); if (r == VK_SUCCESS && out != nullptr && g_device == VK_NULL_HANDLE) // track the first device { @@ -548,6 +562,9 @@ VKAPI_ATTR VkResult VKAPI_CALL hk_vkCreateDevice(VkPhysicalDevice phys, const Vk reinterpret_cast(g_real_gdpa(*out, "vkCreateSwapchainKHR")); g_real_present = reinterpret_cast(g_real_gdpa(*out, "vkQueuePresentKHR")); load_device_fns(*out); + // Arm capture only once every real_* pointer + g_fns is populated (release pairs with the + // present detour's acquire load of the gate, so it sees a fully-initialised state). + g_capture_enabled.store(true, std::memory_order_release); logf("vk: device created (qfam=%u) -- present capture armed", g_qfam); } return r; @@ -647,6 +664,15 @@ bool install_vk_hooks(IpcClient& ipc) void remove_vk_hooks() { + // Close the capture gate and reset the GPA hook FIRST (so no new detour captures and future + // resolutions aren't intercepted), then drain any present/create detour still in-flight on the + // game's thread BEFORE freeing the read-back resources it reads. The game keeps calling our + // cached present detour, but with the gate closed it now passes straight through to the real + // present without touching freed state. + g_capture_enabled.store(false, std::memory_order_release); + g_hk_gipa = {}; + g_gate.drain(); + if (g_device != VK_NULL_HANDLE && g_fns.DeviceWaitIdle != nullptr) { g_fns.DeviceWaitIdle(g_device); @@ -667,7 +693,6 @@ void remove_vk_hooks() g_pool = VK_NULL_HANDLE; } } - g_hk_gipa = {}; hook_set_installed(g_id_present, false); release_shared(); if (g_d3d_ctx != nullptr) diff --git a/hook/src/xinput_hook.cpp b/hook/src/xinput_hook.cpp index fe9c0b7..132151a 100644 --- a/hook/src/xinput_hook.cpp +++ b/hook/src/xinput_hook.cpp @@ -9,6 +9,7 @@ #include +#include "hook_guard.hpp" #include "hook_registry.hpp" namespace coop::hook @@ -17,6 +18,8 @@ namespace coop::hook namespace { +DetourGate g_gate; // drains in-flight XInput detours before remove nulls the IPC pointer + // XInput guide-button bit, reported only by the undocumented ordinal-100 // XInputGetStateEx that many games use. Mirrors how Steam/x360ce expose it. constexpr std::uint16_t kGuideButton = 0x0400; @@ -99,18 +102,21 @@ DWORD query_state(DWORD user_index, XINPUT_STATE* state, bool keep_guide) DWORD WINAPI hk_XInputGetState(DWORD user_index, XINPUT_STATE* state) { + DetourGate::Guard guard(g_gate); // keep g_ipc valid for this whole detour hook_note_call(g_id_getstate); return query_state(user_index, state, /*keep_guide=*/false); } DWORD WINAPI hk_XInputGetStateEx(DWORD user_index, XINPUT_STATE* state) { + DetourGate::Guard guard(g_gate); // keep g_ipc valid for this whole detour hook_note_call(g_id_getstateex); return query_state(user_index, state, /*keep_guide=*/true); } DWORD WINAPI hk_XInputGetCapabilities(DWORD user_index, DWORD /*flags*/, XINPUT_CAPABILITIES* caps) { + DetourGate::Guard guard(g_gate); // keep g_ipc valid for this whole detour hook_note_call(g_id_getcaps); if (caps == nullptr || user_index >= kMaxPads) { @@ -147,6 +153,7 @@ DWORD WINAPI hk_XInputGetCapabilities(DWORD user_index, DWORD /*flags*/, XINPUT_ // Still report success so the game's logic is happy. DWORD WINAPI hk_XInputSetState(DWORD user_index, XINPUT_VIBRATION* vibration) { + DetourGate::Guard guard(g_gate); // keep g_ipc valid for this whole detour hook_note_call(g_id_setstate); if (user_index >= kMaxPads || !g_cache[user_index].connected) { @@ -227,11 +234,12 @@ bool install_xinput_hooks(IpcClient& ipc) void remove_xinput_hooks() { - g_hooks.clear(); // InlineHook destructor restores the original bytes + g_hooks.clear(); // InlineHook destructor restores the original bytes -> no new detour starts hook_set_installed(g_id_getstate, false); hook_set_installed(g_id_getstateex, false); hook_set_installed(g_id_getcaps, false); hook_set_installed(g_id_setstate, false); + g_gate.drain(); // wait for any in-flight detour before nulling the IPC pointer it reads if (g_ipc != nullptr) { g_ipc->mark_detached(); diff --git a/tests/mock_game_test.cpp b/tests/mock_game_test.cpp index a04cb00..a3719fd 100644 --- a/tests/mock_game_test.cpp +++ b/tests/mock_game_test.cpp @@ -9,10 +9,12 @@ // cycles the audio subsystem off/on a few times (hook/unhook stress), and confirms the // game never freezes/crashes. Skips cleanly without a D3D11 device. #include +#include #include #include #include #include +#include #include #include @@ -755,6 +757,172 @@ void test_av_and_hook_cycles(ID3D11Device* device) game.kill(); } + +// Aggressively toggle every injected subsystem on/off from a separate thread while the game is +// actively presenting, to provoke an unsafe hook install/remove race. The known failure mode: +// remove_*_hooks frees the hook's shared D3D state (device / context / keyed-mutex texture, and +// the Vulkan read-back resources) while a capture detour on the game's render thread is still +// using it -> use-after-free -> the game crashes. This is the "spamming the Mirror video button +// crashed Brotato" bug; the gentle, audio-only cycles in test_av_and_hook_cycles never exercised +// the video teardown, so they missed it. We storm ALL subsystems (especially video) across every +// backend, then confirm the game never crashed/froze and that capture resumes. vk_early uses the +// suspended-launch + early-inject path (Vulkan caches its present pointer at init, so a late inject +// can't hook it). +void test_hook_storm(const char* backend, ID3D11Device* device, bool vk_early) +{ + std::printf("== hook/unhook storm: %s ==\n", backend); + + std::wstring wbackend; + for (const char* p = backend; *p != '\0'; ++p) // backend names are ASCII + { + wbackend.push_back(static_cast(*p)); + } + + PROCESS_INFORMATION pi{}; + STARTUPINFOW si{}; + si.cb = sizeof(si); + const std::wstring exe = tool_path(L"coop_mock_game.exe"); + std::wstring cmd = L"\"" + exe + L"\" " + wbackend + L" 60"; + if (vk_early) + { + SetEnvironmentVariableW(L"COOP_MOCK_VK_EARLY", L"1"); + } + const DWORD launch_flags = vk_early ? CREATE_SUSPENDED : 0; + const BOOL launched = + CreateProcessW(exe.c_str(), cmd.data(), nullptr, nullptr, FALSE, launch_flags, nullptr, nullptr, &si, &pi); + if (vk_early) + { + SetEnvironmentVariableW(L"COOP_MOCK_VK_EARLY", nullptr); + } + if (!launched) + { + check(false, "launch mock game (storm)"); + return; + } + auto alive = [&] { return WaitForSingleObject(pi.hProcess, 0) == WAIT_TIMEOUT; }; + auto exit_code = [&] { + DWORD c = 0; + GetExitCodeProcess(pi.hProcess, &c); + return c; + }; + auto cleanup = [&] { + TerminateProcess(pi.hProcess, 0); + WaitForSingleObject(pi.hProcess, 2000); + CloseHandle(pi.hThread); + CloseHandle(pi.hProcess); + }; + + SharedMemory shm; + SharedBlock* block = make_ipc(shm, pi.dwProcessId, /*disabled=*/0); // all subsystems on + SharedMemory ring_shm; + AudioRingHeader* ring = nullptr; + if (ring_shm.create(audio_ring_name(pi.dwProcessId), audio_ring_total_size(kAudioRingCapacity))) + { + ring = ring_shm.as(); + audio_ring_init(*ring, kAudioRingCapacity); + ring->capture_enabled.store(1, std::memory_order_release); + } + const bool injected = block != nullptr && inject_retry(pi.dwProcessId); + if (vk_early) + { + ResumeThread(pi.hThread); // the mock loads Vulkan + waits, then renders + } + if (!injected) + { + if (vk_early && !alive() && exit_code() == 2) + { + std::printf(" Vulkan unavailable -- skipping storm\n"); + } + else + { + check(false, "inject mock game (storm)"); + } + cleanup(); + return; + } + + Sleep(vk_early ? 2500 : 1000); // let the hook attach + the game start presenting + if (vk_early && !alive() && exit_code() == 2) + { + std::printf(" Vulkan unavailable -- skipping storm\n"); + cleanup(); + return; + } + + const std::uint32_t hb_start = block->status.heartbeat.load(std::memory_order_relaxed); + + // Storm thread: flip every subsystem on/off as fast as it can while the game presents, so a + // remove lands while a capture detour is mid-flight on the game's render thread. + std::atomic stop{false}; + std::thread storm([&] { + bool off = false; + while (!stop.load(std::memory_order_relaxed)) + { + off = !off; + for (std::uint32_t s = 0; s < HookSubsys_Count; ++s) + { + block->control.subsystem_disabled[s].store(off ? 1u : 0u, std::memory_order_release); + } + Sleep(60); + } + }); + + bool crashed = false; + for (int i = 0; i < 170 && !crashed; ++i) // ~10 s of storming + { + Sleep(60); + if (!alive()) + { + crashed = true; + } + } + stop.store(true, std::memory_order_relaxed); + storm.join(); + + if (crashed) + { + std::printf(" game CRASHED during the storm (exit 0x%08lX)\n", exit_code()); + } + check(!crashed, "game survived the hook/unhook storm (no crash)"); + if (crashed) + { + cleanup(); + return; + } + + // Re-enable everything and confirm the game is still alive + the hook still beating. + for (std::uint32_t s = 0; s < HookSubsys_Count; ++s) + { + block->control.subsystem_disabled[s].store(0, std::memory_order_release); + } + Sleep(600); + check(alive(), "game alive after the storm settles"); + check(block->status.heartbeat.load(std::memory_order_relaxed) > hb_start, + "hook heartbeat advanced across the storm (no freeze)"); + + // Capture must resume (the rehook works end-to-end). Vulkan can't re-arm after a toggle (its + // present pointer was cached at init), so only assert resume for the other backends. + if (!vk_early) + { + SharedTextureSource src; + src.init(device); + const std::uint64_t frames0 = src.frames_copied(); + bool advanced = false; + for (int i = 0; i < 80 && alive(); ++i) // ~4 s + { + Sleep(50); + const VideoShareView share = read_video_share(block); + if (src.update(share, pi.dwProcessId) && src.frames_copied() > frames0 + 3) + { + advanced = true; + break; + } + } + check(advanced, "video capture resumed after the storm"); + } + + cleanup(); +} } // namespace int main() @@ -787,6 +955,16 @@ int main() test_av_and_hook_cycles(device); + // Aggressive hook/unhook storm across every backend: a separate thread thrashes every + // subsystem on/off while the game presents, to catch an unsafe install/remove race (the + // "spamming Mirror video crashed Brotato" use-after-free). vk uses the early-load path. + test_hook_storm("gl", device, /*vk_early=*/false); + test_hook_storm("dx9", device, /*vk_early=*/false); + test_hook_storm("dx10", device, /*vk_early=*/false); + test_hook_storm("dx11", device, /*vk_early=*/false); + test_hook_storm("dx12", device, /*vk_early=*/false); + test_hook_storm("vk", device, /*vk_early=*/true); + device->Release(); kill_stray_mock_games(); // belt-and-suspenders: ensure nothing is left running