Prevent crash from using invalidated explosion data

[inogenous]

Summary

Bugfixes "Prevent crash from using invalidated explosion data"

Purpose of change

Prevents crashes that previously happened from using references to explosion_data that had been invalidated.

Fixes #75686 .

Describe the solution

The problem before happened when killing "unfolded impossibility" in LIXA facility with a grenade, since this monster runs an eoc that switches maps. That led to explosion_handler::process_explosions being called twice recursively. First invocation iterates references inside this vector, and the second invocation might append to the vector, and most certaily will clear the vector, thus invalidating references for the first invocation.

Example crash being fixed by this commit, notice how explosion_handler::process_explosions occurs twice in the callstack:

Thread 1 "cataclysm-tiles" received signal SIGABRT, Aborted.
__pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44

(gdb) bt
 #0  __pthread_kill_implementation (threadid=<optimized out>, signo=signo@entry=6, no_tid=no_tid@entry=0) at ./nptl/pthread_kill.c:44
 #1  0x00007ffff787840f in __pthread_kill_internal (signo=6, threadid=<optimized out>) at ./nptl/pthread_kill.c:78
 #2  0x00007ffff78294f2 in __GI_raise (sig=sig@entry=6) at ../sysdeps/posix/raise.c:26
 #3  0x00007ffff78124ed in __GI_abort () at ./stdlib/abort.c:79
 #4  0x00007ffff7ad501e in std::__glibcxx_assert_fail(char const*, int, char const*, char const*) () from /lib/x86_64-linux-gnu/libstdc++.so.6
 #5  0x000055555688b471 in std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*> >::operator[] (this=<optimized out>, __n=1967424379) at /usr/include/c++/14/bits/stl_vector.h:1128
 #6  std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*> >::operator[] (this=<optimized out>, __n=1967424379) at /usr/include/c++/14/bits/stl_vector.h:1128
 #7  string_identity_static::get_interned_string[abi:cxx11](int) (id=1967424379) at src/string_id.cpp:51
 #8  0x0000555555fffed7 in string_identity_static::str[abi:cxx11]() const (this=<optimized out>) at src/string_id.h:140
 #9  0x0000555556057423 in string_id<itype>::c_str (this=this@entry=0x5555a126a718) at src/string_id.h:253
 #10 0x00005555560be56a in Item_factory::find_template (this=0x55555751cad0, id=...) at src/item_factory.cpp:2563
 #11 0x0000555555e4b0c5 in explosion_handler::_make_explosion (source=<optimized out>, p=..., ex=...) at /usr/include/c++/14/bits/unique_ptr.h:193
 #12 0x0000555555e4b704 in explosion_handler::process_explosions () at src/explosion.cpp:923
 #13 0x0000555556246e97 in map::actualize (this=this@entry=0x5555a12bf890, grid=...) at src/map.cpp:9168
 #14 0x00005555562472c0 in map::load (this=this@entry=0x5555a12bf890, w=..., update_vehicle=update_vehicle@entry=true, pump_events=pump_events@entry=false) at src/map.cpp:8387
 #15 0x0000555555de2546 in tinymap::load (this=this@entry=0x5555a12bf890, w=..., update_vehicles=update_vehicles@entry=true, pump_events=pump_events@entry=false) at src/map.h:2765
 #16 0x00005555562a7e6f in update_mapgen_function_json::update_map (this=0x55555a8ce8f0, omt_pos=..., args=..., offset=..., miss=miss@entry=0x0, verify=verify@entry=true, mirror_horizontal=false, mirror_vertical=false, rotation=0) at src/mapgen.cpp:8015
 #17 0x00005555562a81e3 in run_mapgen_update_func (update_mapgen_id=..., omt_pos=..., args=..., miss=miss@entry=0x0, cancel_on_collision=cancel_on_collision@entry=true, mirror_horizontal=mirror_horizontal@entry=false, mirror_vertical=false, rotation=0) at src/mapgen.cpp:8117
 #18 0x00005555565a0250 in operator() (__closure=<optimized out>, d=...) at src/npctalk.cpp:3964
 #19 0x000055555659c7a4 in talk_effect_t::apply (this=this@entry=0x55555d5211d8, d=...) at src/npctalk.cpp:6526
 #20 0x0000555555e0c9ba in effect_on_condition::activate (this=0x55555d521160, d=..., require_callstack_check=require_callstack_check@entry=true) at src/effect_on_condition.cpp:329
 #21 0x00005555561f9693 in spell_effect::effect_on_condition (sp=..., caster=..., target=...) at src/magic_spell_effect.cpp:1806
 #22 0x00005555561d101e in spell::cast_all_effects (this=0x7fffffffc788, source=..., target=...) at src/magic.cpp:1907
 #23 0x0000555556463bec in monster::die (this=0x55559c7fc0a0, nkiller=0x0) at src/monster.cpp:2941
 #24 0x0000555555d33386 in Creature::deal_projectile_attack (this=0x55559c7fc0a0, source=0x0, attack=..., print_messages=<optimized out>, wp_attack=...) at src/creature.cpp:1311
 #25 0x0000555556465049 in monster::deal_projectile_attack (this=this@entry=0x55559c7fc0a0, source=source@entry=0x0, attack=..., print_messages=print_messages@entry=false, wp_attack=...) at src/monster.cpp:2212
 #26 0x0000555555e4a9d9 in explosion_handler::shrapnel (range=-1, source=<optimized out>, src=..., power=<optimized out>, casing_mass=<optimized out>, per_fragment_mass=<optimized out>) at src/explosion.cpp:463
 #27 explosion_handler::_make_explosion (source=<optimized out>, p=..., ex=...) at src/explosion.cpp:536
 #28 0x0000555555e4b704 in explosion_handler::process_explosions () at src/explosion.cpp:923
 #29 0x0000555555dcf46f in do_turn () at src/do_turn.cpp:648
 #30 0x00005555557a1227 in main (argc=<optimized out>, argv=<optimized out>) at src/main.cpp:873

