Crash: wizard createObj accepts out-of-range Which, panicking on seven unguarded array dispatches #10

New Issue

2026-08-09T03:43:34+02:00

clawbot commented

2026-08-09 03:43:34 +02:00

Problem

createObj reads a raw nibble from the keyboard and stores it as Which
without any bounds check (game/wizard.go:18-23):

ch := g.readchar()
if isDigit(ch) {
	obj.Which = int(ch - '0')
} else {
	obj.Which = int(ch-'a') + 10
}

So Which can be 0–15 (and negative for input below 'a'). Every
downstream dispatch is a fixed-size array index with no guard:

Site	Index into	Size
`game/sticks.go:34`	`zapHandlers[obj.WandKind()]`	`NumWandTypes` (14)
`game/potions.go:45`	`quaffHandlers[obj.PotionKind()]`	`NumPotionTypes` (14)
`game/scrolls.go:32`	`readHandlers[obj.ScrollKind()]`	`NumScrollTypes`
`game/potions.go:51`, `game/scrolls.go:39,170,171`, `game/things.go:53`	`Items.Potions[obj.Which]`, `Items.Scrolls[obj.Which]`	per-kind
`game/wizard.go:74`	`aClass[obj.Which]`	`NumArmorTypes` (8)
`game/weapons.go:162`	`initWeaps[which]` (reached from `wizard.go:61`)	`NumWeaponTypes` (9)
`game/sticks.go:505`	`Items.WandType[cur.Which]`	per-kind

Reproduce: wizard mode → C → / (wand) → f → index 15 into a
14-element array → panic.

Why it matters more in Go than in C

C indexed past the end of a static array: undefined behavior, but in practice
it read adjacent garbage and kept going, or fell into
otherwise: msg("what a bizarre schtick!"). Go panics — and since refactor
step 8 made one game run one process (myExit → os.Exit), an unrecovered
panic takes the whole game down and leaves the terminal in raw mode.

So a C quirk that was survivable has become a hard crash. This is a genuine
divergence from 5.4.4, not merely a hardening opportunity.

Definition of done

