Skip to content

npc: Deregister events with `ev_db` on free

Free Yorp requested to merge an-eventful-investigation into master

In serverdata@c3b7fe59, magic-knuckles made use of temporary copies of the spell NPC via puppet/destroy to provide additional functionality. This implementation was correct with respect to tmwa behaviour as documented.

However, puppet and destroy doesn't quite return the server to a clean prior state. In builtin_puppet, events with the fresh new copy of NPC are registered into the global ev_db map to track named events, such as OnDischarge, which might be triggered later. However, these registrations are not reversed with destroy, leaving ev_db to retain references to now invalid and destroyed NPCs.

serverdata@c3b7fe59 revealed this oversight through an intersection of rare conditions:

  • An NPC that is routinely cloned and destroyed
  • Having a variety of events
  • Including one that can be triggered during a search for the event over ALL NPCs, when #discharge is cast.

To reproduce, compile with -fsanitize=address -fsanitize=undefined to more reliably catch use of invalid memory, cast magic-knuckles, log out the character which cast magic-knuckles to destroy the NPC, log back in, then cast #discharge. The server will then crash.

Special thanks to SystemError for testing out this theory. Currently lacking a test environment of my own, this would not have been possible without his patience and diligence.

Edited by Free Yorp

Merge request reports