diff --git a/TODO.md b/TODO.md index 2904373..29feb29 100644 --- a/TODO.md +++ b/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 diff --git a/game/object.go b/game/object.go index db67316..c5a48ef 100644 --- a/game/object.go +++ b/game/object.go @@ -164,6 +164,38 @@ func newObject() *Object { return &Object{Launch: noWeapon} } +// whichLimit reports how many entries a kind's per-kind tables hold, so +// Which is a legal index exactly while 0 <= Which < whichLimit(Kind). +// Kinds whose Which is not a table index at all — food (0 ration, 1 the +// fruit), the amulet, gold, and the KindNone an unrecognized glyph maps +// to — report 0, meaning any value is acceptable, which is what C did +// with them too. +// +// Weapons count one past NumWeaponTypes on purpose: Items.Weapons is +// sized NumWeaponTypes+1 for the WeaponFlame dragon-breath entry, and +// fireBolt really does set Which = WeaponFlame on a live Object. That +// entry has no init_dam[] row, so initWeapon and createObj bound +// themselves by NumWeaponTypes instead. +func whichLimit(kind ObjectKind) int { + //nolint:exhaustive // the remaining kinds do not index a per-kind table + switch kind { + case KindPotion: + return int(NumPotionTypes) + case KindScroll: + return int(NumScrollTypes) + case KindRing: + return int(NumRingTypes) + case KindWand: + return int(NumWandTypes) + case KindArmor: + return int(NumArmorTypes) + case KindWeapon: + return int(NumWeaponTypes) + 1 + } + + return 0 +} + // PotionKind returns Which as a potion kind; valid only for KindPotion. func (o *Object) PotionKind() PotionKind { return PotionKind(o.Which) } @@ -182,6 +214,36 @@ func (o *Object) WeaponKind() WeaponKind { return WeaponKind(o.Which) } // ArmorKind returns Which as an armor kind; valid only for KindArmor. func (o *Object) ArmorKind() ArmorKind { return ArmorKind(o.Which) } +// hasValidWhich reports whether Which is a legal index into this +// object's per-kind tables. Objects the game builds itself always +// satisfy it; the wizard-create command and a restored save file are the +// only two ways a malformed one can appear, and both reject it (see +// createObj and validateSnapshotObjects). +// +// The Which >= 0 arm is unreachable from the keyboard: createObj derives +// Which with byte arithmetic (int(ch-'a') + 10), which wraps to a large +// positive value rather than going negative. It is kept as +// defense-in-depth for the non-keyboard source — a decoded save file, +// where Which is an int off the wire and can hold anything — and is +// exercised there by TestRestoreRejectsOutOfRangeWhich. +func (o *Object) hasValidWhich() bool { + limit := whichLimit(o.Kind) + + return limit == 0 || (o.Which >= 0 && o.Which < limit) +} + +// wizardCanCreate reports whether Which names something the wizard-create +// command can actually build. It is hasValidWhich narrowed for weapons: +// WeaponFlame owns a name-table slot but no init_dam[] row, so asking for +// it would leave initWeapon nothing to copy. +func (o *Object) wizardCanCreate() bool { + if o.Kind == KindWeapon { + return o.Which >= 0 && o.Which < int(NumWeaponTypes) + } + + return o.hasValidWhich() +} + // attachObj is list.c attach(): push item onto the front of a list. func attachObj(list *[]*Object, item *Object) { *list = append([]*Object{item}, *list...) diff --git a/game/potions.go b/game/potions.go index 0fae76a..30827e3 100644 --- a/game/potions.go +++ b/game/potions.go @@ -42,13 +42,16 @@ func (g *RogueGame) quaff() { g.leavePack(obj, false, false) - if h := g.data.quaffHandlers[obj.PotionKind()]; h != nil { + if h := g.data.quaffHandler(obj); h != nil { h(g, trip) } g.status() - // Throw the item away - g.callIt(&g.Items.Potions[obj.Which]) + // Throw the item away. A malformed potion has no lore entry to name, + // so it is drunk for no effect and never prompts to be called anything. + if obj.hasValidWhich() { + g.callIt(&g.Items.Potions[obj.Which]) + } } // The per-potion effect handlers, dispatched through @@ -265,7 +268,7 @@ func (g *RogueGame) isMagic(o *Object) bool { //nolint:exhaustive // C-faithful: only the cases C handled (approved 2026-07-07) switch o.Kind { case KindArmor: - return o.Flags.Has(Protected) || o.ArmorClass != g.data.aClass[o.Which] + return o.Flags.Has(Protected) || o.ArmorClass != g.data.armorClass(o.Which) case KindWeapon: return o.HPlus != 0 || o.DPlus != 0 case KindPotion, KindScroll, KindWand, KindRing, KindAmulet: diff --git a/game/rip.go b/game/rip.go index 32fdbe1..a5682cf 100644 --- a/game/rip.go +++ b/game/rip.go @@ -132,6 +132,13 @@ func (g *RogueGame) totalWinner() { // objectWorth appraises one pack item on the way out, marking it known // (the switch of rip.c total_winner). func (g *RogueGame) objectWorth(obj *Object) int { + // Same defense as inventoryName: most arms below appraise from a + // per-kind table at obj.Which, so a malformed item is worth nothing + // rather than a panic on the way to the scoreboard. + if !obj.hasValidWhich() { + return 0 + } + it := &g.Items worth := 0 @@ -146,7 +153,7 @@ func (g *RogueGame) objectWorth(obj *Object) int { case KindArmor: worth = it.Armors[obj.Which].Worth worth += (9 - obj.ArmorClass) * 100 - worth += 10 * (g.data.aClass[obj.Which] - obj.ArmorClass) + worth += 10 * (g.data.armorClass(obj.Which) - obj.ArmorClass) obj.Flags.Set(Known) case KindScroll: worth = loreWorth(&it.Scrolls[obj.Which], obj.Count) diff --git a/game/save.go b/game/save.go index 095e8ab..8d52c67 100644 --- a/game/save.go +++ b/game/save.go @@ -680,6 +680,37 @@ func (g *RogueGame) AutoSave() { // ErrSaveOutOfDate reports a save file from an incompatible version. var ErrSaveOutOfDate = errors.New("sorry, saved game is out of date") +// ErrSaveCorrupt reports a save file whose contents are structurally +// impossible for a game this code could have written. +var ErrSaveCorrupt = errors.New("sorry, saved game is corrupt") + +// validateSnapshotObjects rejects a snapshot carrying an item whose Which +// would index past the end of its kind's tables. The file is written by +// this program, so such a value can only come from corruption or +// tampering; refusing it at the door is what keeps a malformed object +// from reaching the effect tables in doZap, quaff, and readScroll, where +// the wizard-create bug (issue #10) used to put one. +func validateSnapshotObjects(st *SaveState) error { + lists := make([][]Object, 0, 2+len(st.Monsters)) + lists = append(lists, st.Objects, st.Player.Body.Pack) + + for i := range st.Monsters { + lists = append(lists, st.Monsters[i].Pack) + } + + for _, list := range lists { + for i := range list { + obj := &list[i] + if !obj.hasValidWhich() { + return fmt.Errorf("%w: %s has out-of-range which %d", + ErrSaveCorrupt, obj.Kind, obj.Which) + } + } + } + + return nil +} + // Restore restores a saved game from a file (save.c restore). The file is // deleted, as in C, to defeat restarting from the same save. func Restore(path string, params Params) (*RogueGame, error) { @@ -702,6 +733,11 @@ func Restore(path string, params Params) (*RogueGame, error) { return nil, ErrSaveOutOfDate } + valErr := validateSnapshotObjects(&st) + if valErr != nil { + return nil, valErr + } + g := &RogueGame{ data: newGameData(), Rng: &Rng{}, diff --git a/game/scrolls.go b/game/scrolls.go index e0279fd..f2c2dd6 100644 --- a/game/scrolls.go +++ b/game/scrolls.go @@ -29,14 +29,17 @@ func (g *RogueGame) readScroll() { // Get rid of the thing g.leavePack(obj, false, false) - if h := g.data.readHandlers[obj.ScrollKind()]; h != nil { + if h := g.data.readHandler(obj); h != nil { h(g, obj) } g.look(true) // put the result of the scroll on the screen g.status() - g.callIt(&g.Items.Scrolls[obj.Which]) + // A malformed scroll has no lore entry to name (see quaff). + if obj.hasValidWhich() { + g.callIt(&g.Items.Scrolls[obj.Which]) + } } // The per-scroll effect handlers, dispatched through @@ -166,10 +169,15 @@ func (g *RogueGame) createMonsterSpot() (Coord, bool) { } func (g *RogueGame) readIdentify(obj *Object) { - // Identify, let him figure something out + // Identify, let him figure something out. idType is shorter than the + // scroll table keying it (it stops after the last identify scroll), + // so the filter lookup carries its own bound. That bound is not + // reachable today: readHandlers registers readIdentify only for the + // identify scrolls, all of which sit inside idType. It is kept as + // defense-in-depth against a future table resize. g.Items.Scrolls[obj.Which].Know = true g.msg("this scroll is an %s scroll", g.Items.Scrolls[obj.Which].Name) - g.whatis(true, g.data.idType[obj.ScrollKind()]) + g.whatis(true, g.data.identifyType(obj.ScrollKind())) } func (g *RogueGame) readMagicMapping(*Object) { diff --git a/game/sticks.go b/game/sticks.go index f346d5a..e93bc8c 100644 --- a/game/sticks.go +++ b/game/sticks.go @@ -31,7 +31,11 @@ func (g *RogueGame) doZap() { return } - if h := g.data.zapHandlers[obj.WandKind()]; h != nil { + // A malformed Which yields no handler, which is exactly what C did in a + // non-MASTER build: its do_zap switch matched no case, fell out, and + // still ran o_charges--. (The MASTER-only "what a bizarre schtick!" + // message for that arm is issue #13, not this bounds fix.) + if h := g.data.zapHandler(obj); h != nil { if !h(g, obj) { return // the zap aborted; no charge is used } @@ -502,7 +506,11 @@ func (g *RogueGame) boltStrikesHero(start Coord, name string, fromHero bool) boo // fixStick sets up a new wand or staff (sticks.c fix_stick). func (g *RogueGame) fixStick(cur *Object) { - if g.Items.WandType[cur.Which] == staffName { + // ws_type[] is indexed by Which; a malformed one is treated as a wand, + // which is the branch the C string compare would take against any + // value that is not literally "staff". The charge switch below already + // funnels everything but WandLight into its default arm. + if cur.hasValidWhich() && g.Items.WandType[cur.Which] == staffName { cur.Damage = dice("2x3") } else { cur.Damage = dice("1x1") diff --git a/game/tables.go b/game/tables.go index 90b0d83..c914f43 100644 --- a/game/tables.go +++ b/game/tables.go @@ -859,6 +859,69 @@ func newGameData() *gameData { } } +// The three effect-table lookups below are the guarded form of a raw +// index into quaffHandlers/readHandlers/zapHandlers. An object whose +// Which is out of range for its kind reports no handler rather than +// panicking, which lands on the same do-nothing behavior C's switches +// had when no case matched. + +// quaffHandler returns the potion effect for obj, or nil when obj is not +// a potion the table knows about. +func (d *gameData) quaffHandler(obj *Object) func(g *RogueGame, trip bool) { + if !obj.hasValidWhich() { + return nil + } + + return d.quaffHandlers[obj.PotionKind()] +} + +// readHandler returns the scroll effect for obj, or nil when obj is not +// a scroll the table knows about. +func (d *gameData) readHandler(obj *Object) func(g *RogueGame, obj *Object) { + if !obj.hasValidWhich() { + return nil + } + + return d.readHandlers[obj.ScrollKind()] +} + +// zapHandler returns the wand effect for obj, or nil when obj is not a +// wand the table knows about. +func (d *gameData) zapHandler(obj *Object) func(g *RogueGame, obj *Object) bool { + if !obj.hasValidWhich() { + return nil + } + + return d.zapHandlers[obj.WandKind()] +} + +// identifyType reads extern.c's identify-scroll to item-kind map, +// returning KindNone for any scroll past the last identify scroll (the +// table is shorter than the scroll table it is keyed by). No scroll that +// can reach readIdentify is past that end, so the guard is defensive +// rather than a live bound; it exists so a future table resize cannot +// turn the lookup into a panic. +func (d *gameData) identifyType(kind ScrollKind) ObjectKind { + if kind < 0 || int(kind) >= len(d.idType) { + return KindNone + } + + return d.idType[kind] +} + +// armorClass reads extern.c a_class[] for an armor object's Which, +// returning 0 when Which is out of range. Only a malformed object can +// hit that arm — createObj and Restore both reject one — but a bounds +// check here is what keeps a bad index from panicking the process +// instead of merely naming a suit of armor oddly. +func (d *gameData) armorClass(which int) int { + if which < 0 || which >= int(NumArmorTypes) { + return 0 + } + + return d.aClass[which] +} + // Version strings (vers.c). The encstr/statlist XOR keys are not ported: // the Go save format does not use them. const ( diff --git a/game/things.go b/game/things.go index b918663..c28d7cc 100644 --- a/game/things.go +++ b/game/things.go @@ -13,6 +13,16 @@ import ( func (g *RogueGame) inventoryName(obj *Object, drop bool) string { var pb strings.Builder + // Every arm below reaches into a per-kind name table at obj.Which: + // the potion colors, ring stones, wand material/type, scroll titles, + // and the weapon and armor name tables. An object with an out-of-range + // Which cannot reach here (createObj and Restore both reject one), but + // if one ever did, naming it must not take the process down — so it + // falls back to the bare category name from C's type_name() vocabulary. + if !obj.hasValidWhich() { + return obj.Kind.String() + } + which := obj.Which it := &g.Items @@ -112,7 +122,7 @@ func (g *RogueGame) nameArmor(pb *strings.Builder, obj *Object) { sp := g.Items.Armors[obj.Which].Name if obj.Flags.Has(Known) { fmt.Fprintf(pb, "%s %s [", - num(g.data.aClass[obj.Which]-obj.ArmorClass, 0, Armor), sp) + num(g.data.armorClass(obj.Which)-obj.ArmorClass, 0, Armor), sp) if !g.Options.Terse { pb.WriteString("protection ") diff --git a/game/weapons.go b/game/weapons.go index 7290a8e..1d6757c 100644 --- a/game/weapons.go +++ b/game/weapons.go @@ -159,6 +159,15 @@ type weaponSetup struct { // initWeapon sets up a new weapon (weapons.c init_weapon). func (g *RogueGame) initWeapon(weap *Object, which WeaponKind) { + // init_dam[] has a row only for the real weapons: WeaponFlame (dragon + // breath) and anything past it have none. createObj rejects such a + // choice before calling here, so this arm is unreachable in practice; + // it exists so a malformed kind leaves the weapon untouched instead of + // panicking on the table read. + if which < 0 || int(which) >= int(NumWeaponTypes) { + return + } + iwp := &g.data.initWeaps[which] weap.Kind = KindWeapon weap.Which = int(which) diff --git a/game/wizard.go b/game/wizard.go index 4b59628..26b641f 100644 --- a/game/wizard.go +++ b/game/wizard.go @@ -26,6 +26,23 @@ func (g *RogueGame) createObj() { obj.Count = 1 g.Msgs.Mpos = 0 + // Deliberate divergence from 5.4.4: C stored this nibble unchecked, so + // 'a'-'f' indexed straight past the ends of the per-kind static + // tables. Input outside '0'-'9' and 'a'-'f' overshoots much further: + // readchar returns a byte, so ch-'a' above is byte arithmetic and + // wraps instead of going negative ('A' gives 234, '!' gives 202). + // Reading past a static array was undefined behavior C happened to + // survive by picking up adjacent memory; in Go it is a panic that + // kills the process with the terminal still in raw mode. C had no + // defined behavior here to be faithful to, so the choice is rejected + // outright rather than emulating a garbage read. The check precedes + // every rnd() call below, so the RNG sequence is untouched either way. + if !obj.wizardCanCreate() { + g.msg("there is no such %s", obj.Kind) + + return + } + //nolint:exhaustive // C-faithful: only the cases C handled (approved 2026-07-07) switch obj.Kind { case KindWeapon, KindArmor: @@ -71,7 +88,7 @@ func (g *RogueGame) createWeaponArmor(obj *Object) { return } - obj.ArmorClass = g.data.aClass[obj.Which] + obj.ArmorClass = g.data.armorClass(obj.Which) if bless == '-' { obj.ArmorClass += g.rnd(3) + 1 } diff --git a/game/wizard_test.go b/game/wizard_test.go new file mode 100644 index 0000000..f6973b0 --- /dev/null +++ b/game/wizard_test.go @@ -0,0 +1,426 @@ +//nolint:testpackage // white-box tests reach unexported state (approved 2026-07-07) +package game + +import ( + "encoding/gob" + "errors" + "os" + "path/filepath" + "testing" +) + +// mustNotPanic runs fn and turns a panic into an ordinary test failure. +// The bug these tests cover (issue #10) panicked out of an array index, +// and an unrecovered panic would take the whole test binary down instead +// of reporting which dispatch regressed. +func mustNotPanic(t *testing.T, what string, fn func()) { + t.Helper() + + defer func() { + if r := recover(); r != nil { + t.Errorf("%s panicked: %v", what, r) + } + }() + + fn() +} + +// TestCreateObjWandFReproducer is the exact reported crash: wizard mode, +// C, '/' for a wand, 'f' for which. 'f' is nibble 15 and there are only +// NumWandTypes (14) wands, so fixStick used to index two past the end of +// ws_type[] and panic. +func TestCreateObjWandFReproducer(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + g.Wizard = true + before := len(g.Player.Pack) + + setInput(t, g, '/', 'f') + + mustNotPanic(t, "createObj with wand 'f'", g.createObj) + + if len(g.Player.Pack) != before { + t.Errorf("out-of-range wand was added to the pack: %d items, want %d", + len(g.Player.Pack), before) + } +} + +// TestCreateObjRejectsOutOfRangeWhich sweeps the rejection across every +// kind whose Which is a table index, including input outside '0'-'f'. +// isDigit is false for such input, so it takes the letter branch, where +// ch-'a' is byte arithmetic and wraps rather than going negative: 'A' +// (65) gives int(224)+10 == 234 and '!' (33) gives int(192)+10 == 202. +// Those far-past-the-end values, not negative ones, are what the guard +// has to catch on the keyboard path. +func TestCreateObjRejectsOutOfRangeWhich(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + typ byte + which byte + }{ + {"wand f is past NumWandTypes", Stick, 'f'}, + {"potion f is past NumPotionTypes", Potion, 'f'}, + {"ring f is past NumRingTypes", Ring, 'f'}, + // NumScrollTypes is 18, past the 'f' the prompt tops out at, so a + // scroll can only be driven out of range by input that wraps. + {"scroll 'A' wraps to 234", Scroll, 'A'}, + {"armor 9 is past NumArmorTypes", Armor, '9'}, + {"weapon 9 is the flame pseudo-weapon", Weapon, '9'}, + {"wand 'A' wraps to 234", Stick, 'A'}, + {"wand '!' wraps to 202", Stick, '!'}, + {"armor 'A' wraps to 234", Armor, 'A'}, + {"weapon '!' wraps to 202", Weapon, '!'}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + g.Wizard = true + before := len(g.Player.Pack) + + setInput(t, g, tc.typ, tc.which) + + mustNotPanic(t, tc.name, g.createObj) + + if len(g.Player.Pack) != before { + t.Errorf("pack grew to %d items, want %d: a rejected item was created", + len(g.Player.Pack), before) + } + }) + } +} + +// TestCreateObjAcceptsValidWhich pins the other half of the contract: the +// bounds check must not touch any in-range choice. +func TestCreateObjAcceptsValidWhich(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + typ byte + which byte + kind ObjectKind + want int + }{ + {"wand of light", Stick, '0', KindWand, int(WandLight)}, + {"potion 0", Potion, '0', KindPotion, int(PotionConfusion)}, + {"scroll 9", Scroll, '9', KindScroll, int(ScrollIdentifyRingOrStick)}, + {"ring d, the last ring", Ring, 'd', KindRing, int(NumRingTypes) - 1}, + {"armor 7, the last armor", Armor, '7', KindArmor, int(NumArmorTypes) - 1}, + {"weapon 8, the last real weapon", Weapon, '8', KindWeapon, int(WeaponSpear)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + g.Wizard = true + + before := make(map[*Object]bool, len(g.Player.Pack)) + for _, o := range g.Player.Pack { + before[o] = true + } + + // The armor and weapon arms read one more character for the + // blessing prompt; 'n' means neither cursed nor blessed. + setInput(t, g, tc.typ, tc.which, 'n') + g.createObj() + + if len(g.Player.Pack) != len(before)+1 { + t.Fatalf("pack has %d items, want %d: valid item not created", + len(g.Player.Pack), len(before)+1) + } + + // addPack files the new item in kind order, so find it by + // identity rather than assuming it landed at the end. + var made *Object + + for _, o := range g.Player.Pack { + if !before[o] { + made = o + } + } + + if made.Kind != tc.kind || made.Which != tc.want { + t.Errorf("created %v which %d, want %v which %d", + made.Kind, made.Which, tc.kind, tc.want) + } + }) + } +} + +// TestCreateObjKeepsRNGSequence proves the guard costs no RNG draws: a +// rejected creation must leave the generator exactly where it was, or +// every later roll in the game would shift. +func TestCreateObjKeepsRNGSequence(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + g.Wizard = true + before := g.Rng.Seed + + setInput(t, g, Stick, 'f') + g.createObj() + + if g.Rng.Seed != before { + t.Errorf("rejected creation consumed RNG: seed %d, want %d", + g.Rng.Seed, before) + } +} + +// malformed builds an object of the given kind whose Which sits one past +// the end of that kind's table — the state the wizard bug used to leave +// behind, and the state a corrupt save file could still describe. +func malformed(kind ObjectKind) *Object { + obj := newObject() + obj.Kind = kind + obj.Which = whichLimit(kind) + obj.Count = 1 + + return obj +} + +// TestZapMalformedWandDoesNotPanic covers the sticks.go dispatch. C's +// non-MASTER do_zap matched no case and still ran o_charges--, so the +// charge must be spent even though nothing happens. +func TestZapMalformedWandDoesNotPanic(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + wand := malformed(KindWand) + wand.Charges = 3 + ch := give(g, wand) + + setInput(t, g, ch) + + mustNotPanic(t, "doZap on a malformed wand", g.doZap) + + if wand.Charges != 2 { + t.Errorf("charges = %d after zapping, want 2", wand.Charges) + } +} + +// TestQuaffMalformedPotionDoesNotPanic covers the potions.go dispatch and +// the callIt lookup that follows it. +func TestQuaffMalformedPotionDoesNotPanic(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + before := len(g.Player.Pack) + ch := give(g, malformed(KindPotion)) + + setInput(t, g, ch) + + mustNotPanic(t, "quaff of a malformed potion", g.quaff) + + if len(g.Player.Pack) != before { + t.Errorf("pack has %d items, want %d: the potion was not consumed", + len(g.Player.Pack), before) + } +} + +// TestReadMalformedScrollDoesNotPanic covers the scrolls.go dispatch and +// its callIt lookup. +func TestReadMalformedScrollDoesNotPanic(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + before := len(g.Player.Pack) + ch := give(g, malformed(KindScroll)) + + setInput(t, g, ch) + + mustNotPanic(t, "readScroll of a malformed scroll", g.readScroll) + + if len(g.Player.Pack) != before { + t.Errorf("pack has %d items, want %d: the scroll was not consumed", + len(g.Player.Pack), before) + } +} + +// TestMalformedArmorDoesNotPanic covers the a_class[] reads: pricing at +// death, the identified-armor name, and the detect-magic test. +func TestMalformedArmorDoesNotPanic(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + armor := malformed(KindArmor) + armor.Flags.Set(Known) + + mustNotPanic(t, "naming a malformed suit of armor", func() { + if got := g.inventoryName(armor, false); got != armor.Kind.String() { + t.Errorf("inventoryName = %q, want %q", got, armor.Kind.String()) + } + }) + + mustNotPanic(t, "isMagic on a malformed suit of armor", func() { + g.isMagic(armor) + }) + + mustNotPanic(t, "appraising a malformed suit of armor", func() { + if worth := g.objectWorth(armor); worth != 0 { + t.Errorf("objectWorth = %d, want 0", worth) + } + }) + + if got := g.data.armorClass(armor.Which); got != 0 { + t.Errorf("armorClass(%d) = %d, want 0", armor.Which, got) + } +} + +// TestMalformedWeaponDoesNotPanic covers the init_dam[] read. WeaponFlame +// is the first kind with no table row, so initWeapon must leave the +// object alone rather than index past the end. +func TestMalformedWeaponDoesNotPanic(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + weap := newObject() + + mustNotPanic(t, "initWeapon with the flame pseudo-weapon", func() { + g.initWeapon(weap, WeaponFlame) + }) + + mustNotPanic(t, "initWeapon with a negative weapon kind", func() { + g.initWeapon(weap, WeaponKind(-1)) + }) + + if weap.Kind != KindNone { + t.Errorf("weapon was initialized from a missing table row: kind %v", + weap.Kind) + } +} + +// TestFixStickMalformedWhichDoesNotPanic covers the ws_type[] read that +// the reported reproducer actually crashed on. +func TestFixStickMalformedWhichDoesNotPanic(t *testing.T) { + t.Parallel() + + g := mkGameInput(t) + wand := malformed(KindWand) + + mustNotPanic(t, "fixStick on a malformed wand", func() { + g.fixStick(wand) + }) + + if wand.Damage.String() != "1x1" { + t.Errorf("damage = %q, want the wand damage %q", + wand.Damage.String(), "1x1") + } +} + +// TestRestoreRejectsOutOfRangeWhich is the save-file half of the fix: a +// malformed object must not be able to sneak past the keyboard guard by +// arriving in a snapshot. +// +// A decoded save is also the only place a *negative* Which can come +// from. On the keyboard path createObj's ch-'a' is byte arithmetic and +// wraps, so 'A' and '!' land at 234 and 202; Which is a plain int in the +// gob stream, so a tampered file can carry any value at all. Both shapes +// are covered here, and the negative case is what exercises the +// Which >= 0 arm of hasValidWhich. +func TestRestoreRejectsOutOfRangeWhich(t *testing.T) { + t.Parallel() + + cases := []struct { + name string + which int + }{ + {"one past the wand table", int(NumWandTypes)}, + {"the value 'A' wraps to on the keyboard path", 234}, + {"the value '!' wraps to on the keyboard path", 202}, + {"negative, reachable only from a tampered file", -1}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + g := mkGame(t, 11) + st := g.snapshot() + + if len(st.Player.Body.Pack) == 0 { + t.Fatal("starting pack is empty; nothing to corrupt") + } + + st.Player.Body.Pack[0].Kind = KindWand + st.Player.Body.Pack[0].Which = tc.which + + path := filepath.Join(t.TempDir(), "rogue.save") + writeSnapshot(t, path, st) + + _, restoreErr := Restore(path, Params{Term: &testTerm{}}) + if !errors.Is(restoreErr, ErrSaveCorrupt) { + t.Errorf("Restore error = %v, want ErrSaveCorrupt", restoreErr) + } + + _, statErr := os.Stat(path) + if statErr != nil { + t.Error("a rejected save file was deleted; it should be left alone") + } + }) + } +} + +// writeSnapshot gob-encodes a snapshot to path the way saveFile does. +func writeSnapshot(t *testing.T, path string, st *SaveState) { + t.Helper() + + f, err := os.Create(path) //nolint:gosec // G304: test temp path + if err != nil { + t.Fatal(err) + } + + encErr := gob.NewEncoder(f).Encode(st) + if encErr != nil { + t.Fatal(encErr) + } + + closeErr := f.Close() + if closeErr != nil { + t.Fatal(closeErr) + } +} + +// TestWhichLimitCoversEveryIndexedTable pins the bounds table itself +// against the per-kind arrays it has to agree with. +func TestWhichLimitCoversEveryIndexedTable(t *testing.T) { + t.Parallel() + + // len() of an array field is a compile-time constant, so the zero + // value is enough to read the table sizes off. + var it ItemLore + + cases := []struct { + kind ObjectKind + size int + }{ + {KindPotion, len(it.Potions)}, + {KindScroll, len(it.Scrolls)}, + {KindRing, len(it.Rings)}, + {KindWand, len(it.Sticks)}, + {KindArmor, len(it.Armors)}, + {KindWeapon, len(it.Weapons)}, + } + + for _, tc := range cases { + if got := whichLimit(tc.kind); got != tc.size { + t.Errorf("whichLimit(%v) = %d, want the table size %d", + tc.kind, got, tc.size) + } + } + + // Kinds whose Which is not a table index accept anything, as in C. + for _, kind := range []ObjectKind{KindFood, KindAmulet, KindGold, KindNone} { + obj := &Object{Kind: kind, Which: 99} + if !obj.hasValidWhich() { + t.Errorf("%v should not be bounds-checked on Which", kind) + } + } +}