From 9e473faa6c14457427ad131e5da101a79cc56be3 Mon Sep 17 00:00:00 2001 From: Dan Turner Date: Sun, 4 Oct 2026 11:17:53 +0100 Subject: [PATCH] =?UTF-8?q?macos/arm64:=20review=20fixes=20=E2=80=94=20NUL?= =?UTF-8?q?L-safe=20portAddrIsInWindow,=20drop=20duplicate=20MSAA=20clamp?= =?UTF-8?q?=20and=20wrong=20terminator?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - port_addr.h: portAddrIsInWindow() now rejects the low 64 KiB of the window. With PORT_ADDR_BASE == 0 that range contains NULL; the previous version let NULL through on Windows/Linux, regressing langGet()'s cleared-bank guard (the old numeric bound rejected < 0x10000). Matches the old bounds on every platform. Self-test covers NULL, the low guard, DRAM and above-window. - gfx_opengl.cpp: revert to origin/macos. upstream already clamps MSAA to GL_MAX_SAMPLES (D405 / #106); the branch's second clamp and its comments were a duplicate. MACOS-MSAA-CLAMP finding removed. - chr.c: drop the branch's crouched_rifle_firing_animation_group1 {0} terminator. GCC places groupA below group1, so it never resolved groupA and did not fix the crouched-rifle crash. Replaced by #120 (next commit). - boss.c: guard the rspReplyMsg re-base with #ifdef PORT (the file has no PORT_N64PTR identity fallback for the N64 build). - findings.md: correct the objDeform vtxslot write-up (host pointers, not u32 N64 addresses), give accurate numbers for unguarded src/ edits, and record why the terminator was replaced. --- docs/dev/findings-index.csv | 1 - docs/dev/findings.md | 7 +++--- port/fast3d/gfx_opengl.cpp | 44 +------------------------------------ port/include/port_addr.h | 11 ++++++++-- port/src/port_addr.c | 8 +++++++ src/boss.c | 6 +++++ src/game/chr.c | 10 --------- 7 files changed, 27 insertions(+), 60 deletions(-) diff --git a/docs/dev/findings-index.csv b/docs/dev/findings-index.csv index 608d6aab..5d06e463 100644 --- a/docs/dev/findings-index.csv +++ b/docs/dev/findings-index.csv @@ -297,4 +297,3 @@ MACOS-CAST-CENSUS,FIXED,"macOS/arm64: six more pointer truncations, found by a s MACOS-TEXALIGN,FIXED,"macOS/arm64: texReadUncompressed() aligned its destination through u32 ((u32 *)(((u32)dst + 0xf) & ~0xf) x3), truncating a host pointer and then writing throug…","FIXED defensively (image.c), latent not live." MACOS-RENDER-CRASH,OPEN,"Intermittent renderer crash, level-agnostic, observed once on Dam:","OPEN — observed once, not investigated." MACOS-ADDR-STRICT,FIXED,PORT_ADDR_STRICT: a debug gate that catches the whole MACOS-SFX-BOUND through MACOS-RENDER-CRASH family at the conversion rather than at the fault.,FIXED (new CMake option + port_addr.h/port_addr.c). -MACOS-MSAA-CLAMP,FIXED,MSAA=8 produced a completely BLACK screen —,FIXED (port/fast3d/gfx_opengl.cpp) — diff --git a/docs/dev/findings.md b/docs/dev/findings.md index 657e9ad5..d9768361 100644 --- a/docs/dev/findings.md +++ b/docs/dev/findings.md @@ -705,7 +705,6 @@ covers D24–D69; the log continues in §H (D32 procedure, D70–D121). | MACOS-TEXALIGN | **macOS/arm64: `texReadUncompressed()` aligned its destination through `u32`** (`(u32 *)(((u32)dst + 0xf) & ~0xf)` x3), truncating a host pointer and then writing through it — but the path is **unreachable on PC** (a `GE_D303` probe over a full level boot recorded zero calls; textures come from the pre-converted `pccg` sidecars) | FIXED defensively (`image.c`), latent not live. Group-C census otherwise clean: `init()` is dead on PC, `rsp.c`'s XOR toggle already has a `PLATFORM_64BIT` branch, `objecthandler_2.c`'s `name` is an int-in-a-pointer, `animationtable_data.h`'s 25 warnings are placeholders rewritten by `initResolveAnimTable()` via `ANI_ADDR`. | | MACOS-RENDER-CRASH | **Intermittent renderer crash, level-agnostic, observed once on Dam: SIGSEGV in `gfx_sp_tri1` -> `gfx_texture_cache` with a garbage fault address (`0x16d5bbd636173b9e`).** Did not reproduce in 8 subsequent Dam runs, and is not attributable to the MACOS-CAST-CENSUS changes landed in the same session. Not a known signature (D233 was `gfx_sp_tri1` but a visibility bug, not a crash) | **OPEN — observed once, not investigated.** Garbage (non-address-shaped) fault value suggests a corrupted texture-cache node or a race rather than the MACOS-SFX-BOUND through MACOS-TEXALIGN truncation family. Next: run with `PORT_ADDR_STRICT=ON` to rule the address family out, then instrument the texture cache. | | MACOS-ADDR-STRICT | **`PORT_ADDR_STRICT`: a debug gate that catches the whole MACOS-SFX-BOUND through MACOS-RENDER-CRASH family at the conversion rather than at the fault.** Off by default, compiled out entirely; when on, `portN64ToHost()` validates its argument against the mapped regions and reports offenders with a backtrace | FIXED (new CMake option + `port_addr.h`/`port_addr.c`). Negative self-test (`GE_ADDRSTRICT_SELFTEST=1`) proves the detector fires. Zero reports across 6 levels, 5 under full-control input. | -| MACOS-MSAA-CLAMP | **MSAA=8 produced a completely BLACK screen** — `GL_MAX_SAMPLES` is 4 on this GPU, and nothing ever clamped the requested level, so `glRenderbufferStorageMultisample` left the renderbuffer unallocated -> incomplete FBO. `video.c`'s own comment claims MSAA "snaps down to the highest supported level"; it did not | FIXED (`port/fast3d/gfx_opengl.cpp`) — clamp to `GL_MAX_SAMPLES`, log the snap once. Also fixed two index-vs-GL-name bugs in the same file (`resolve_msaa_color_buffer`, `copy_framebuffer`). Verified MSAA 1/2/4/8 all render. | Phase 2 replaced the Phase-1 demo loop with the real `mainproc()` on real OS @@ -15617,7 +15616,7 @@ if (newvertices != NULL) { ... newvertices[j] = ...; } /* test passes; sto **Lesson.** Both defects were invisible to a `-Wint-to-pointer-cast` census: defect 1 has no cast at all, and defect 2 was written as `(T *)(uintptr_t)x`, which silences the warning. A warning sweep finds the *producer* side of this class; the consumer side needs a type-driven audit of which 32-bit fields hold addresses. **The two `vtxstore_allocate()` callers, and `objDeform()`, are the same class.** Defect 1's failure-signalling shape (`p = PORT_N64PTR(T, f()); if (p != NULL)`) appears at three more sites, all re-based through `PORT_N64PTR` so a returned `0` still becomes `NULL`: - `src/game/chr.c:3609` and `:3626` — the two `chrCreateBloodStain()` callers (this finding's own function). `vtxstore_allocate()` returns `0` when the store is exhausted. -- `src/game/propobj.c` `objDeform()` — `newverts = PORT_N64PTR(Vertex, vtxstore_allocate(...))`, plus every read of the `*vtxslot` field (a `u32` N64 address) through `PORT_N64PTR(Vertex, *vtxslot)`, for the same reason the `CollisionRelatedNode` cast in defect 2 was wrong. +- `src/game/propobj.c` `objDeform()` — `newverts = PORT_N64PTR(Vertex, vtxstore_allocate(...))`, plus every read of `*vtxslot` through `PORT_N64PTR(Vertex, *vtxslot)`. `*vtxslot` is a `Vertex *` slot in the model RW pool that holds host pointers (`newverts`, `rodata->Vertices`), so the re-base is a lossless round trip for any pointer inside the window (`PORT_ADDR_BASE` is 4 GiB-aligned); it does not change behaviour and could be dropped, but it is harmless. These are pointer-width/ABI only (semantics-preserving, identity at `PORT_ADDR_BASE == 0`); the `src/` hunks were carried under this finding's `#ifdef PORT` guard work and are called out here so they are not mistaken for behaviour changes. ## MACOS-CAST-CENSUS — macOS/arm64: six pointer truncations found by census, not by crashing @@ -15632,8 +15631,8 @@ These are pointer-width/ABI only (semantics-preserving, identity at `PORT_ADDR_B So: **after landing any fix in this family, grep the repository for the distinctive fragment of the line you changed and confirm every other occurrence is already in the fixed form.** That one step would have found four of these six for free. **The most interesting one is silent.** `bgRoomCalcBB()` (`bg.c:5213`) truncated its loop bound, so on arm64 `vertices < bound` was false on the first iteration, the body never executed, `limits` kept its sentinel init, and lines 5242-5247 wrote `minbounds = pos + 0x7fff`, `maxbounds = pos - 0x7fff` — an inverted bounding box for **every room of every level**. No crash. Those bounds feed room centres (`bg.c:5155`), portal corners (`bg.c:1468`) and `chrprop.c:619`. Worth checking against open **D249** (distant geometry dropping out on Streets/Egyptian). **Not verified.** The two MP-only fixes (`mp_respawn_handler`'s stan pointer, and the intro-record walk) are unreachable from the solo sweeps and need a multiplayer respawn. -**Guarding status (honest note).** Only the `bondview2.c:8762` (`StandTile *start_stan`, `MACOS-CAST-CENSUS`) and `bg.c:5213` (`bgRoomCalcBB`) hunks are inside an `#ifdef PORT` block. The `intro_record` strides in `bondview_r.c`/`bondview2.c` are **unguarded**: the change is `(s32)` → `(uintptr_t)` in place, which is the identity under N64's 32-bit pointers and therefore semantics-preserving, matching the existing house style for these files (`main` already has unguarded `(uintptr_t)` pointer arithmetic in `bondview_r.c:392`, `bondview2.c:1374-1399`, `image.c:2406,2538`). The owner's branch ground rule is stricter (`#ifdef PORT` on every `src/` edit); converting these to guarded `#ifdef PORT` / `#else` pairs is left as a reviewer-flagged follow-up rather than done unilaterally. -**Terminator note (documented here because it landed in the same `chr.c` hunks).** While in `chr.c` this pass also added a `#ifdef PORT` `{0}` terminator to `crouched_rifle_firing_animation_group1`. Every other `*_firing_animation_groupN[]` is zero-terminated and `initResolveAnimGroupTable()` walks to that `0`; without one here the walk runs into the next global (`crouched_rifle_firing_animation_groupA`) and over-counts `len`. This is the same layout/linker-adjacency class as `main`'s `D420`/PR #120, which fixes the same table independently; the branch's copy is a `#ifdef PORT`-only guard and will be dropped in favour of the `main` fix when it merges. +**Guarding status.** Not every `src/` hunk on this branch is inside `#ifdef PORT`. Against `origin/macos` there are about 190 unguarded lines that call `PORT_N64PTR`/`portN64ToHost` inline. Every file that does so defines `PORT_N64PTR(T, x)` as `((T *)(x))` for the N64 build, so the N64 text compiles to the original cast. There are also about 50 unguarded `(s32)`/`(u32)` → `(uintptr_t)` widenings (`prop.c` x20, `bondview_r.c`/`bondview2.c` `intro_record` x20, `bg.c` x4, `music.c` x3, `image.c`, `cseq.c`), which are the identity where pointers are 32-bit. A few cast-only edits are also unguarded (`image.c:2478`, `options.c:3493`, `bondview2.c:7998`, `model.c:6251-6273`). `main` already uses this unguarded style in these files (`bondview_r.c:392`, `bondview2.c:1374-1399`, `image.c:2406,2538`), but the branch rule asks for `#ifdef PORT` with the N64 text unchanged. The open question for review is whether inline `PORT_N64PTR` with an identity N64 fallback counts as meeting that rule, or whether each site should become an `#ifdef PORT` / `#else` pair. +**Crouched-rifle table.** An earlier version of this branch added a `{0}` terminator to `crouched_rifle_firing_animation_group1`. It was wrong: GCC places `groupA` *below* `group1` (confirmed with `nm`), so the terminator never resolved `groupA` and the crouched-rifle `firing_animation_groups` crash remained. It has been replaced by a cherry-pick of `main`'s fix from PR #120 (`D420`, one array with `groupA` as an alias of `&group1[1]`). ## MACOS-TEXALIGN — macOS/arm64: latent truncation in texReadUncompressed (unreachable on PC) `texReadUncompressed()` (`image.c`) aligned its destination through `u32` — `(u32 *)(((u32)dst + 0xf) & ~0xf)` plus `u16`/`u8` siblings — truncating a host pointer and then writing through it. On arm64 `dst` is `~0x1000_70xx_xxxx`, so the aligned pointers would be unbased `0x70xx_xxxx`. diff --git a/port/fast3d/gfx_opengl.cpp b/port/fast3d/gfx_opengl.cpp index 9c433d56..c3881e12 100644 --- a/port/fast3d/gfx_opengl.cpp +++ b/port/fast3d/gfx_opengl.cpp @@ -1222,44 +1222,11 @@ static int gfx_opengl_create_framebuffer() { return i; } -/* video.c documents MSAA as "snaps down to the highest supported level", but - * nothing ever queried the limit: gfx_msaa_level went to - * glRenderbufferStorageMultisample() verbatim, and a request above - * GL_MAX_SAMPLES leaves the renderbuffer unallocated -> incomplete FBO -> - * a completely BLACK screen (MSAA=8 on Apple Silicon, where GL_MAX_SAMPLES - * is 4). Clamp here, where the GL limit actually matters. */ -static uint32_t gfx_opengl_clamp_msaa(uint32_t level) -{ - static GLint max_samples = 0; - static uint32_t warned_level = 0; - - if (level <= 1) { - return 1; - } - if (max_samples == 0) { - glGetIntegerv(GL_MAX_SAMPLES, &max_samples); - if (max_samples < 1) { - max_samples = 1; - } - } - if ((GLint)level > max_samples) { - if (warned_level != level) { - sysLogPrintf(LOG_WARNING, - "MSAA: %u samples requested but GL_MAX_SAMPLES is %d; using %d", - level, (int)max_samples, (int)max_samples); - warned_level = level; - } - return (uint32_t)max_samples; - } - return level; -} - static void gfx_opengl_update_framebuffer_parameters(int fb_id, uint32_t width, uint32_t height, uint32_t msaa_level, bool opengl_invert_y, bool render_target, bool has_depth_buffer, bool can_extract_depth) { Framebuffer& fb = framebuffers[fb_id]; - msaa_level = gfx_opengl_clamp_msaa(msaa_level); width = max(width, 1U); height = max(height, 1U); @@ -1373,12 +1340,7 @@ void gfx_opengl_resolve_msaa_color_buffer(int fb_id_target, int fb_id_source) { glBindFramebuffer(GL_READ_FRAMEBUFFER, fb_src.fbo); glBlitFramebuffer(0, 0, fb_src.width, fb_src.height, 0, 0, fb_dst.width, fb_dst.height, GL_COLOR_BUFFER_BIT, GL_NEAREST); - /* Restore whatever was bound before the resolve. `current_framebuffer` is - * an INDEX into framebuffers[], NOT a GL framebuffer name -- binding it - * directly named an unrelated GL object (or nothing) and left the wrong - * target bound for the rest of the frame. With MSAA on that produced a - * completely black screen. Use the same fb_id==0 -> GL name 0 rule as - * gfx_opengl_bind_framebuffer above. */ + /* current_framebuffer is an INDEX into framebuffers[], not a GL name; fb 0 is the window's default framebuffer (name 0). */ glBindFramebuffer(GL_FRAMEBUFFER, current_framebuffer == 0 ? 0 : framebuffers[current_framebuffer].fbo); glEnable(GL_SCISSOR_TEST); } @@ -1443,10 +1405,6 @@ void gfx_opengl_copy_framebuffer(int fb_dst, int fb_src, int left, int top, bool glBlitFramebuffer(srcX0, srcY0, srcX1, srcY1, dstX0, dstY0, dstX1, dstY1, GL_COLOR_BUFFER_BIT, GL_NEAREST); - /* Same index-vs-name bug as the MSAA resolve above: for current_framebuffer - * == 0 this bound framebuffers[0].fbo, which is a generated but - * attachment-less FBO (incomplete on a strict GL implementation) -- the - * window's default framebuffer is GL name 0. */ glBindFramebuffer(GL_FRAMEBUFFER, current_framebuffer == 0 ? 0 : framebuffers[current_framebuffer].fbo); glReadBuffer(GL_BACK); diff --git a/port/include/port_addr.h b/port/include/port_addr.h index f9adb079..ac2945ad 100644 --- a/port/include/port_addr.h +++ b/port/include/port_addr.h @@ -134,11 +134,18 @@ static inline uint32_t portHostToN64(const void *p) /* Is a host pointer inside the N64 address window (DRAM / cart / stacks)? * Use this instead of hardcoded numeric bounds: the window is shifted by * PORT_ADDR_BASE on arm64, so an absolute bound that is correct on x86_64 - * silently rejects every real pointer there. */ + * silently rejects every real pointer there. + * + * The low PORT_ADDR_LOW_GUARD bytes of the window are rejected: nothing is + * mapped there, and with PORT_ADDR_BASE == 0 that range contains NULL, which + * is exactly what the callers' plausibility checks exist to catch (e.g. + * langGet() on a cleared bank). */ +#define PORT_ADDR_LOW_GUARD 0x10000u static inline int portAddrIsInWindow(const void *p) { uintptr_t v = (uintptr_t)p; - return v - (uintptr_t)PORT_ADDR_BASE < (uintptr_t)PORT_ADDR_WINDOW; + uintptr_t off = v - (uintptr_t)PORT_ADDR_BASE; + return off >= PORT_ADDR_LOW_GUARD && off < (uintptr_t)PORT_ADDR_WINDOW; } /* Cast-site helper for game code (the D3x ABI/layout class): reads a 32-bit diff --git a/port/src/port_addr.c b/port/src/port_addr.c index 57c1db00..bb201f15 100644 --- a/port/src/port_addr.c +++ b/port/src/port_addr.c @@ -58,6 +58,14 @@ static void portAddrSelfTest(void) portN64ToHost(0x10456789u) == (void *)(B + 0x10456789u)); ok &= portAddrCheck("cfb_16 at base+DRAM_V1", (uintptr_t)cfb_16 == B + 0x70000000u); + ok &= portAddrCheck("in-window: NULL rejected", + !portAddrIsInWindow(NULL)); + ok &= portAddrCheck("in-window: low guard", + !portAddrIsInWindow((void *)(B + PORT_ADDR_LOW_GUARD - 1u))); + ok &= portAddrCheck("in-window: DRAM V1", + portAddrIsInWindow((void *)(B + 0x70001234u))); + ok &= portAddrCheck("in-window: above window", + !portAddrIsInWindow((void *)(B + (uintptr_t)PORT_ADDR_WINDOW))); if (g_portUseImageRel) { ok &= portAddrCheck("host->n64 image", portHostToN64((void *)(g_portImageBase + 0x1234u)) == 0x40001234u); diff --git a/src/boss.c b/src/boss.c index b73b5330..4e5deac9 100644 --- a/src/boss.c +++ b/src/boss.c @@ -833,7 +833,13 @@ void bossMainloop(void) } rspReplyMsg = (s32)(&localGfxDoneMsg); +#ifdef PORT + /* MACOS-ADDR-WINDOW: rspReplyMsg is an s32 holding a + * truncated window pointer; re-base it. */ rspGfxTaskStart(firstGdl, gdl, 0, PORT_N64PTR(s32, rspReplyMsg)); +#else + rspGfxTaskStart(firstGdl, gdl, 0, (s32*)rspReplyMsg); +#endif pendingGfx++; memaSingleDefragPass(); diff --git a/src/game/chr.c b/src/game/chr.c index 0fb4a55b..f485351e 100644 --- a/src/game/chr.c +++ b/src/game/chr.c @@ -896,16 +896,6 @@ struct anim_group_info *ptr_doubles_firing_animation_groups[] = { struct weapon_firing_animation_table crouched_rifle_firing_animation_group1[] = { { PTR_ANIM_fire_kneel_right_leg, 27.0, 0, 0, 0, -1.0, 35.0, 75.0, -1.0, -1.0, 31.0, 75.0, 0.87266463, -0.69813174, 0.90757126, -0.69813174, 1.5, 1.5 }, -#if defined(PORT) - /* MACOS-ADDR-WINDOW: every other *_firing_animation_groupN[] is zero-terminated; - * initResolveAnimGroupTable() walks to that 0, so without one here it runs - * into the next global (crouched_rifle_firing_animation_groupA) and - * over-counts `len`. On N64 the 72-byte stride happens to stop it; on PC - * the 80-byte stride runs several globals deep, so `len` allows a - * `next_anim` past the array and the guard-attack path reads a bogus - * animation. N64 build unchanged. */ - {0, 0.0, 0, 0, 0, -1.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0} -#endif }; struct weapon_firing_animation_table crouched_rifle_firing_animation_groupA[] = {