Harden every hook against the install/remove use-after-free
Spamming a subsystem toggle (the "Mirror video" button) could crash the game: remove_*_hooks freed a hook's shared D3D / Vulkan / IPC state immediately, while a capture detour was still mid-flight on the game's render thread -> use-after-free. Only the audio hooks had the safe-unhook drain; the video (Present/D3D9/D3D10/GL/ Vulkan) and XInput/focus/MKB hooks did not. Test-first: mock_game_test now runs an aggressive hook/unhook storm -- a separate thread thrashes every subsystem on/off while the game presents, across all backends. It crashed gl + vk (0xC0000005) and failed dx9 capture-resume before the fix. Fix (hook/src/hook_guard.hpp, DetourGate): each detour wraps its body in an RAII active-count Guard; remove_* restores the hook first (so no new detour starts), drains the in-flight detours to zero, and only then frees the shared state. Vulkan is special-cased -- the game caches hk_vkQueuePresentKHR, so removal closes an atomic capture gate (detours then pass through to the real present), drains, then frees the read-back resources. Storm now passes on every backend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
77
hook/src/hook_guard.hpp
Normal file
77
hook/src/hook_guard.hpp
Normal file
@@ -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 <atomic>
|
||||
|
||||
#include <windows.h>
|
||||
|
||||
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<int> m_active{0};
|
||||
};
|
||||
|
||||
} // namespace coop::hook
|
||||
Reference in New Issue
Block a user