BlackMark/CoopAllTheThings
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Harden hook removal: disable -> drain -> destroy, and drain settles first

Browse Source 
Uncapping the mock game (next commit) turned mock_game_test's hook/unhook storm
into a real stress test (thousands of presents/s instead of tens), which reliably
crashed the game on remove (0xC0000005) for dx9/dx11/dx12. Two races the slow
vsync'd mock had masked:

1. Trampoline use-after-free. remove_*_hooks did `hook = {}` (destroy) BEFORE the
   DetourGate drain. Destroying a SafetyHook InlineHook frees its trampoline
   immediately, but an in-flight detour about to call the original via .stdcall()
   (the trampoline) then used freed memory. Fix: disable() first (restores the
   original bytes under thread suspension, but KEEPS the trampoline alive) -> drain
   -> only then destroy. Applied to present/d3d9/opengl/vk/xinput/mkb/focus.

2. Entry-window race in DetourGate::drain(). It returned the instant the active
   count read zero, but a thread can be inside the detour yet not have reached its
   Guard constructor (the prologue is unguarded), so the count reads zero while a
   detour is about to run -- and the freed state is then used. Fix: Sleep(1) BEFORE
   each zero-check; with the hook disabled no new detour starts, so any
   already-entered thread registers within that window. This alone fixed dx11 (the
   highest present rate, ~11000/s, which hit the window every storm).

Audio is unaffected (it uses vtable swaps, which keep a real original pointer, not
a trampoline). Full suite 21/21, and the storm now survives on every backend.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
BlackMark
2026-06-23 09:44:42 +02:00
parent c480dfe152
commit 958c355126
8 changed files with 79 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

18
hook/src/present_hook.cpp
View File

@@ -994,16 +994,22 @@ 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 = {};
// 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.
(void)g_hk_present.disable();
(void)g_hk_present1.disable();
(void)g_hk_ecl.disable();
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 = {};
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;