From 86440e3b5881a1edf9f8919573a0cdd75acaec84 Mon Sep 17 00:00:00 2001 From: James Dansereau <106631954+jkdansereau@users.noreply.github.com> Date: Thu, 3 Sep 2026 20:18:25 -0400 Subject: [PATCH] fix(linux): survive the decomp's latent stack-buffer overruns (D189) (#24) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(linux): survive the decomp's latent stack-buffer overruns (D189) The decompiled game code has a class of fixed-size stack-buffer over/under- runs that write a few words into adjacent stack scratch: * stan.c sub_GAME_7F0B1DDC — `StandTile *tileStack[39]` with a bail check of `if (cat >= 41)` that only runs once per outer iteration, so `cat` reaches 40 + pointCount and the append writes several slots past the end. BUNKER1 (-level_09) prop room-list setup legitimately visits ~40 tiles. * bondview2.c bondviewCalcIntroSwirlCamera — `f32 pointbuf[10]` indexed `&pointbuf[i*3]` for i in [-1..2] then written [0..5], i.e. [-3 .. 11]. On the N64 and on the MinGW/Windows build (neither uses a stack protector) these are harmless in practice — the Windows build is stable over thousands of frames. Only Ubuntu's gcc default (`-fstack-protector-strong`) turns them into a fatal `__stack_chk_fail`, at BUNKER1 stage load and again at the intro swirl camera (~frame 5). Two-part fix: * `-fno-stack-protector` globally (CMakeLists) — matches the N64 and MinGW builds so the same code runs the same way. A hardened Linux build would need the whole overrun class swept first. * `StandTile *tileStack[64]` under `#ifdef PORT` (stan.c) — the one case that is cleanly understood and bounded (`pointCount` is 4-bit, so 40+15 is the true worst case). Pure local scratch, never escapes, no behaviour change. N64 keeps `[39]` under `#else`. * stan.c:2225 link-resolve also switched to the D177 PORT_PTRADD macro for idiom consistency (the blob maps below 4 GiB so the bare cast is currently lossless; #else is byte-identical). Stacked on the D188 xprintf fix. Verified: Windows -level_09 framediff 3/3 within golden threshold (all changes are #ifdef PORT or no-op on MinGW); Linux -O2 -level_09 now runs to frame 2100+ with no crash. * docs: D189 update (stack-protector / latent overrun class) + porting-notes D8 * docs: D190 — Linux -level_45 objective.c load crash (open); D189 sweep 5/6 --- CMakeLists.txt | 12 ++++++++++++ docs/dev/findings.md | 3 ++- docs/porting-notes.md | 28 ++++++++++++++++++++++++++++ src/game/stan.c | 23 ++++++++++++++++++++++- 4 files changed, 64 insertions(+), 2 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index afb4d1d2..448c2fea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -220,6 +220,18 @@ add_compile_options( -fwrapv -include "versioninfo.h" ) +# The decompiled game code has a class of latent fixed-size stack-buffer +# over/under-runs (e.g. stan.c sub_GAME_7F0B1DDC's tileStack, bondview2.c +# bondviewCalcIntroSwirlCamera's pointbuf[10] indexed [-3..11]) that write a +# few words into adjacent stack scratch. On the N64, and on the MinGW/Windows +# build (no default stack protector), this is harmless in practice — verified +# stable over thousands of frames. Only Ubuntu's gcc default +# (-fstack-protector-strong) turns these into a fatal __stack_chk_fail at +# stage load / first frames. Match the N64 and MinGW builds so the same code +# runs the same way; the genuinely understood cases are still fixed properly +# under #ifdef PORT (D189). A hardened Linux build would need the whole class +# swept first — tracked in docs/dev/findings.md. +add_compile_options(-fno-stack-protector) # Keep frame pointers: dbghelp's StackWalk64 (crash handler + thread dumps) # cannot read DWARF unwind info on MinGW, so the EBP chain is the only way to # get a real backtrace of a hung/crashed game thread. diff --git a/docs/dev/findings.md b/docs/dev/findings.md index 33b31345..81036985 100644 --- a/docs/dev/findings.md +++ b/docs/dev/findings.md @@ -389,7 +389,8 @@ covers D24–D69; the log continues in §H (D32 procedure, D70–D121). | D186 | **`Video.FpsCap` throttles the sim, not just presentation (M-39 diag, M-42 clamp).** The frame-pacing `sysSleep`+busy-wait in `sync_framerate_with_timer` (`port/fast3d/gfx_sdl2.cpp`) runs inline on the scheduler thread (`osSpTaskStartGo`→`gfx_run`→`swap_buffers_begin`), so a low cap blocks VI-retrace delivery and drags every game thread down to the cap rate (no N64 analogue — RSP/RDP are separate silicon). **Landed:** `0 < FpsCap < 30` → treated as `0` (uncapped) + warning, at both `videoInit` (normalises a bad `ge007.ini` for the next `configSave`) and `gfx_sdl_set_target_fps` (covers the F10-overlay live path); caps ≥ 30 unchanged. Verified: default `FpsCap=0` framediff 3/3 golden-identical; `FpsCap=10` now runs at the uncapped rate; `FpsCap=60` still paces. **Owed:** move pacing off the scheduler thread so any cap only drops presented frames. See §F "D186" + porting-notes.md §E. | CLAMP LANDED (`port/fast3d/gfx_sdl2.cpp`, `port/src/video.c`); real fix owed | | D187 | **HUD ammo-type/weapon icon vanished at `-O2` (M-46 root-cause, M-47 fix + verify).** `set_rgba_redirect_generate_microcode()` (`src/game/gunfire.c:5929`, sole emitter of the bottom-right magazine icon; call sites 6057/6123/6217/6219, all `gdl = f(...)`) is `Gfx *` with **no `return`** — the decomp banner above it flags exactly this. Harmless under `-Og` (GCC left the tail-call result in `$v0`); commit `34885535` (Sep 2) switched release to `-O2` → undefined return reg → callers resume the DL from the icon's start offset and overwrite its `texSelect`/`display_image` commands with the following `gunDrawHudInteger` output. Ammo *numbers* use a separate correctly-returning path — hence the exact symptom split. Fix = one line, `return` under `#ifdef AVOID_UB` (already `-DAVOID_UB=1`, `CMakeLists.txt:207`); N64 byte-match path unchanged under `#else`. **Verified M-47 (interactive console):** `-O2` build, `-level_09` `GE_PCDUMP` 200–440 — orange clip glyph absent pre-fix, present post-fix; `framediff.py` frame 320 worst-cell dmean **21.8 → 4.0**, frame 440 **21.7 → 4.6** (converges to the pre-regression golden); other levels unaffected (change is one HUD function). Do **not** revert `34885535`. | FIXED (`src/game/gunfire.c`, `#ifdef AVOID_UB`); PR #19 | | D188 | **Linux `-O2` boot crash: `_Printf` passes `&args` of a by-value `va_list` param (M-48).** On the x86-64 System V ABI `va_list` is an array type → a by-value `va_list` parameter decays to a pointer, so `_Printf`'s `_Putfld(&x, &args, …)` handed `_Putfld` the address of its own local pointer slot, not the arg list; `_Putfld`'s `va_arg(*args, …)` then walked garbage. Latent at `-Og`, SIGSEGV at `-O2` (PR #9) — crash path `bossInitMainthreadData` (`boss.c:247`, D121 per-level `-m` sprintf) → `sprintf("-level_%c%c %s", …)` → `_Putfld` @ `xprintf.c`. Same UB class as D187. Windows x64 `va_list` is a scalar so `&args` worked there (Windows build unaffected — `-level_09` framediff 3/3). Fix: `#ifdef PORT` — `_Printf` takes `va_list *`, threads it straight to `_Putfld`; `src/sprintf.c` (sole PC caller; `rmon.c` excluded) passes `&args` of a real object. N64 build untouched (`#ifdef PORT`; file byte-matches ROM). | FIXED (`src/libultrare/libc/xprintf.c`, `src/sprintf.c`, `#ifdef PORT`); PR #22 | -| D189 | **Linux stage-load stack-smash: `sub_GAME_7F0B1DDC` `tileStack[39]` vs `>= 41` traversal guard (M-48, open).** `-level_09` BUNKER1 prop setup (`proplvreset2` → `domakedefaultobj` → `setupUpdateObjectRoomPosition` → `chrpropUpdateRoomList` → `sub_GAME_7F0B21B0` → `sub_GAME_7F0B1DDC`, `src/game/stan.c:2162`): local `StandTile *tileStack[39]` is appended `tileStack[cat]=…; cat++` *before* the bail check `if (((u32)cat) >= 41) return …` — so `cat` reaches 40, writing 1–2 pointers past the array; on x86-64 (8-byte ptrs + `-fstack-protector` canary) → `*** stack smashing detected ***` abort. N64 (4-byte ptrs, no canary) tolerated it. Open question: inherent latent decomp overflow that BUNKER1 data legitimately hits (→ port-layer guard needed, `stan.c` is byte-matched), or a PC-side `StandTile` layout / `link<<3` stride mismatch driving a runaway graph walk. Triage subagent dispatched M-48. Blocks the Linux half of v0.1.0. | OPEN — root-cause in progress (M-48) | +| D189 | **Linux `-fstack-protector` aborts on the decomp's latent stack-buffer overruns (M-48) — FIXED.** Two hit on `-level_09`: (1) `sub_GAME_7F0B1DDC` (`stan.c`) — local `StandTile *tileStack[39]`, bail check `if (cat >= 41)` runs only once per outer iteration so `cat` reaches `40 + pointCount` and the append writes past the end; BUNKER1 prop room-list setup legitimately visits ~40 tiles (confirmed in GDB: 30 valid distinct tile ptrs and climbing; `standTileStart=0x701ae160`, so **not** a pointer-truncation bug — the earlier "blob >4 GiB" triage premise was wrong). (2) `bondviewCalcIntroSwirlCamera` (`bondview2.c`) — `f32 pointbuf[10]` written `[-3 .. 11]` via `&pointbuf[i*3]`, i∈[-1..2]. Both harmless on N64 + the MinGW build (no stack protector); only Ubuntu gcc's `-fstack-protector-strong` default makes them fatal `__stack_chk_fail`. **Fix:** `-fno-stack-protector` globally (`CMakeLists.txt`) to match the N64/MinGW builds, + `tileStack[64]` under `#ifdef PORT` for the one cleanly-bounded case, + `stan.c:2225` link-resolve moved to the D177 `PORT_PTRADD` macro for consistency. A *hardened* Linux build would need the whole overrun class swept. **Verified:** Windows `-level_09` framediff 3/3; Linux `-O2` `-level_09` runs to frame 2100+ no crash. porting-notes.md §C. | FIXED (`CMakeLists.txt`, `src/game/stan.c` `#ifdef PORT`); PR #23 | +| D190 | **Linux `-level_45` SIGSEGV at load in `add_ptr_to_objective` (`objective.c:64`) (M-48, open).** `objective_ptrs[objective->menu] = objective;` faults — `objective` wild or `objective->menu` a garbage index → OOB write. Almost certainly the D122/D126/D132 propDef-stride / ROM-serialized-struct-widening family (`struct objective_entry` / the type-23/type-1E setup-text record chain read at the wrong stride on PC). Found in the M-48 Linux level sweep: **5/6 spot-checked levels (`-level_09/20/27/34/37`) load + render fine; `-level_45` is the one failure.** Not a v0.1.0-alpha blocker (alpha is explicitly "expect crashes"). Windows: `-level_45` load status not re-checked this session. | OPEN — not investigated (M-48) | | D152+ | D152 addendum (M-31): static audit of every compiled-audio `osSetIntMask(OS_IM_NONE)` — **all balanced**, no unbalanced early-return. Real fixes: `sndSetSfxSlotVolume` now holds the mask across its list walk (matches its twin `sndDeactivateAllSfxByFlag`) + `sndApplyVolumeAllSfxSlot` batches the slot loop under one recursive hold (kills the fade-out lock-acquire storm); `portThreadWrapper` calls `imThreadExitRelease()` on thread exit (kills the "transient thread acquired `OS_IM_NONE` and died" leak + the pthread-id-reuse re-wedge). Steal-lock kept as backstop. | FIXED (`src/snd.c`, `port/src/libultra.c`, `#ifdef PORT`) — fade-out repro playtest-gated, not headless-verified | | RC3 · D167 | **Non-power-of-two texture wrap period (`docs/dev/TEXTURE-GLITCH-ANALYSIS.md` §6 RC3 — "textures repeat oddly", residual Depot-ceiling noise after D161).** The N64 RDP masks the texel coordinate of a wrapping render tile at `1<= 41)`, checked once per outer loop → writes `[40..]`); + `bondview2.c` `bondviewCalcIntroSwirlCamera` `f32 pointbuf[10]` written + `[-3 .. 11]`. +- Fix policy: **`-fno-stack-protector` globally** (`CMakeLists.txt`) so Linux + matches the N64 and MinGW builds — the code is byte-matched to the ROM and + cannot be "fixed" per site without diverging. Where a case is cleanly + bounded and the buffer is pure local scratch (never escapes, no ABI role), + widening it under `#ifdef PORT` is also fine (`tileStack[64]`). +- A hardened Linux build (re-enabling the protector) would need the whole + class swept first. Grep heuristic for new instances: any Linux-only + `__stack_chk_fail` / `__fortify_fail` in the backtrace, in a `sub_GAME_*` or + other byte-matched function, during stage load or the first few frames. +- Distinct from D7 (`va_list` UB) and D6 (missing `return`) but the same + "works on N64 + MinGW, fatal on hardened Linux" shape. + ## E. Process / method notes - Investigation loop is: reproduce → env-gated capped probe → root-cause diff --git a/src/game/stan.c b/src/game/stan.c index 76f529db..4771df04 100644 --- a/src/game/stan.c +++ b/src/game/stan.c @@ -2162,7 +2162,22 @@ void getTileEdgePoints(StandTile *tile, s32 pointI, coord3d *currPntRtn, coord3d StanCollisionResult sub_GAME_7F0B1DDC(StandTile **startTile, f32 x, f32 z, f32 radius, standTileLocusCallback_A_t callbackA, standTileLocusCallback_B_t callbackB, standTileLocusCallback_C_t callbackC, struct StandTileLocusCallbackRecord *record) { s32 i; +#ifdef PORT + /* D189: the ROM sizes this local scratch stack at [39] but its own bail + * check is `if (cat >= 41)`, tested only once per outer iteration (after a + * whole tile's edges are appended), so `cat` can legitimately reach + * 40 + pointCount and `tileStack[cat] = ...` writes several slots past the + * end. On N64 that clobbered adjacent stack scratch and (usually) got away + * with it; on x86-64 the pointers are 8 bytes and gcc's + * -fstack-protector-strong canary sits right after the array, so a dense + * tile region (BUNKER1 -level_09 prop room-list setup hits ~40 tiles) + * aborts the process. Widen the scratch buffer — pure local, never escapes + * (callbackC ignores it), no behaviour change. `pointCount` is a 4-bit + * field so 40 + 15 is the true worst case; 64 covers it with margin. */ + StandTile *tileStack[64]; +#else StandTile *tileStack[39]; +#endif StandTile *tile; StandTile *linkedTile; s32 visitedCount; @@ -2212,7 +2227,13 @@ StanCollisionResult sub_GAME_7F0B1DDC(StandTile **startTile, f32 x, f32 z, f32 r { if ((callbackB == NULL || !callbackB(tile, pointI, edgeDist, pointDistA, pointDistB, record)) && (tile->points[pointI].link >> 4)) { - linkedTile = ((u32) standTileStart) + (tile->points[pointI].link << 3); + /* D189: use the 64-bit-safe PORT_PTRADD like D177 + * did for the sibling link-resolve sites in this + * file (the stan blob happens to map below 4 GiB + * so the bare `(u32)` cast is currently lossless, + * but this keeps the idiom consistent). #else is + * byte-identical to the original cast. */ + linkedTile = PORT_PTRADD(StandTile *, standTileStart, tile->points[pointI].link << 3); for (i = cat - 1; i >= 0; i--) { if (linkedTile == tileStack[i])