fix: bound wizard-created Which against its item table (closes #10)
createObj stored the raw 0-f nibble as Object.Which with no bounds check, so wizard mode -> C -> / -> f made a wand numbered 15 against a 14-entry table and panicked in fixStick. Input outside 0-f overshoots much further rather than going negative: readchar returns a byte, so the int(ch-'a') + 10 branch is byte arithmetic and wraps, giving 234 for 'A' and 202 for '!', and panicked the same way. C's create_obj() was equally unchecked, but its consumers were either switches (defined for any value) or static-array reads past the end (undefined, and survivable in practice). Since one game is now one process, the Go panic kills the game outright and leaves the terminal in raw mode. Reject at the two boundaries a bad Which can enter through. createObj now refuses an out-of-range choice with a message drawn from C's own type_name() vocabulary and adds nothing to the pack, a deliberate divergence recorded in a comment because C had no defined behavior here to be faithful to. Restore refuses a snapshot describing such an object (ErrSaveCorrupt) rather than loading a game that would explode later. A decoded snapshot is also the only source of a genuinely negative Which, Which being a plain int off the wire, so it is what the Which >= 0 arm of hasValidWhich defends against. Behind those, whichLimit/hasValidWhich back defensive guards at every dispatch the issue names: the quaffHandler/readHandler/zapHandler accessors return no handler instead of indexing (for wands that is exactly what non-MASTER C did, matching no case and still running o_charges--), the callIt lore lookups, identifyType, armorClass for the a_class[] reads, initWeapon against the missing init_dam[] row for WeaponFlame, fixStick's ws_type[] read, and inventoryName and objectWorth, hoisted so one check each covers the whole family of per-kind name and appraisal tables. identifyType's bound is defensive rather than live: readHandlers registers readIdentify only for the identify scrolls, all of which sit inside the shorter idType table. No in-range input changes behavior and no guard consumes a random number: the rejection precedes every rnd() call. TestSeedCompatItemTables stays green untouched. New game/wizard_test.go covers the exact reproducer, a rejection sweep over every indexed kind including the wrapped values from input outside 0-f, an acceptance sweep proving valid choices still build the right item, one no-panic test per guarded family, the fixStick crash site, the corrupt-save rejection over both the wrapped values and a negative Which, and a check that whichLimit still agrees with the table sizes. Each guard was confirmed load-bearing by reverting it and watching the test fail. TODO.md records the step; Next Step is deliberately left alone, since this arrived out of band via an issue.
This commit is contained in:
43
TODO.md
43
TODO.md
@@ -34,6 +34,49 @@ wizard commands).
|
||||
|
||||
# Completed Steps
|
||||
|
||||
- 2026-08-09 Wizard-create bounds fix (`fix/wizard-which-bounds`, closes #10):
|
||||
`createObj` stored the raw `0-f` nibble as `Object.Which` with no bounds
|
||||
check, so wizard mode -> `C` -> `/` -> `f` produced a wand numbered 15 against
|
||||
a 14-entry table and panicked in `fixStick`. Input outside `0-f` overshoots
|
||||
much further rather than going negative: `readchar` returns a `byte`, so the
|
||||
`int(ch-'a') + 10` branch is byte arithmetic and wraps — `'A'` gives 234 and
|
||||
`'!'` gives 202 — and panicked the same way. C's `create_obj()` was equally
|
||||
unchecked, but every C consumer was either a `switch` (defined for any value)
|
||||
or a static-array read past the end (undefined, and survivable in practice),
|
||||
whereas since refactor step 8 one game is one process, so the Go panic kills
|
||||
the game with the terminal still in raw mode. Fixed at the two boundaries a
|
||||
bad `Which` can enter through: `createObj` now rejects an out-of-range choice
|
||||
with a message built from C's own `type_name()` vocabulary and adds nothing to
|
||||
the pack (a deliberate, commented divergence, since C had no defined behavior
|
||||
here to be faithful to), and `Restore` refuses a snapshot describing such an
|
||||
object (`ErrSaveCorrupt`) instead of loading a game that would explode later.
|
||||
Behind those, `whichLimit`/`hasValidWhich` back defensive guards at every
|
||||
dispatch named in the issue: the three effect tables (the new `quaffHandler`,
|
||||
`readHandler`, and `zapHandler` accessors return no handler rather than
|
||||
indexing — for wands that is exactly non-`MASTER` C, which matched no case and
|
||||
still ran `o_charges--`), the `callIt` lore lookups, `identifyType` (whose
|
||||
table is shorter than the scroll table keying it, though no scroll that can
|
||||
reach `readIdentify` overshoots it, so that one is defensive rather than a
|
||||
live bound), `armorClass` for the four `a_class[]` reads, `initWeapon` against
|
||||
the missing `init_dam[]` row for `WeaponFlame`, `fixStick`'s `ws_type[]` read,
|
||||
and `inventoryName`, hoisted so one check covers the scroll-title read the
|
||||
issue listed plus its potion-color, ring-stone, wand-material, weapon and
|
||||
armor siblings. `objectWorth` got the same hoisted guard, since the
|
||||
death-screen appraisal reads the identical per-kind tables. No in-range input
|
||||
changes behavior and no guard consumes a random number — the rejection
|
||||
precedes every `rnd()` call, verified both by an explicit seed-unchanged test
|
||||
and by `TestSeedCompatItemTables` staying green untouched. New
|
||||
`game/wizard_test.go`: the exact reproducer, a rejection sweep over every
|
||||
indexed kind including both wrapping-input forms, an acceptance sweep proving
|
||||
valid choices still build the right item, one no-panic test per guarded family
|
||||
(wand/potion/scroll/armor/weapon), the `fixStick` crash site, the corrupt-save
|
||||
rejection over the wrapped values and a negative `Which` (a decoded snapshot
|
||||
is the only source of one, so it is what exercises the `Which >= 0` arm of
|
||||
`hasValidWhich`), and a check that `whichLimit` still agrees with the table
|
||||
sizes. Each guard was confirmed load-bearing by reverting it and watching the
|
||||
test panic. `Next Step` deliberately not rotated: this was out-of-band issue
|
||||
work.
|
||||
|
||||
- 2026-08-09 Stale-docs correction (`docs-staleness`, closes #3): four claims in
|
||||
`MEMORY.md`/`TODO.md`/`README.md` had gone false and were misdirecting agents
|
||||
— the reviewer on PR #9 repeated one of them verbatim. Each was re-verified
|
||||
|
||||
Reference in New Issue
Block a user