Skip to content

Guard recon/recoff against nil Spellups["RT"][rc] to prevent state corruption#103

Open
rodarvus wants to merge 1 commit into
zzyzzyzzx:masterfrom
rodarvus:fix/recon-recoff-nil-rt-crash
Open

Guard recon/recoff against nil Spellups["RT"][rc] to prevent state corruption#103
rodarvus wants to merge 1 commit into
zzyzzyzzx:masterfrom
rodarvus:fix/recon-recoff-nil-rt-crash

Conversation

@rodarvus

Copy link
Copy Markdown

Summary

{recon}sn,timer and {recoff}sn carry a recovery group number (the rc from slist recoveries), not a skill number. Spellups["RT"][rc] is only populated by SlistUpdate for recovery groups where the player has a practiced spell anchor (rc ~= -1 and pc > 1 at SpellupRecast/Hadar_Spellups.xml:984). If the player casts a spell whose recovery group has no practiced anchor — e.g. an attack spell they only just learned, or any group missing because slist was interrupted — both recon() and recoff() dereference Spellups["RT"][sn] on a nil and crash.

The crash is silently destructive:

  1. recon() writes Spellups["RT"]["affectedrecovery"][sn] = tm before the metadata read. The stale entry is committed to state.
  2. The next line throws on nil indexing. The plugin keeps running.
  3. OnPluginSaveState serializes the partially-populated table to disk.
  4. Eventually {recoff}sn fires. recoff()'s very first line is the same nil-deref. The cleanup at the end — the only place that clears affectedrecovery[sn] — never runs.
  5. The stale entry persists. Across OnPluginEnable it's loaded back from disk. Across MUSHclient restarts.
  6. From that point on, every {affoff} for any spell in that recovery group hits the check at line ~1158, sees the group still "in cooldown", and silently skips the recast. The user sees: certain spellups stop autocasting and never recover, even after /reload or restart.
  7. Recovery requires manually deleting the plugin state file.

This was reported in the wild by a user who hit it casting hex of entropy (a Necromancer subclass attack spell in recovery group Hex) shortly after learning it. With Hex of Entropy at low practice and no other Hex-group spell to anchor Spellups["RT"][40], the crash chain above fires on the very first cast.

Fix

Guard both reads:

  • recon — write affectedrecovery (unchanged), then look up Spellups["RT"][sn] once; log either the spell name or a generic "unknown recovery group" debug line. No state-mutating path changes.
  • recoff — look up Spellups["RT"][sn] once at the top. If nil, still clear affectedrecovery[sn] (the cleanup the old code never reached on this path) and early-return. Otherwise behave exactly as before.

The unknown-group case becomes a safe no-op + debug log instead of a crash + corrupted state.

Diff overview

-- recon: was
hadarprint("Recovery on:"..Spellups["RT"][sn]["name"]..  -- ← crash if RT[sn] nil

-- recon: now
local entry = Spellups["RT"][sn]
if entry then
    hadarprint("Recovery on:"..entry["name"]..", For:"..tm,"debug")
else
    hadarprint("Recovery on unknown group "..sn.." for "..tm.."s","debug")
end
-- recoff: was
local skillnum = Spellups["RT"][sn]["skillnum"]  -- ← crash if RT[sn] nil
local skilltype = Spellups["RT"][sn]["type"]
local skillname = Spellups["RT"][sn]["name"]
-- ... cleanup at end never reached on crash ...

-- recoff: now
local entry = Spellups["RT"][sn]
if entry == nil then
    Spellups["RT"]["affectedrecovery"][sn] = nil
    hadarprint("recoff for unknown recovery group "..sn..", cleared","debug")
    return
end
local skillnum = entry["skillnum"]
local skilltype = entry["type"]
local skillname = entry["name"]
-- ... existing logic unchanged ...

Test plan

  • Static analysis confirms the nil-deref path: Spellups["RT"][rc] is only written by SlistUpdate :986 and only when pc > 1. Any {recon} for a group without a practiced anchor hits this path.
  • Behavior for the happy path (RT[sn] populated) is unchanged. The same metadata is read, the same downstream logic runs.
  • In-game verification of the unknown-group path: cast an attack spell whose recovery group has no other practiced anchor, observe debug log line instead of error and confirm subsequent affoff/recoff cycles behave normally.
  • Recommended remediation for users with already-corrupted state: delete the plugin's state file before reload (this PR prevents future occurrences but does not heal an already-stale affectedrecovery table).

🤖 Generated with Claude Code

{recon}sn,timer and {recoff}sn carry a recovery GROUP number, not a
skill number. Spellups["RT"][rc] is only populated by SlistUpdate for
recovery groups where the player has a practiced spell anchoring them
(pc > 1). If the player casts a spell whose recovery group has no
practiced anchor in their slist -- e.g. an attack spell they just
learned, or any group missing because slist was interrupted -- both
recon() and recoff() dereference Spellups["RT"][sn] on a nil and
crash.

The crash is silently destructive: recon() writes
Spellups["RT"]["affectedrecovery"][sn] = tm BEFORE the metadata read,
so the stale entry is committed to state. recoff() crashes on the
very first line, so the cleanup at the end -- the only place that
clears affectedrecovery[sn] -- never runs. The stale entry then
persists to disk via OnPluginSaveState, survives across MUSHclient
restarts, and silently blocks all future autocasts for any spell in
that recovery group, because affoff()'s recovery check at line ~1158
sees the group as still in cooldown. Recovery requires manually
deleting the state file.

Guard both reads against a nil Spellups["RT"][rc]:

  - recon(): write the affectedrecovery state, then log either with
    the entry metadata or with a generic "unknown group" message.
  - recoff(): if the entry is nil, still clear affectedrecovery[sn]
    (the cleanup that the old code never reached on this path) and
    return early. Otherwise behave exactly as before.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant