fix(linux): survive the decomp's latent stack-buffer overruns (D189) (#24)

* 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

@@ -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.

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<<mask`, and GE sets `mask = texDimensionToMask(dim) = ceil(log2(dim))` (`src/game/tex.c:361`), so a non-PoT tile (Depot's 65×65 / 96×48 / 56×56 room surfaces) repeats at the **next power of two**, not at its image size the way GL `GL_REPEAT` does → the pattern is squashed/stretched and the seam lands in the wrong place. fast3d never stored `masks`/`maskt` at all (`gfx_dp_set_tile` dropped them) and wrapped purely at the uploaded image dimension. **Fix (`port/fast3d/gfx_pc.cpp`, behind the existing `Video.WrapFix` knob, default OFF):** store `masks`/`maskt` on the tile; in the hoisted per-texunit pre-wrap block in `gfx_sp_tri1` (D74 block — already lifted out of the vertex loop, indexed by texunit `t` not vertex `i`), when the tile is WRAP (no CLAMP/MIRROR bit) and `1<<mask != tex_width`, fold the UV at the N64 period `1<<mask` and clamp the `[dim, 1<<mask)` overflow band (which is a TMEM smear on console, no real texels) to the last texel so it reads as an edge streak instead of a bogus early image restart. `GE_WRAPFIX=0/1` env override added (env wins over the ini, matching `Debug.FrameDump`). **Per-level captures (`-level_09`/`-30`/`-34`/`-20`, WrapFix OFF vs ON, `GE_PCDUMP` 6-frame windows):** no crashes, 6/6 frames each; on settled/comparable frames Silo is ~pixel-identical (phash 0–11), Facility 180–260 pixel-identical, Depot shows small localized texel changes on ceiling/wall cells (dmean 5–9, no structural break); the large per-run deltas are all the D117 intro-camera-pan nondeterminism, not the fix. Default kept **OFF** — no regression, but a headless structural diff can't confirm the Depot ceiling actually looks *better*; needs a human eyeball with `Video.WrapFix=1`. Default-off is byte-identical to golden (all new behaviour is inside `if (g_wrap_fix)`). D74's dead in-vertex-loop wrap block was already reworked/hoisted at M-30; this only adds the mask-period trigger. Confidence: **medium** (mechanism correct; overflow-band handling is an approximation, not exact TMEM-smear emulation; visual win unconfirmed). porting-notes.md §D. | KNOB ADDED, default OFF (`port/fast3d/gfx_pc.cpp`). Needs user visual check on Depot. |
 | D168 | **`GE_PCDUMP` / F12 PPM captures were vertically flipped — the entire source of the bogus D114/D116 "HUD/text X-mirror" (M-33, developer-confirmed).** `gfx_opengl_dump_bound_fbo` (`port/fast3d/gfx_opengl.cpp`) wrote `glReadPixels` output straight to a P6 PPM. GL framebuffer origin is bottom-left; PPM P6 is top-row-first — so every capture (and the `tools_pc/golden/` set, and every screenshot pasted into the finding log since M-6) was upside-down. On real hardware / an actual screen the game renders correctly (developer confirmed). The successive D114→D116 "shared fast3d mirror" investigations — each of which found *every probed stage clean yet the output "mirrored"* — were reading an inverted capture and pattern-matching upside-down asymmetric content (text, guards, the Nintendo logo) as "mirrored". Fix: emit PPM rows bottom-to-top. `tools_pc/golden/*.png` flipped in place to match (see `tools_pc/golden/README.md`); regenerate from a real run when convenient. **Not runtime-verified this session** (no ROM / toolchain in the migrated tree — see the migration note) — needs a fresh capture to confirm text reads normally. Confidence: **high** (mechanism is unambiguous; developer has hardware confirmation the screen is correct). porting-notes.md §D2. | FIXED (`port/fast3d/gfx_opengl.cpp`); D114 / D116 reclassified as capture-orientation artifacts (below). |

docs/porting-notes.md

@@ -631,6 +631,34 @@ doing `va_arg(*args, …)` walks garbage → segfault. Latent at `-Og`, fatal at
   this class for any Linux-only crash whose backtrace runs through
   `xprintf`/`_Putfld`/`vsnprintf`-alikes.
 
+## D8. Latent fixed-size stack-buffer over/under-runs — fatal only under a stack protector
+
+The decomp has a class of local scratch buffers that game code writes a few
+slots past (or before) the declared bounds — e.g. a traversal stack whose
+bail check is looser than its array size, or `&buf[i*3]` with `i` starting at
+-1. The N64 build and the MinGW/Windows build have **no stack protector**, so
+the overrun lands in adjacent stack scratch and is (in practice) harmless —
+the Windows build is stable over thousands of frames. **Ubuntu's gcc default
+is `-fstack-protector-strong`**, which places a canary right after the buffer
+and turns every such overrun into a fatal `*** stack smashing detected ***`
+`__stack_chk_fail`.
+
+- Instances (D189): `stan.c` `sub_GAME_7F0B1DDC` `StandTile *tileStack[39]`
+  (bail is `if (cat >= 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

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])