Audio: fix five hook/unhook concurrency + over-write bugs

Found by the mock-game capture/audio/hook stress test (toggling the audio
subsystem while a game renders):

1. Guessed-stream silence over-WRITE: hk_ReleaseBuffer zeroed num_frames *
   guessed_block bytes, but a guess can be larger than the real per-frame size
   (e.g. an 8ch device guess for a 2ch game), so the memset wrote past the real
   buffer into adjacent audio memory -> intermittent access violation in the
   game. Fix: capture but do NOT silence a guessed stream (it stays audible --
   echo); only an exact/override format, whose frame size is known, gets the
   no-echo silence.
2. VtableHook::remove nulled m_original, racing an in-flight detour into a null
   call -> keep it valid (the original function stays mapped).
3. g_ipc was a non-atomic pointer read on the hot path while unhook nulled it
   (TOCTOU) -> make it atomic, load once.
4. Stale GetBuffer/ReleaseBuffer pairing across a toggle -> epoch-stamp the
   GetBuffer and only capture in the same hooked epoch.
5. COM-object churn: re-creating the probe client every enable raced AudioSes ->
   build the probe once, keep it across toggles (only swap vtable slots);
   release on detach (shutdown_audio_hooks). Plus drain in-flight detours before
   tearing down state.

Stress test: 0 crashes in many repeated runs (was ~50%). Guessed streams now
echo (the no-echo path is reached via an exact/auto-attach format or override).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-06-22 05:21:17 +02:00
parent 549b91a9ce
commit e2562ac63c
4 changed files with 195 additions and 79 deletions

View File

@@ -74,9 +74,14 @@ and covers anything the hooked path doesn't (Vulkan, D3D9 — see Roadmap).
client's format — so they're *assumed* to match the device mix format. That's correct
for the common case (engines render stereo float, matching the endpoint, differing
only in rate). A game rendering a *different* channel count or bit depth than the
device would be mirrored with the wrong layout (garbled audio) on the hooked path —
but never an over-read/crash (a `VirtualQuery` clamp guards the copy), and the
loopback fallback is always format-correct. The Audio panel shows each stream's
device is mirrored with the wrong layout (garbled audio) on the hooked path, but never
an over-read/crash: a guessed stream is **captured but not silenced** (so it stays
audible locally — an echo), because zeroing it could over-*write* past the real buffer
(zeroing 8-channel-worth into a 2-channel buffer corrupts adjacent audio memory). Only
an **exact / override** format gets the no-echo silence (its frame size is known), so
the no-echo experience comes from an early (auto-attach) exact format or an operator
override. The loopback fallback is always format-correct. The Audio panel shows each
stream's
format provenance (*known* / *measuring* / *measured rate* / *low-confidence* /
*override*) so the assumption is visible, and (under Debug details) lets the operator
**re-measure** the rate or **override** the format when the guess is wrong. Overrides
@@ -378,6 +383,18 @@ Non-obvious things that cost time and constrain the design:
simulation is brittle. A tiny debug-only command channel (`-DCOOP_TEST_HARNESS`, file-
based) that calls the *same* code the buttons do — and replies with state — makes UI
validation deterministic and scriptable, and is compiled out of the shipped product.
- **A capture-style stress test against a frame-numbered mock game is worth a lot.** An
animated game that encodes its frame number in the pixels lets a test assert the mirror
shows a *monotonic, advancing* sequence (the bar for "no dropped / stale / out-of-order
frames"), and toggling subsystems while it renders flushes out concurrency bugs. This
one caught **five** real audio races: a `memset` over-*write* past a guessed stream's
real buffer (zeroing 8ch into a 2ch buffer corrupts adjacent audio memory → crash; the
fix: capture but don't silence a guessed stream), a null call through a vtable hook's
`original` after unhook (keep it valid), a non-atomic `g_ipc` TOCTOU, a stale
GetBuffer/ReleaseBuffer pairing across a hook toggle (epoch-stamp it), and COM-object
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.
- **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