From f843c56f5ba4d19ed962efc7a2d77b0dd2b88476 Mon Sep 17 00:00:00 2001 From: BlackMark Date: Tue, 23 Jun 2026 11:51:48 +0200 Subject: [PATCH] Fix stale removal comments (persistent, not destroy) + add deterministic install test - The remove_* comments still described the superseded "disable -> drain -> destroy" flow; the code keeps hooks alive (persistent) and re-enables on re-install. Updated the comments to match, and corrected the XInput note (its detours return synthesized state and never call the trampoline, so destroying its vector is safe -- unlike the trampoline-calling present/MKB/focus-cursor hooks). - hook_install_test: a fast, single-threaded contract test for hook_install.hpp -- install_inline creates the hook once and reuses the SAME trampoline across 50 install/remove cycles (never freed -> no stale-detour UAF), toggling enable/disable cleanly. Fills the guard the removed (flaky, concurrency-bound) reproducer left, with no threads so it can't flake on SafetyHook's enable/disable atomicity. x64 23/23, x86 3/3. Co-Authored-By: Claude Opus 4.8 --- hook/src/d3d9_hook.cpp | 10 ++-- hook/src/focus_spoof.cpp | 1 - hook/src/mkb_hook.cpp | 10 ++-- hook/src/opengl_hook.cpp | 10 ++-- hook/src/present_hook.cpp | 12 ++--- hook/src/xinput_hook.cpp | 8 +-- tests/CMakeLists.txt | 9 ++++ tests/hook_install_test.cpp | 105 ++++++++++++++++++++++++++++++++++++ 8 files changed, 139 insertions(+), 26 deletions(-) create mode 100644 tests/hook_install_test.cpp diff --git a/hook/src/d3d9_hook.cpp b/hook/src/d3d9_hook.cpp index 3cf6a6a..63d7bc2 100644 --- a/hook/src/d3d9_hook.cpp +++ b/hook/src/d3d9_hook.cpp @@ -367,11 +367,11 @@ bool install_d3d9_hooks(IpcClient& ipc) void remove_d3d9_hooks() { - // DISABLE (not destroy) first: restores Present's bytes under thread suspension (no new detour) - // but keeps the trampoline alive, so an in-flight detour about to call g_hk_present9.stdcall() - // (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. + // DISABLE (persistent model -- never destroy during the session): restores Present's bytes under + // thread suspension (no new detour) but keeps the trampoline alive, so an in-flight detour about + // to call g_hk_present9.stdcall() (the trampoline) never has it freed under it. Destroying (= {}) + // would free it -- a UAF the thousands/s storm hits reliably (0xC0000005). Disable -> drain -> + // leave alive (re-install re-enables; see hook_install.hpp). disable_for_removal(g_hk_present9); hook_set_installed(g_id_present9, false); g_gate.drain(); diff --git a/hook/src/focus_spoof.cpp b/hook/src/focus_spoof.cpp index 0fcf6d9..808f1b7 100644 --- a/hook/src/focus_spoof.cpp +++ b/hook/src/focus_spoof.cpp @@ -314,7 +314,6 @@ void remove_focus_spoof() // 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) { disable_for_removal(*it); diff --git a/hook/src/mkb_hook.cpp b/hook/src/mkb_hook.cpp index 54c39ab..cdc9334 100644 --- a/hook/src/mkb_hook.cpp +++ b/hook/src/mkb_hook.cpp @@ -515,11 +515,11 @@ void remove_mkb_hooks() return; } g_active.store(false, std::memory_order_release); - // Disable (restore original bytes) the inline 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 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.) + // Disable (restore original bytes) the inline hooks so no new detour starts, KEEPING the + // trampolines alive for any in-flight detour calling its trampoline (persistent model -- never + // destroyed during the session; re-install re-enables, see hook_install.hpp). Destroying would + // free a trampoline under a detour about to call it -- the present-storm UAF class. (The DI hook + // is a vtable swap: remove() restores the slot and keeps m_original valid, no trampoline either.) disable_for_removal(g_hk_async); disable_for_removal(g_hk_kbstate); disable_for_removal(g_hk_cursor); diff --git a/hook/src/opengl_hook.cpp b/hook/src/opengl_hook.cpp index 417982a..725d580 100644 --- a/hook/src/opengl_hook.cpp +++ b/hook/src/opengl_hook.cpp @@ -323,11 +323,11 @@ bool install_opengl_hooks(IpcClient& ipc) void remove_opengl_hooks() { - // DISABLE (not destroy) first: restores the SwapBuffers bytes under thread suspension (no new - // detour) but keeps the trampolines alive, so an in-flight detour about to call .stdcall() (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 swaps/s) can hit. - // Disable -> drain -> only then destroy. + // DISABLE (persistent model -- never destroy during the session): restores the SwapBuffers bytes + // under thread suspension (no new detour) but keeps the trampolines alive, so an in-flight detour + // about to call .stdcall() (the trampoline) never has it freed under it. Destroying (= {}) would + // free it -- a UAF the thousands/s storm can hit. Disable -> drain -> leave alive (re-install + // re-enables; see hook_install.hpp). disable_for_removal(g_hk_swapbuffers); disable_for_removal(g_hk_wglswap); hook_set_installed(g_id_swapbuffers, false); diff --git a/hook/src/present_hook.cpp b/hook/src/present_hook.cpp index f689bc2..3df9427 100644 --- a/hook/src/present_hook.cpp +++ b/hook/src/present_hook.cpp @@ -995,12 +995,12 @@ bool install_present_hooks(IpcClient& ipc) void remove_present_hooks() { - // DISABLE (not destroy) the hooks first: this restores the Present/Present1/ECL bytes under - // thread suspension so no NEW detour starts, but KEEPS the trampolines alive -- an in-flight - // detour about to call g_hk_present.stdcall() (the trampoline) must not have it freed under it. - // 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. + // DISABLE (persistent model -- never destroy during the session): this restores the + // Present/Present1/ECL bytes under thread suspension so no NEW detour starts, while KEEPING the + // trampolines alive -- an in-flight detour about to call g_hk_present.stdcall() (the trampoline) + // must never have it freed under it (destroying = {} would, an AV the thousands/s storm hits + // reliably). So: disable -> drain (in-flight detours finish on the live trampoline) -> leave the + // hooks alive (re-install re-enables them; see hook_install.hpp). disable_for_removal(g_hk_present); disable_for_removal(g_hk_present1); disable_for_removal(g_hk_ecl); diff --git a/hook/src/xinput_hook.cpp b/hook/src/xinput_hook.cpp index 5e75168..8eac26d 100644 --- a/hook/src/xinput_hook.cpp +++ b/hook/src/xinput_hook.cpp @@ -237,10 +237,10 @@ bool install_xinput_hooks(IpcClient& ipc) void remove_xinput_hooks() { - // Disable (restore original bytes) first so no new detour starts, but KEEP the trampolines alive - // for any in-flight detour calling its trampoline; destroy only after the drain. Clearing the - // vector here would free the trampolines immediately -- a UAF a game polling XInput at a high - // rate could hit (the same class as the mock-game present-storm crash). + // Disable (restore original bytes) first so no new detour starts, then drain in-flight detours + // before nulling the IPC pointer they read. The XInput detours return synthesized pad state and + // never call the trampoline, so (unlike the present/MKB hooks) destroying the vector after the + // drain is safe -- there's no live trampoline a stale detour could jump through. for (auto& h : g_hooks) { disable_for_removal(h); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index d745008..b8b772e 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -45,6 +45,14 @@ add_executable(detour_gate_test detour_gate_test.cpp) target_include_directories(detour_gate_test PRIVATE ${CMAKE_SOURCE_DIR}/hook/src) add_test(NAME detour_gate_test COMMAND detour_gate_test) +# Deterministic contract test for the persistent inline-hook model (hook/src/hook_install.hpp): +# install_inline creates a hook once and re-enables it on re-install, reusing the trampoline (never +# freed) so a stale detour can't UAF. Single-threaded, so it can't flake on enable/disable atomicity. +add_executable(hook_install_test hook_install_test.cpp) +target_include_directories(hook_install_test PRIVATE ${CMAKE_SOURCE_DIR}/hook/src) +target_link_libraries(hook_install_test PRIVATE safetyhook::safetyhook) +add_test(NAME hook_install_test COMMAND hook_install_test) + # Unit test for the MKB event ring (SPSC push/pop, wrap-around, full/empty). add_executable(mkb_ring_test mkb_ring_test.cpp) target_link_libraries(mkb_ring_test PRIVATE coop_common) @@ -284,6 +292,7 @@ coop_output_subdir(tests dinput_hook_test audio_ring_test detour_gate_test + hook_install_test mkb_ring_test mkb_map_test audio_mix_test diff --git a/tests/hook_install_test.cpp b/tests/hook_install_test.cpp new file mode 100644 index 0000000..14d0c08 --- /dev/null +++ b/tests/hook_install_test.cpp @@ -0,0 +1,105 @@ +// Deterministic, single-threaded contract test for the PERSISTENT inline-hook model +// (hook/src/hook_install.hpp + hook_guard.hpp): install_inline() creates a hook once and thereafter +// only re-enables it, so the trampoline is allocated ONCE and never freed across install/remove +// cycles -- which is what makes spamming a subsystem on/off safe (a stale detour can never jump +// through a freed trampoline). The mock_game_test storm covers the concurrent behaviour; this guards +// the contract with no threads, so it can't flake on SafetyHook's enable/disable atomicity. +#include +#include + +#include + +#include "hook_guard.hpp" +#include "hook_install.hpp" + +using coop::hook::DetourGate; + +namespace +{ +int g_failures = 0; +void check(bool ok, const char* what) +{ + std::printf("%s %s\n", ok ? " ok:" : "FAIL:", what); + if (!ok) + { + ++g_failures; + } +} + +safetyhook::InlineHook g_hook; +DetourGate g_gate; + +// A target with a real (relocatable) body, never inlined. +__declspec(noinline) int target_fn(int x) +{ + volatile int a = x; + a = a * 3 + 7; + a ^= (a >> 2); + a += (a << 1); + return a; +} + +constexpr int kMark = 100000; // the detour adds this so "did the detour run" is observable + +int detour_fn(int x) +{ + DetourGate::Guard guard(g_gate); + return g_hook.call(x) + kMark; // reach the original through the trampoline +} + +void install() +{ + coop::hook::install_inline(g_hook, reinterpret_cast(&target_fn), reinterpret_cast(&detour_fn)); +} +void remove() +{ + if (g_hook && !g_hook.disable()) + { + // surface, don't discard + } + g_gate.drain(); // persistent model: disable + drain, but do NOT destroy +} +} // namespace + +int main() +{ + const int probe = 5; + const int original = target_fn(probe); // unhooked baseline + + // First install: hook enabled, detour runs and reaches the original through the trampoline. + install(); + check(g_hook.enabled(), "install_inline enables the hook"); + void* const tramp1 = g_hook.original(); + check(tramp1 != nullptr, "trampoline allocated"); + check(target_fn(probe) == original + kMark, "detour runs and reaches the original via the trampoline"); + + // Remove (persistent): disable + drain, but keep the hook ALIVE. + remove(); + check(!g_hook.enabled(), "remove disables the hook"); + check(static_cast(g_hook), "remove keeps the hook alive (persistent -- not destroyed)"); + check(target_fn(probe) == original, "disabled -> the original runs"); + + // Re-install: SAME trampoline reused (not recreated/freed), detour runs again. + install(); + check(g_hook.enabled(), "re-install re-enables the hook"); + check(g_hook.original() == tramp1, "re-install REUSES the same trampoline (never freed -> no UAF)"); + check(target_fn(probe) == original + kMark, "detour runs again after re-enable"); + + // Many cycles: the trampoline must never change and enable/disable must toggle cleanly. + bool reuse = true, toggles = true; + for (int i = 0; i < 50; ++i) + { + remove(); + toggles = toggles && !g_hook.enabled(); + install(); + reuse = reuse && (g_hook.original() == tramp1); + toggles = toggles && g_hook.enabled(); + } + check(reuse, "trampoline stays identical across 50 install/remove cycles (no churn, no leak)"); + check(toggles, "enable/disable toggles cleanly across the cycles"); + check(target_fn(probe) == original + kMark, "hook still works after the cycles"); + + g_hook = {}; // teardown (process is single-threaded here, so destroying is fine) + std::printf(g_failures == 0 ? "PASS hook_install_test\n" : "FAILED hook_install_test (%d)\n", g_failures); + return g_failures == 0 ? 0 : 1; +}