Skip to content

Null-guard AreaInstance lookups in combat / death broadcasts#154

Merged
CoreyRDean merged 1 commit into
developfrom
harden/gameserver-areainstance-null
May 25, 2026
Merged

Null-guard AreaInstance lookups in combat / death broadcasts#154
CoreyRDean merged 1 commit into
developfrom
harden/gameserver-areainstance-null

Conversation

@CoreyRDean
Copy link
Copy Markdown
Collaborator

Summary

Six sites in GameServer.bb resolved Object.AreaInstance(actor\ServerArea) and immediately dereferenced AInstance\FirstInZone / AInstance\Spawned without checking the lookup succeeded. Object.X(handle) returns Null on a stale handle — typical trigger here is an actor mid-warp whose ServerArea was cleared before SetArea fully settled the new zone, or a zone-unload race against a script-spawned attack.

Sites

Site Function Behavior
KillActor:130-140 AI-death broadcast loop Tell players in zone the AI died
KillActor:191-202 Zone linked-list removal Detach the dead AI from FirstInZone chain
KillActor:204-207 Spawn-point counter Decrement Spawned[SourceSP]
FireProjectile:227 Visual broadcast for ranged shots Tell players to render the in-flight projectile
ActorAttack:303 Ranged-attack broadcast (CombatFormula <> 4) Tell area about the ranged attack
ActorAttack:546 Melee-attack broadcast Tell area about the melee attack

All six follow the same recovery: if AInstance is Null, skip the loop / counter update. The orphaned NextInZone pointers from a freed zone are dangling anyway — there's nothing to detach from, and no one to broadcast to. Authoritative damage / kill state still resolves on the server; only the visual / network broadcast is skipped.

Pattern reference

Established by the rcce2-packet-handler skill and recently documented in CLAUDE.md under "Handle-lookup Null discipline" (PR #145). This is the server-side application of the same Object.X(handle) guard pattern the client-side PR #144 added for PlayerTarget.

Out of scope (follow-up)

UpdateActorInstances at line 625+ has the same AInstance pattern at higher fanout (per-actor per-tick AInstance access for water damage, etc.). That surface needs a different shape of fix — skip the actor's entire tick, not just one broadcast — and lands in a separate PR.

Test plan

  • compile.bat -t builds Server / Client / GUE / Project Manager cleanly.
  • Force a kill of an actor whose ServerArea has been cleared mid-warp (e.g. a BVM script that calls KILLACTOR on an actor immediately after WARP). Server logs (well, doesn't crash) and the kill cleanup proceeds without the broadcast.
  • Sanity: normal in-area combat still propagates attack and death packets.

🤖 Generated with Claude Code

Six sites in GameServer.bb resolved Object.AreaInstance(actor\ServerArea)
and immediately dereferenced AInstance\FirstInZone / AInstance\Spawned
without checking the lookup succeeded. Object.X(handle) returns Null on
a stale handle -- typical trigger here is an actor mid-warp whose
ServerArea was cleared before SetArea fully settled the new zone, or a
zone-unload race against a script-spawned attack.

Sites:
  - KillActor:130-140   AI-death broadcast loop
  - KillActor:191-202   zone linked-list removal on AI death
  - KillActor:204-207   spawn-point counter decrement
  - FireProjectile:227  visual broadcast for ranged shots
  - ActorAttack:303     ranged-attack broadcast (CombatFormula <> 4)
  - ActorAttack:546     melee-attack broadcast (post-SkipAttackNet)

All six follow the same recovery: if AInstance is Null, skip the loop /
counter update. The orphaned NextInZone pointers from a freed zone are
dangling anyway -- there's nothing to detach from, and no one to
broadcast to. Authoritative damage / kill state still resolves on the
server; only the visual / network broadcast is skipped.

UpdateActorInstances at line 625+ has the same pattern at higher
fanout (per-actor per-tick AInstance access for water damage, etc.);
that surface needs a different shape of fix (skip the actor's entire
tick, not just one broadcast) and lands in a separate PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@CoreyRDean CoreyRDean requested a review from a team as a code owner May 25, 2026 17:14
@CoreyRDean CoreyRDean merged commit 4c3c17c into develop May 25, 2026
1 check passed
@CoreyRDean CoreyRDean deleted the harden/gameserver-areainstance-null branch May 25, 2026 17:15
CoreyRDean added a commit that referenced this pull request May 25, 2026
Follow-up to PR #154. Closes the rest of the AInstance Null-deref
sites in GameServer.bb:

1. UpdateActorInstances per-actor pass (line 650+) -- the entire per-
   tick update body now sits inside `If AInstance <> Null`. Body
   derefs AInstance\Area for waypoint movement, water damage, AI
   aggression, and several broadcast loops; each was a one-tick
   crash if ServerArea was stale (mid-warp, freed zone). Skip the
   tick rather than guard each individual site -- the actor's
   intrinsic state (HP, position) is preserved for when SetArea
   re-establishes the zone link.

2. UpdateActorInstances broadcast pass (line 986+) -- the
   per-RNID-player "tell them about other actors in their zone"
   loop now skips players whose area lookup is Null. They'll catch
   up on the next P_ChangeArea / P_StandardUpdate.

3. CommandPet pet-rename broadcast (line 1335+).

4. AILookForTargets (line 1350+) -- aggressive-NPC target acquisition
   bails early if its host area is gone.

5. AICallForHelp (line 1378+) -- "rally nearby NPCs" likewise bails.

Together with PR #154, every AInstance.AreaInstance call site in
GameServer.bb that previously crashed on a stale ServerArea is now
guarded. Cross-checked: SetArea (line 1094) and KillActor (already
in #154) were the two pre-existing guarded sites.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CoreyRDean added a commit that referenced this pull request May 25, 2026
Closes the AInstance-Null sweep for ServerNet.bb. Sites:

  - /me chat broadcast        (line ~284, "* AI\Name$ Params$")
  - /players zone count       (line ~399, "PlayersInZone N")
  - General chat broadcast    (line ~555, "<Name$> message")
  - P_Jump broadcast          (line ~930)
  - P_InventoryUpdate "P"     (line ~1409, pickup-removal)
  - P_InventoryUpdate "D"     (line ~1450, drop-broadcast)
  - SendEquippedUpdate        (line ~2645, gear-change broadcast)

Each was the same AInstance = Object.AreaInstance(...) /
A2 = AInstance\FirstInZone shape. A stale ServerArea (mid-warp,
freed zone) crashed the server on the next deref.

Same recovery: skip the broadcast on Null. The state change
already applies in-memory.

Continues the AInstance-Null sweep across rcce2 (PRs #154 / #155 /
#176 / #182 / #183 / #184 / #185 / #186).

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