From a40afdd86dddcbf9841e48f7a647cc3d30a4d557 Mon Sep 17 00:00:00 2001 From: chrg6 Date: Fri, 13 Nov 2026 19:37:45 -0500 Subject: [PATCH] Log a splitter framing fault met while the update is applied as the rest of the section left out, not as a refused update. The review found that audio_mixer.c splitters_walk used AUDIO_REFUSE for a framing fault in both of its walks. In the apply walk (audio_mixer_update_routing, after audio_mixer_routing_check's walk passed) such a fault can only come from the game rewriting its input during its own RequestUpdate. The update is taken by then: it is answered 0, its voices and effects and the splitter records before the fault are applied, and the mix section is applied after it. Yet the log said "RequestUpdate refused with 0x5299 ...; the renderer goes on with the last update it took". SPL_FAULT now refuses the update in the check walk as before, and in the apply walk logs the fault with "the rest of the splitter section is left out". The same double read of the mix section by audio_mixer_mix_layout_check and audio_mixer_update_routing stays as it is; its comment now says the check holds for the apply only because the game does not rewrite its input during its own RequestUpdate. docs/building.md's audio_params_check paragraph names the new case and is wrapped again at 120 columns. Not reproduced before the change: the game's sending thread waits in the IPC call, so neither case has a known way to happen. A new case in tests/audio_params_check calls audio_mixer_update_routing with a splitter section whose first destination record's magic is broken after a valid update. It checks that the splitter info before the fault and the mix section are applied, that the destination record is not, and that the one log line names the rest of the section left out. It failed on the code before this change ("RequestUpdate refused with 0x5299: splitter destination 0 ... not SNDD"). audio_params_check (27 refused updates, 11 left-out records, 38 effects and sinks, both strict runs), check_content, a compile of every runtime file and -Wall -Wextra on the audio files and cfg.c with no warnings passed. Co-Authored-By: Claude Opus 5.5 --- docs/building.md | 34 ++++++++++++----------- runtime/audio_mixer.c | 56 ++++++++++++++++++++++---------------- tests/audio_params_check.c | 30 +++++++++++++++++++- 3 files changed, 80 insertions(+), 40 deletions(-) diff --git a/docs/building.md b/docs/building.md index 144f0bac..2a44535c 100644 --- a/docs/building.md +++ b/docs/building.md @@ -517,22 +517,24 @@ not queued; the silence before the first prefill and across a stop is not owed, up. The render thread's own timing (`runtime/audren.c` `render_thread`) is not built by it. No game files and no audio device. -A change to what the audio renderer does with values the game sends (`runtime/audren.c` `update_check`, -`request_update` and `audio_bad`; `runtime/audio_mixer.c` `splitters_walk`, `audio_mixer_routing_check`, -`audio_mixer_mix_layout_check`, `audio_mixer_update_effects` and the render-pass effects; `runtime/audio_effects.c` -`audio_effect_invalid`; `runtime/audio_reverb.c` `audio_reverb_invalid`) passes `python tests\audio_params_check.py`. -It builds `audren.c` and `audio_mixer.c` with the rest of the runtime stubbed and the real effect DSPs linked, and -sends hand-built RequestUpdates through `renderer_handler`. An update whose sections do not match the renderer (sizes, -magic words, overruns, the output buffer, a mix layout with a negative count or more buffers than the renderer has, a -splitter id run whose length wraps in 32 bits) answers 0x5299, a result the SDK hands back to the game (it aborts on -any result but 0x5299, 0x5499 and 0x5699), and writes no output, changes no renderer state and places no C buffers. A channel, voice or splitter -record with an id the renderer does not have, a wave buffer index of 4, a filter that is not a finite number and a -changed mix with a foreign id are left out of an update that is otherwise applied; a splitter destination id the -renderer does not have is never read (a guard entry before the destination table would make it heard). Each effect and -sink whose values the renderer cannot use is passed through or skipped in every render pass, with nothing written to -the game's rings or sink buffer, and runs again once its values are usable. Each kind is logged once and counted every -time; valid effects of every type over 20 passes leave nothing out. With `NX2NAVI_AUDIO_STRICT=1` such a value ends -the process, in two runs of their own. No game files and no audio device. +A change to what the audio renderer does with values the game sends (`runtime/audren.c` `update_check`, `request_update` +and `audio_bad`; `runtime/audio_mixer.c` `splitters_walk`, `audio_mixer_routing_check`, `audio_mixer_mix_layout_check`, +`audio_mixer_update_effects` and the render-pass effects; `runtime/audio_effects.c` `audio_effect_invalid`; +`runtime/audio_reverb.c` `audio_reverb_invalid`) passes `python tests\audio_params_check.py`. It builds `audren.c` and +`audio_mixer.c` with the rest of the runtime stubbed and the real effect DSPs linked, and sends hand-built +RequestUpdates through `renderer_handler`. An update whose sections do not match the renderer (sizes, magic words, +overruns, the output buffer, a mix layout with a negative count or more buffers than the renderer has, a splitter id run +whose length wraps in 32 bits) answers 0x5299, a result the SDK hands back to the game (it aborts on any result but +0x5299, 0x5499 and 0x5699), and writes no output, changes no renderer state and places no C buffers. A channel, voice or +splitter record with an id the renderer does not have, a wave buffer index of 4, a filter that is not a finite number +and a changed mix with a foreign id are left out of an update that is otherwise applied; a splitter destination id the +renderer does not have is never read (a guard entry before the destination table would make it heard); a splitter +section whose framing breaks between the check and the apply keeps what came before the fault and is logged as the rest +of the section left out, not as a refused update. Each effect and sink whose values the renderer cannot use is passed +through or skipped in every render pass, with nothing written to the game's rings or sink buffer, and runs again once +its values are usable. Each kind is logged once and counted every time; valid effects of every type over 20 passes leave +nothing out. With `NX2NAVI_AUDIO_STRICT=1` such a value ends the process, in two runs of their own. No game files and no +audio device. A change to the sprint button (`runtime/sprint.c`, the set `sprint` of `recomp/patches-v111.txt`, its call in `runtime/ov_input.c` `npad_states`) passes `python tests\sprint_check.py`, which builds `sprint.c` with the rest of the diff --git a/runtime/audio_mixer.c b/runtime/audio_mixer.c index cef0ce97..124d4e80 100644 --- a/runtime/audio_mixer.c +++ b/runtime/audio_mixer.c @@ -719,37 +719,44 @@ void audio_mixer_close(AudioRenderer *r) { // lock held) it stores the records: a record that names a splitter or destination the renderer does not have is left // out, a destination id in a splitter's list that the renderer does not have is stored as -1, which both splitter loops // of audio_mixer_render skip, and a destination filter with a coefficient that is not a finite number is off (audio_bad -// each). A framing fault while applying (the game's input changed between the two walks) stops the walk there. -// Offsets are 64-bit, so a huge id count cannot wrap a bounds test. +// each). Offsets are 64-bit, so a huge id count cannot wrap a bounds test. +// A framing fault (SPL_FAULT): with `apply` 0 the update is refused (AUDIO_REFUSE). With `apply` 1 it can only come +// from the game rewriting its input during its own RequestUpdate, between the two walks; the update is taken by then, +// so the walk stops there, the records before it stay applied, the mix section is applied after it, and the log says +// the rest of the splitter section is left out (audio_mixer_update_routing ignores the result). +#define SPL_FAULT(kind, fmt, ...) \ + (apply ? (audio_bad(r, (kind), fmt "; the rest of the splitter section is left out", ##__VA_ARGS__), \ + AUDREN_R_INVALID_UPDATE) \ + : AUDIO_REFUSE(r, (kind), fmt, ##__VA_ARGS__)) static uint32_t splitters_walk(AudioRenderer *r, uint32_t revision, const uint8_t *p, uint32_t n, int apply) { Mixer *m = r->mixer; const uint32_t nsplit = (uint32_t)r->param.splitter_count, ndest = (uint32_t)r->param.splitter_destination_count; if (!n) return 0; if (n < 0x20) - return AUDIO_REFUSE(r, AB_SPL_HEADER, "the splitter section is %u bytes, under its 32-byte header", n); + return SPL_FAULT(AB_SPL_HEADER, "the splitter section is %u bytes, under its 32-byte header", n); uint32_t hdr[3]; memcpy(hdr, p, 12); - if (hdr[0] != SNDH) return AUDIO_REFUSE(r, AB_SPL_MAGIC, "the splitter section starts with %08x, not SNDH", hdr[0]); + if (hdr[0] != SNDH) return SPL_FAULT(AB_SPL_MAGIC, "the splitter section starts with %08x, not SNDH", hdr[0]); const uint32_t ni = hdr[1], nd = hdr[2]; const unsigned rev = (revision >> 24) - '0', stride = rev >= 15 ? 0xa8 : rev >= 12 ? 0x90 : 0x70; uint64_t at = 0x20; for (uint32_t i = 0; i < ni; i++) { uint32_t h[4]; if (at + 16 > n) - return AUDIO_REFUSE(r, AB_SPL_INFO_OVERRUN, - "splitter info %u of %u at %#llx overruns the %u-byte splitter section", i, ni, - (unsigned long long)at, n); + return SPL_FAULT(AB_SPL_INFO_OVERRUN, + "splitter info %u of %u at %#llx overruns the %u-byte splitter section", i, ni, + (unsigned long long)at, n); memcpy(h, p + at, 16); at += 16; if (h[0] != SNDI || at + 4ull * h[3] > n) - return AUDIO_REFUSE(r, AB_SPL_INFO, - "splitter info %u at %#llx: %08x %08x %08x %08x (section %u bytes, infos %u, " - "destinations %u)", - i, (unsigned long long)(at - 16), h[0], h[1], h[2], h[3], n, ni, nd); + return SPL_FAULT(AB_SPL_INFO, + "splitter info %u at %#llx: %08x %08x %08x %08x (section %u bytes, infos %u, " + "destinations %u)", + i, (unsigned long long)(at - 16), h[0], h[1], h[2], h[3], n, ni, nd); if (at + 4ull * h[3] + 12 > n) - return AUDIO_REFUSE(r, AB_SPL_PADDING, - "the reserved bytes after splitter info %u's %u ids overrun the %u-byte splitter " - "section", i, h[3], n); + return SPL_FAULT(AB_SPL_PADDING, + "the reserved bytes after splitter info %u's %u ids overrun the %u-byte splitter " + "section", i, h[3], n); if (apply && (h[1] >= nsplit || h[3] > ndest)) { audio_bad(r, AB_SPL_ID, "splitter info %u names splitter %u of %u with %u destinations of %u: that record is left out", @@ -774,9 +781,9 @@ static uint32_t splitters_walk(AudioRenderer *r, uint32_t revision, const uint8_ for (uint32_t i = 0; i < nd; i++) { uint8_t e[0xa8] = {0}; if (at > n || stride > n - at) - return AUDIO_REFUSE(r, AB_DEST_OVERRUN, - "splitter destination %u of %u at %#llx overruns the %u-byte splitter section " - "(records of %#x bytes)", i, nd, (unsigned long long)at, n, stride); + return SPL_FAULT(AB_DEST_OVERRUN, + "splitter destination %u of %u at %#llx overruns the %u-byte splitter section " + "(records of %#x bytes)", i, nd, (unsigned long long)at, n, stride); memcpy(e, p + at, stride); at += stride; uint32_t magic, id; @@ -785,8 +792,8 @@ static uint32_t splitters_walk(AudioRenderer *r, uint32_t revision, const uint8_ memcpy(&id, e + 4, 4); memcpy(&dst, e + 0x68, 4); if (magic != SNDD) - return AUDIO_REFUSE(r, AB_DEST_MAGIC, "splitter destination %u at %#llx starts with %08x, not SNDD", i, - (unsigned long long)(at - stride), magic); + return SPL_FAULT(AB_DEST_MAGIC, "splitter destination %u at %#llx starts with %08x, not SNDD", i, + (unsigned long long)(at - stride), magic); if (!apply) continue; if (id >= ndest) { audio_bad(r, AB_DEST_ID, "splitter destination record %u names destination %u of %u: that record is left " @@ -821,11 +828,12 @@ static uint32_t splitters_walk(AudioRenderer *r, uint32_t revision, const uint8_ } if (at <= n && n - at < 16) at = n; // the padding to 16 bytes, which the game does not clear (measured) if (at != n) - return AUDIO_REFUSE(r, AB_SPL_SIZE, - "the splitter section is %u bytes and its records end at %#llx (infos %u, destinations %u)", - n, (unsigned long long)at, ni, nd); + return SPL_FAULT(AB_SPL_SIZE, + "the splitter section is %u bytes and its records end at %#llx (infos %u, destinations %u)", + n, (unsigned long long)at, ni, nd); return 0; } +#undef SPL_FAULT uint32_t audio_mixer_routing_check(AudioRenderer *r, uint32_t revision, const uint8_t *splitters, uint32_t splitter_bytes, const uint8_t *mixes, uint32_t mix_bytes, int mix_count) { uint32_t refused = splitters_walk(r, revision, splitters, splitter_bytes, 0); @@ -848,7 +856,9 @@ uint32_t audio_mixer_routing_check(AudioRenderer *r, uint32_t revision, const ui // negative count of a mix in use would start the mixes after it before buffer 0, and more buffers than the renderer // has would run past them, so such an update is refused before anything of it is applied; the render's layout check is // then an invariant. The update's records are applied as audio_mixer_update_routing applies them (a changed-mix record -// with an id the renderer does not have is left out). +// with an id the renderer does not have is left out). This check and audio_mixer_update_routing each read the mix +// section from guest memory, so the check holds for the apply only because the game does not rewrite its input during +// its own RequestUpdate (its sending thread waits in the IPC call). uint32_t audio_mixer_mix_layout_check(AudioRenderer *r, const uint8_t *mixes, uint32_t mix_bytes, int mix_count) { int32_t buffers[256]; uint8_t used[256]; diff --git a/tests/audio_params_check.c b/tests/audio_params_check.c index 0a0a6fa9..4041e8c5 100644 --- a/tests/audio_params_check.c +++ b/tests/audio_params_check.c @@ -667,6 +667,34 @@ static void left_out_records(void) { CHECK(apply(r, &u) == 0 && r->mixes[1].volume == 0.5f, "the valid changed mix next to mix 5 is applied"); CHECK(left_out(r, lines, AB_MIX_ID), "mix 5 is logged once (%s)", last_once); close_renderer(r); + + /* A splitter section whose framing breaks between audio_mixer_routing_check's walk and the apply walk (the game + rewriting its input during its own RequestUpdate): audio_mixer_update_routing is called directly, as + audren.c request_update calls it once the update was taken. The splitter info before the fault and the mix + section after it are applied, the destination record at the fault is not, and the log says the rest of the + splitter section is left out, not that the update was refused. */ + r = open_renderer(); + update_default(&u, 11); + u.split_bytes = splitter_section(u.split, 11, 0, 2, both_ids, 2, both_dests); + apply(r, &u); + lines = once_lines; + dest0 = ((Mixer *)r->mixer)->dest[0]; + const int32_t swapped[2] = {1, 0}; + Dest dq[2] = {both_dests[0], both_dests[1]}; + dq[0].volume = 0.25f; + u.split_bytes = splitter_section(u.split, 11, 0, 2, swapped, 2, dq); + u.split[0x44] = 'X'; /* the first destination record's magic, after the splitter info and its 2 ids */ + u.mix[1].volume = 0.5f; + build(&u); + audio_mixer_update_routing(r, in + L.splitters, u.split_bytes, in + L.mixes, 2 * sizeof(AudrenMixIn), 2); + CHECK(split_is(r, 1, 0) && r->mixes[1].volume == 0.5f, + "a fault in the apply walk: the splitter info before it and the mix section are applied"); + CHECK(!memcmp(&dest0, &((Mixer *)r->mixer)->dest[0], sizeof dest0), + "a fault in the apply walk: the destination record at the fault is left out"); + CHECK(left_out(r, lines, AB_DEST_MAGIC) && strstr(last_once, "the rest of the splitter section is left out") && + !strstr(last_once, "refused"), + "a fault in the apply walk is logged once as the rest of the section left out (%s)", last_once); + close_renderer(r); } /* 4. Effects and sinks the renderer cannot use: passed through or skipped in each render pass. */ @@ -923,6 +951,6 @@ int main(int argc, char **argv) { effects_left_out(); in_range_quiet(); printf("audio_params_check: %s (%d refused updates, %d left-out records, %d effects and sinks)\n", - fails ? "FAIL" : "ok", R_ROWS, 10, EFFECT_ROWS); + fails ? "FAIL" : "ok", R_ROWS, 11, EFFECT_ROWS); return fails ? 1 : 0; }