Review fixes - #16
Merged
Merged
Conversation
vm_var_get/set mask every script-var index with SCRIPT_VAR_INDEX_MASK (0x1FF = 511), but g_script_vars was only 0x129 (297) entries, so any index in 0x129..0x1FF wrote/read out of bounds into adjacent globals. A shipped or corrupt script using VAR_SET in that window silently corrupted unrelated engine state. Grow the in-memory register file to 0x200 (= mask + 1) so every reachable index is in bounds. The save format is untouched: WackiSlot still persists only script_vars[0x129] and save.c copies by sizeof s->script_vars, so the extra slots are runtime scratch — which matches the original engine's 512-slot register file. Re-enables the previously-skipped var_set_index_0x1FE test (it now asserts the high write lands in a real slot) and unifies the 14 test files that locally re-declared the array size.
The FLIC frame decoder trusted the AVI-supplied frame dimensions and
chunk sizes with no bounds:
* w*h came straight from the frame header into memset/memcpy on the
fixed 640x480 g_back_shadow — a frame declaring e.g. 5000x5000
wrote ~25 MB past a 300 KB heap buffer (confirmed live under ASan:
"heap-buffer-overflow WRITE of size 25000000").
* BRUN/DELTA/COLOR/COPY chunk bodies were walked with no read limit,
so a truncated body strode past the malloc'd frame data.
* DELTA's line cursor y (driven by LINE_SKIP from the file) could
address rows past the frame, and bsz underflowed when sz < 6.
Fixes, decode semantics for valid input unchanged:
* reject frames whose w/h exceed FLIC_MAX_W/H (640x480) before any write;
* thread the (clamped-to-buffer) body end into every chunk decoder and
bound each read against it;
* gate DELTA writes on 0 <= y < h;
* clamp CK_COPY/CK_BLACK length to the buffer and the available body;
* bail on sz < 6 and on a chunk size that overruns the frame.
This matters because the site steers users to third-party abandonware
copies of the game, where a corrupt Dane_*.dta cutscene is realistic.
Verified: real cutscenes still decode clean under ASan; 5 malformed
frames that crash the old decoder now survive.
FLIC had zero test coverage — the module wasn't even compiled into the
test binary. Wire src/flic/decoder.c into the test link and add 8 cases:
* positive: CK_COPY writes the frame, CK_BLACK clears it (proves the
new bounds didn't regress valid decoding);
* negative: oversized w/h, zero/negative dims, short CK_COPY body,
lying chunk size, garbage BRUN, and a DELTA line-skip past the
frame — all must stay in bounds.
g_back_shadow is pointed at an exact-size 640*480 buffer (saved and
restored around each test) so under -fsanitize=address any overshoot
aborts. The decoder was independently confirmed ASan-clean on these
inputs while the pre-fix decoder overflowed 25 MB on case "oversized".
The title menu's Load flow loaded the wrong save. The picker's commit handler (LoadSlotClick) already fully restores the chosen slot — LoadSaveSlot + LoadKomnataScene — before returning LOAD_COMPLETED, which maps to MAIN_MENU_RC_LOAD_SAVE. dispatch_main_menu_rc then ran a SECOND LoadSaveSlot(g_selected_save_slot); but g_selected_save_slot had no writer anywhere and was always 0, so picking any slot 1..9 from the title menu silently replaced the just-restored game with slot 0's. Drop the redundant load (the stage loop launches on the already-restored state) and delete the dead g_selected_save_slot global.
LoadSaveSlot poured a slot's script_vars/entity_state into the live
globals and returned success even when LoadStage() failed on an
out-of-range etap_id — leaving g_stage pointing at the previous or
fallback stage while the vars said otherwise, i.e. a wrong-world hybrid
reported as a good load. Now it checks LoadStage's result and refuses
the slot (live state untouched) before the memcpy.
Also NUL-terminate every slot name read from disk in
LoadSaveStateOrInitialize: a corrupt file with 30 non-zero name bytes
made the picker's strcmp / snprintf("%s", name) read past name[30] into
script_vars. Both are disk-sourced-data hardening in the funnels all
loads pass through.
Tests: faithful LoadStage stub (1..STAGE_COUNT) so the reject path is
exercisable; new load_slot_invalid_etap_returns_zero asserts refusal +
untouched live state.
handle_keydown set the process-wide s_quit latch on ESC. Because that
flag is polled by PlatformShouldQuit() in every gameplay and menu loop,
one ESC press cascaded through all of them and hard-exited the whole
app, unsaved — and defeated the engine's own ESC design:
* gameplay ESC already routes through handle_gameplay_keys →
GAME_OVER_USER_QUIT (a clean quit-to-menu), not s_quit;
* menu ESC already routes through poll_menu_keyboard_quit →
MENU_ESC_RC (back out one level);
* at the title menu MENU_ESC_RC == MAIN_MENU_RC_QUIT_CONFIRM_A, i.e.
it should raise the quit-confirm ("bomb") dialog.
All three read ESC from g_key_state (SDLK_ESCAPE = 0x1B) via WaitForKey,
independent of s_quit, so the latch added nothing but the cascade. Drop
it. ESC now backs out of submenus, raises the quit-confirm at the title,
and quits gameplay to the menu; genuine hard quit stays on SDL_QUIT /
WINDOWCLOSE / Cmd-Q. port_attribution and other any-key screens still
dismiss on ESC via g_key_state.
The PKv2 back-reference copy guarded only the write side (mlen vs out-dst). The read pointer back = out + moff was computed from moff, which is derived from the file's offset width tables, with no upper bound — a corrupt offset makes back point above the output buffer and *--back reads past dst+unp. Add the read-side sibling of the existing mlen/llen clamps: bail when moff would push the source past the buffer top. Dead for valid data (round-trip tests unchanged); a corrupt asset now bails instead of over-reading.
DepackRleFrame walked the compressed stream with `*p++` until it had
emitted dst_len output bytes, with no limit on the source — a truncated
or corrupt kind=3 ("rich") ANIM frame (small encoded runs, short
stream) read past the end of the atlas buffer.
Add a src_len parameter and bound every stream read against it (NOTE:
diverges from the original FUN_00410cb0, which trusted the data). New
helper AnimFrameRleSrcLen computes the bytes from a frame's pixel
pointer to the end of AnimAsset.raw_buffer; both engine callers
(paint_primitives, actor/render) pass it. For valid frames the dst_len
loop still terminates first, so decoding is unchanged.
Tests: all existing RLE cases updated to pass src_len; two new cases
(truncated stream, truncated marker_B) pin the bound.
resolve_pe_table fell back to (const int16_t *)(uintptr_t)addr for a VA outside the loaded PE image. On a 64-bit host that turns a 32-bit VA into an unmapped low pointer, which segfaults the instant PVM_X/Y_ OSCILLATE dereferences osc_table_x[0] — and defeats the `osc_table_x ?` NULL guards that already wrap every use. Return NULL instead so those guards fire and the oscillation no-ops for that actor. Valid in-PE tables are unaffected (per_entity_vm_real still green).
ScriptCallDestroyEnt freed the entity struct with a bare xfree(e), leaking e->pixels — the owned bitmap that alpha-plane and doubled sprites allocate via init_entity_bitmap. Use FreeEntity, which frees that buffer too. Safe: e->pixels is only ever an owned xmalloc or NULL (verified: init_entity_bitmap is its sole writer), and the preceding UnregisterEntityByPtr + UnlinkEntity remove e from every table before the free, so nothing dangles. Scope: this plugs the per-destroy bitmap leak only. The larger per-komnata leak in EntityListClearAll (dropped non-actor entities + registered kind=1 atlases) is deliberately left alone — freeing those safely needs cross-table dedup and asset-cache ownership analysis (ScriptCallLoadAsset's "don't free the AnimAsset yet (TBD)") that risks a double-free, so it wants a dedicated, separately-tested change.
poll_virtual_cursor advanced s_vcur_hold_ticks both in the d-pad-held branch and again unconditionally at function end, so a held d-pad ramped twice per pump — the cursor reached VCUR_MAX in ~half of VCUR_ACCEL_TICKS (twitchier than tuned) and the counter also crept up during analog-only motion. Drop the unconditional increment; the branch already advances it and resets to 0 on release.
plat_save_write did fflush + fclose + rename. tmp+rename protects against a crash but not a power cut: on ext4/f2fs (handhelds) the rename can be committed while the file's data is still in the page cache, so a hard power-off leaves a zero/garbage Wacki.sav that the loader resets to defaults — all slots lost. Add an fsync (POSIX) / _commit (Windows) of the tmp file before the rename.
opcode_exactly_0x57_is_valid emitted 3 emit_imm32 (4 halfwords each) + 1 emit into a uint16_t prog[8], writing to index 13 — a stack-buffer- overflow that only surfaced under ASan (the VM itself reads just a valid prefix). This was the one real bug blocking a sanitizer run of the whole suite. Size it to [16].
Same sources as `make test`, built with ASan + UBSan so the file-format parsers (depack / flic / RLE / archive) and raw-byte entity paths run with memory checking on — the leg the review flagged as missing. With the prog[] overflow fixed the whole suite is clean under it (494/494). Alignment checking is off (the suite intentionally mirrors the unaligned Entity layout that EOFF handles safely in production; same rationale as the engine's -fno-strict-aliasing); leak detection off to match smoke-runner. Needs neither SDL2 nor WACKI.EXE, so it's CI-ready.
The unit tests link the SDL stub and the embedded-PE stub, so they need neither SDL2 nor the WACKI.EXE secret — yet the only place they ran was inside each artifact job, which front-loads the WACKI_EXE_URL fetch and hard-fails without the secret. Result: fork PRs got zero CI signal. Add a standalone `test` job on ubuntu that runs `make test` and the new `make test-asan` (ASan + UBSan). It has no secret dependency, so it runs on fork PRs, and it gives the parsers memory-checked coverage the build matrix has no room for. Addresses the "fork PRs are always red" and "no sanitizer leg" gaps from the review.
Targeted de-stale of the parts this branch changed: 50 suites (was 41), the FLIC frame decoder is now tested (flic_decoder suite), and the "CI: not yet wired" section is replaced with what actually runs — the new secret-free test + `make test-asan` job, the per-OS `make test` in the build matrix, and the boot smoke. Also documents `make test-asan`. (The coverage table still carries pre-refactor paths like src/script.c / src/actor.c — a broader doc pass, left for a dedicated cleanup.)
The assets-explorer viewer reuses the engine's graphics.c, so the DepackRleFrame src_len change broke its own caller in assets-explorer/src/render.c (too-few-arguments error → the assets-explorer CI workflow failed while the engine + test jobs passed). Pass AnimFrameRleSrcLen(a, src) — the atlas is in scope here just like the engine call sites.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.