In the crash above, the explosion_data has been invalidated:

(gdb) frame 11
193           pointer    _M_ptr() const noexcept { return std::get<0>(_M_t); }

(gdb) print ex
$1 = (const explosion_data &) @0x5555a126a6f8: {power = 1.75295132e+25, distance_factor = 2.76847299e+20, max_noise = 1667855474, fire = 117, shrapnel = {casing_mass = 1936026889, fragment_mass = 6.82915174e+22, recovery = -1584519120, drop = {_version = 13059389229367304, _cid = 2019155690, _id = {
        _id = 1967424379}}}}

(gdb) print ex.shrapnel.drop
$2 = {_version = 13059389229367304, _cid = 2019155690, _id = {_id = 1967424379}}

Describe alternatives you've considered

Testing

Tested the savegame from #75686 extensively with additional debuglogging added (not commited here). Can no longer reproduce the cases where invalidated data was used.

Additional context

When testing the steps from #75686 , the game shows the grenade explosion twice when the monster dies, which could be interpreted as a separate bug. But I believe that's a consequence of explosion_handler::process_explosions running twice and incorrectly re-evaluating the grenade explosion again. On this branch, it correctly only shows up as one explosion.

[PatrikLundell]

An item triggering an explosion is processed and its associated explosion is stored into the _explosions vector, and once all items have been processed and the triggers somehow (I haven't checked how) have been removed, the process_explosions operation is called.

Critters, on the other hand, are part of the explosions_ element data, so they're not defused. If they're triggering a map load, they'll trigger the same explosion again (and if you're really lucky, they'll keep teleporting until you run out of stack...).

Thus, I think you're addressing the symptoms rather than the cause, i.e. a need to disarm critters once they're processed by the process_explosions loop. So, the question for this particular critter is: should it teleport leaving behind an explosion, or should it teleport and explode at the destination? If it's the first, the explosion processing should detach the explosion from the critter and tie it to its current location and then teleport the critter. If it's the latter, the actualize code called prior to the call to process_explosions should teleport the critter and the explosion handling should execute the explosion in place.

Edit: After a bit of additional thought, I think the actualization code should detach explosions from critters regardless, and it should be up to that code to handle teleportation before or after adding the detached explosion data to explosions_. That way any explosions triggered in the submap the critter teleports to gets to be added to the vector and processed there, before returning to the continued actualization processing of the primary submap to add any additional explosions and process those. We wouldn't recurse within the handle_explosions loop.

I first thought it might be possible to always detach explosions from the critter, but it might not be that simple, since critters exploding and releasing other critters (like the flying nasty buggers, for instance) should destroy the host, but not the spawned monsters. Thus, it might be that only critters that teleport need to be pre processed (because they are the only ones triggering recursion).

[inogenous]

Thanks for the input @PatrikLundell .

a need to disarm critters once they're processed by the process_explosions loop

Would the static bool process_explosions_in_progress from #75567 solve this in a more efficient way you think?

[PatrikLundell]

No, not really.

Guarding all calls to process_explosions by the static variable would only work for the case where the critter leaves an explosion behind and then teleport, but if the teleportation takes it to somewhere that has a pending explosion that pending explosion is triggered but not executed, and if you actually want to teleport first and explode at the destination that too would be blocked (although the trigger wouldn't be disabled, so when that map is loaded later it might either explode or teleport again (possibly to the same location, but the recursion check won't know that).

Conclusion: Unintended recursion is nasty business, and even with intended recursion you have to be quite careful.

[inogenous]

Ok, good to know. Maybe we're thinking of different scenarios here. The problem in #75686 is:

1. Player throws grenade at monster
2. process_explosions executes and iterates through the vector, keeping & references to objects inside
   2.1. Shrapnel from the grenade kills the monster
   2.2. When the monster dies, it runs an eoc that teleports the player
   2.3. When teleporting the player, map::actualize calls process_explosions again
   2.4. process_explosions from step 2.3 clears the vector, invalidating the references in step 2 above. <- this is the problem.
3. References are now invalidated
4. process_explosions from step 2 clears the vector.

This change instead makes it so that objects are copied, and step 2 clears the vector instead of step 4. So there are never any queued explosions lingering around when process_explosions runs.

[PatrikLundell]

Yes, I misunderstood the scenario in thinking the monster was teleported rather than the PC.

The PC shouldn't really be moved until the processing is done, because all explosions are simultaneous, with only essentially random factors determining the order in which they are processed, and a later explosion might hurt the PC if it isn't teleported away beforehand.
Also, teleporting the PC may cause some explosions to fall outside of the reality bubble and thus just be erased without being processed with the current code (they'd still be processed by my PR).

Your code would indeed separate the explosions in the two locations safely, although they'd be performed in an incorrect order that probably never matters.

Teleporting in the middle of a map::shift (moving the reality bubble) via the explosion handling causes the map::shift handling to perform actualization for submaps map::load has already actualized as part of the teleportation, but that's probably harmless.

In summary: Your solution should work for the case where the PC is teleported, which probably is the only relevant case, because come to think about it, an asshole monster dropping an explosion and teleporting away (or exploding and then is resurrected elsewhere) shouldn't actually cause recursion, because it should just be moved, but no map should actually be loaded at the target location, so no recursion should happen. I think I've been over thinking the situation.