sneak/rgoue
1
0
Fork 0
Code Issues 10 Pull Requests Actions Packages Projects Releases Wiki Activity

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

New Issue
Closed
opened 2026-08-09 03:43:34 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 03:43:34 +02:00
Collaborator
Copy Link

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

  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.

## 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
Author
Collaborator
Copy Link

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)`.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
Review passed and gate is green; ready to merge
 needs-checks
make check is failing on this PR
 needs-rebase
PR conflicts with main and must be rebased
 needs-review
PR awaiting independent adversarial review
 needs-rework
Review found problems; author must rework
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/rgoue#10
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?