createObj validates Which against the valid range for the chosen item
kind and re-prompts or rejects out-of-range input, rather than storing it.
Match C's user-facing behavior where C had defined behavior.
Defensive guards at the dispatch sites listed above, so no reachable path
can index out of bounds even if a malformed object arrives from elsewhere
(notably: a restored save file).
A regression test constructs the exact reproducer above and asserts it does
not panic — plus at least one test per guarded dispatch family
(wand / potion / scroll / armor / weapon).
Negative Which (input below 'a' and below '0') is covered too.
make check fully green.
TODO.md updated in the same commit.
Commit title ends with (closes #N).

Implementation requirements

Read C's wizard.c create_obj() and the sticks.c dispatch
(git show origin/c-master:wizard.c, …:sticks.c) before choosing the
rejection behavior. Do NOT check out or modify origin/c-master.
Where C's behavior was undefined, prefer a clean rejection with a message
over emulating garbage reads, and note the deliberate divergence in a code
comment. Do not invent new user-facing message text where C had some.
Restoring the "what a bizarre schtick!" message is tracked separately —
do not fold it in here beyond what the bounds fix strictly needs.
Guards must not change behavior for valid input; the RNG call order must be
identical for every in-range case.
Tests need t.Parallel() and the //nolint:testpackage header.
make targets only. Do NOT modify .golangci.yml.
Never mention Claude or Anthropic anywhere.

Priority

Highest of the open code issues — it is the only known way to crash the game
from normal (wizard-mode) input.

## Problem `createObj` reads a raw nibble from the keyboard and stores it as `Which` without any bounds check (`game/wizard.go:18-23`): ```go ch := g.readchar() if isDigit(ch) { obj.Which = int(ch - '0') } else { obj.Which = int(ch-'a') + 10 } ``` So `Which` can be **0–15** (and negative for input below `'a'`). Every downstream dispatch is a fixed-size array index with no guard: | Site | Index into | Size | | --- | --- | --- | | `game/sticks.go:34` | `zapHandlers[obj.WandKind()]` | `NumWandTypes` (14) | | `game/potions.go:45` | `quaffHandlers[obj.PotionKind()]` | `NumPotionTypes` (14) | | `game/scrolls.go:32` | `readHandlers[obj.ScrollKind()]` | `NumScrollTypes` | | `game/potions.go:51`, `game/scrolls.go:39,170,171`, `game/things.go:53` | `Items.Potions[obj.Which]`, `Items.Scrolls[obj.Which]` | per-kind | | `game/wizard.go:74` | `aClass[obj.Which]` | `NumArmorTypes` (8) | | `game/weapons.go:162` | `initWeaps[which]` (reached from `wizard.go:61`) | `NumWeaponTypes` (9) | | `game/sticks.go:505` | `Items.WandType[cur.Which]` | per-kind | **Reproduce:** wizard mode → `C` → `/` (wand) → `f` → index 15 into a 14-element array → panic. ## Why it matters more in Go than in C C indexed past the end of a static array: undefined behavior, but in practice it read adjacent garbage and kept going, or fell into `otherwise: msg("what a bizarre schtick!")`. Go panics — and since refactor step 8 made one game run one process (`myExit` → `os.Exit`), an unrecovered panic takes the whole game down and leaves the terminal in raw mode. So a C quirk that was survivable has become a hard crash. This is a genuine divergence from 5.4.4, not merely a hardening opportunity. ## Definition of done 1. `createObj` validates `Which` against the valid range **for the chosen item kind** and re-prompts or rejects out-of-range input, rather than storing it. Match C's user-facing behavior where C had defined behavior. 2. Defensive guards at the dispatch sites listed above, so no reachable path can index out of bounds even if a malformed object arrives from elsewhere (notably: a restored save file). 3. A regression test constructs the exact reproducer above and asserts it does **not** panic — plus at least one test per guarded dispatch family (wand / potion / scroll / armor / weapon). 4. Negative `Which` (input below `'a'` and below `'0'`) is covered too. 5. `make check` fully green. 6. `TODO.md` updated in the same commit. 7. Commit title ends with ` (closes #N)`. ## Implementation requirements - Read C's `wizard.c create_obj()` and the `sticks.c` dispatch (`git show origin/c-master:wizard.c`, `…:sticks.c`) before choosing the rejection behavior. Do NOT check out or modify `origin/c-master`. - Where C's behavior was undefined, prefer a **clean rejection with a message** over emulating garbage reads, and note the deliberate divergence in a code comment. Do not invent new user-facing message text where C had some. - Restoring the `"what a bizarre schtick!"` message is tracked separately — do not fold it in here beyond what the bounds fix strictly needs. - Guards must not change behavior for valid input; the RNG call order must be identical for every in-range case. - Tests need `t.Parallel()` and the `//nolint:testpackage` header. - `make` targets only. Do NOT modify `.golangci.yml`. - Never mention Claude or Anthropic anywhere. ## Priority Highest of the open code issues — it is the only known way to crash the game from normal (wizard-mode) input.

clawbot commented

2026-08-09 07:00:42 +02:00

Implementation plan

Read wizard.c create_obj() and sticks.c do_zap()/fix_stick() on
origin/c-master (via git show, no checkout). Findings that shape the fix:

C's create_obj() stores the nibble unchecked
(obj->o_which = isdigit(ch) ? ch - '0' : ch - 'a' + 10) and never
validates. Every downstream use is either a switch (defined behavior in C,
even for a bogus value) or a static-array read past the end (undefined). So
C has no defined user-facing behavior to be faithful to here — this is
exactly the "prefer a clean rejection" case.
do_zap()'s switch has no array index at all in C; the otherwise arm is
the MASTER-only "what a bizarre schtick!" message, and then
obj->o_charges-- runs regardless. Without MASTER, C silently did nothing
and still burned a charge. That gives the Go guard a zero-invention
semantic: an out-of-range wand yields a nil handler, no effect, and
obj.Charges-- still runs. Restoring the message stays with #13.

1. One shared bounds predicate (`game/object.go`)

whichLimit(kind ObjectKind) int — how many entries the kind's per-kind
tables have: NumPotionTypes, NumScrollTypes, NumRingTypes,
NumWandTypes, NumArmorTypes; NumWeaponTypes + 1 for weapons (the
Items.Weapons array is deliberately sized +1 for the WeaponFlame
dragon-breath entry that fireBolt really does set on a live Object);
0 for food / amulet / gold / KindNone, whose Which is not a table
index — those keep accepting anything, exactly as today.
(*Object).hasValidWhich() bool — limit == 0 || 0 <= Which < limit.

2. Reject at the two boundaries where a bad `Which` can enter

Keyboard (createObj): after reading the nibble, reject out-of-range
with a message and return without adding to the pack. The message reuses
C's existing type_name() vocabulary (ObjectKind.String()) rather than
inventing a new noun. Weapons validate against NumWeaponTypes (not
+1): WeaponFlame has a name-table entry but no init_dam[] row, so it
is not creatable. A code comment records the deliberate divergence from
5.4.4. The check sits before any rnd() call, so RNG order is untouched
for every valid input and for the rejected path alike.
Save file (Restore): validate every Object in the snapshot
(st.Objects, st.Player.Body.Pack, each st.Monsters[i].Pack) and
return a new ErrSaveCorrupt-style error rather than loading a game that
will explode later. A save file is machine-written, so an out-of-range
Which can only mean corruption or tampering.

3. Defensive guards at the listed dispatch sites

None of these change behavior for any in-range value; each is an
if !obj.hasValidWhich() short-circuit around the existing code:

Site	Guarded behavior
`sticks.go:34` `zapHandlers[…]`	treated as a `nil` handler: no effect, `Charges--` still runs (C's non-`MASTER` `otherwise`)
`potions.go:45` `quaffHandlers[…]`	`nil` handler: no effect (C's quaff switch had no default)
`scrolls.go:32` `readHandlers[…]`	`nil` handler
`potions.go:51`, `scrolls.go:39` `callIt(&Items.X[Which])`	skip the call-it prompt
`scrolls.go:170-172` (`readIdentify`, incl. the `data.idType[…]` index the issue table missed)	skip
`things.go:53` `nameScroll`	guard hoisted to `inventoryName`, which covers `nameScroll` plus its siblings (`PotColors`/`RingStones`/`WandType`/`WandMade`/`Sticks`/`Weapons`/`Armors` indexes); falls back to the bare `ObjectKind.String()` category name
`wizard.go:74` `aClass[Which]`	via a new `(*gameData).armorClass(which)` accessor, also used at `rip.go:149`, `things.go:115`, `potions.go:268` so the guard is actually complete
`weapons.go:162` `initWeaps[which]`	early return in `initWeapon` for an out-of-range kind
`sticks.go:505` `Items.WandType[Which]` in `fixStick`	out-of-range takes the existing non-staff branch (`1x1`); the `switch` below already has a `default`

4. Tests (`game/wizard_test.go`, new)

//nolint:testpackage header, t.Parallel() in every test:

the exact reproducer: testTerm scripted "/f", createObj(), assert no
panic and that nothing was added to the pack;
negative Which: input below 'a' (e.g. '_') and below '0' (e.g. '!');
one test per guarded family — wand (doZap), potion (quaff), scroll
(readScroll), armor (createObj armor path), weapon (initWeapon) —
each with a deliberately malformed pack object, asserting no panic;
a restore test proving a snapshot with an out-of-range Which is refused;
a "valid input still works" test so the guards are shown not to change
in-range behavior.

TestSeedCompatItemTables is the RNG-order canary; it must stay green
untouched.

5. Bookkeeping

make fmt, make check green. TODO.md gets a Completed Steps entry in
the same commit; per the PR #9 precedent for out-of-band issue work, Next
Step is left alone (it is still the unfinished coverage-broadening step).
Commit title ends with (closes #10).

## Implementation plan Read `wizard.c create_obj()` and `sticks.c do_zap()`/`fix_stick()` on `origin/c-master` (via `git show`, no checkout). Findings that shape the fix: - C's `create_obj()` stores the nibble unchecked (`obj->o_which = isdigit(ch) ? ch - '0' : ch - 'a' + 10`) and never validates. Every downstream use is either a `switch` (defined behavior in C, even for a bogus value) or a static-array read past the end (undefined). So C has **no defined user-facing behavior** to be faithful to here — this is exactly the "prefer a clean rejection" case. - `do_zap()`'s switch has no array index at all in C; the `otherwise` arm is the `MASTER`-only `"what a bizarre schtick!"` message, and then `obj->o_charges--` runs regardless. Without `MASTER`, C silently did nothing and still burned a charge. That gives the Go guard a zero-invention semantic: an out-of-range wand yields a `nil` handler, no effect, and `obj.Charges--` still runs. Restoring the message stays with #13. ### 1. One shared bounds predicate (`game/object.go`) - `whichLimit(kind ObjectKind) int` — how many entries the kind's per-kind tables have: `NumPotionTypes`, `NumScrollTypes`, `NumRingTypes`, `NumWandTypes`, `NumArmorTypes`; `NumWeaponTypes + 1` for weapons (the `Items.Weapons` array is deliberately sized `+1` for the `WeaponFlame` dragon-breath entry that `fireBolt` really does set on a live `Object`); `0` for food / amulet / gold / `KindNone`, whose `Which` is not a table index — those keep accepting anything, exactly as today. - `(*Object).hasValidWhich() bool` — `limit == 0 || 0 <= Which < limit`. ### 2. Reject at the two boundaries where a bad `Which` can enter - **Keyboard** (`createObj`): after reading the nibble, reject out-of-range with a message and return without adding to the pack. The message reuses C's existing `type_name()` vocabulary (`ObjectKind.String()`) rather than inventing a new noun. Weapons validate against `NumWeaponTypes` (not `+1`): `WeaponFlame` has a name-table entry but no `init_dam[]` row, so it is not creatable. A code comment records the deliberate divergence from 5.4.4. The check sits before any `rnd()` call, so RNG order is untouched for every valid input and for the rejected path alike. - **Save file** (`Restore`): validate every `Object` in the snapshot (`st.Objects`, `st.Player.Body.Pack`, each `st.Monsters[i].Pack`) and return a new `ErrSaveCorrupt`-style error rather than loading a game that will explode later. A save file is machine-written, so an out-of-range `Which` can only mean corruption or tampering. ### 3. Defensive guards at the listed dispatch sites None of these change behavior for any in-range value; each is an `if !obj.hasValidWhich()` short-circuit around the existing code: | Site | Guarded behavior | | --- | --- | | `sticks.go:34` `zapHandlers[…]` | treated as a `nil` handler: no effect, `Charges--` still runs (C's non-`MASTER` `otherwise`) | | `potions.go:45` `quaffHandlers[…]` | `nil` handler: no effect (C's quaff switch had no default) | | `scrolls.go:32` `readHandlers[…]` | `nil` handler | | `potions.go:51`, `scrolls.go:39` `callIt(&Items.X[Which])` | skip the call-it prompt | | `scrolls.go:170-172` (`readIdentify`, incl. the `data.idType[…]` index the issue table missed) | skip | | `things.go:53` `nameScroll` | guard hoisted to `inventoryName`, which covers `nameScroll` plus its siblings (`PotColors`/`RingStones`/`WandType`/`WandMade`/`Sticks`/`Weapons`/`Armors` indexes); falls back to the bare `ObjectKind.String()` category name | | `wizard.go:74` `aClass[Which]` | via a new `(*gameData).armorClass(which)` accessor, also used at `rip.go:149`, `things.go:115`, `potions.go:268` so the guard is actually complete | | `weapons.go:162` `initWeaps[which]` | early return in `initWeapon` for an out-of-range kind | | `sticks.go:505` `Items.WandType[Which]` in `fixStick` | out-of-range takes the existing non-staff branch (`1x1`); the `switch` below already has a `default` | ### 4. Tests (`game/wizard_test.go`, new) `//nolint:testpackage` header, `t.Parallel()` in every test: - the exact reproducer: `testTerm` scripted `"/f"`, `createObj()`, assert no panic and that nothing was added to the pack; - negative `Which`: input below `'a'` (e.g. `'_'`) and below `'0'` (e.g. `'!'`); - one test per guarded family — wand (`doZap`), potion (`quaff`), scroll (`readScroll`), armor (`createObj` armor path), weapon (`initWeapon`) — each with a deliberately malformed pack object, asserting no panic; - a restore test proving a snapshot with an out-of-range `Which` is refused; - a "valid input still works" test so the guards are shown not to change in-range behavior. `TestSeedCompatItemTables` is the RNG-order canary; it must stay green untouched. ### 5. Bookkeeping `make fmt`, `make check` green. `TODO.md` gets a **Completed Steps** entry in the same commit; per the PR #9 precedent for out-of-band issue work, **Next Step is left alone** (it is still the unfinished coverage-broadening step). Commit title ends with ` (closes #10)`.

clawbot referenced this issue from a commit

2026-08-09 07:09:08 +02:00

fix: bound wizard-created Which against its item table (closes #10)

clawbot referenced a pull request that will close this issue

2026-08-09 07:09:49 +02:00

Bound wizard-created Which against its item table (closes #10) #20

clawbot referenced this issue

2026-08-09 07:12:01 +02:00

Wizard createObj accepts an unrecognized type glyph and adds an inert KindNone object to the pack #21

clawbot referenced this issue

2026-08-09 07:18:47 +02:00

Bound wizard-created Which against its item table (closes #10) #20