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 <noreply@anthropic.com>
This commit is contained in:
2026-06-23 11:51:48 +02:00
parent 8a183902ad
commit f843c56f5b
8 changed files with 139 additions and 26 deletions

View File

@@ -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();

View File

@@ -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);

View File

@@ -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);

View File

@@ -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);

View File

@@ -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);

View File

@@ -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);