From 398f7017d616973859c06f4398cdbd5ce87a50bf Mon Sep 17 00:00:00 2001 From: inogenous <123803852+inogenous@users.noreply.github.com> Date: Mon, 1 Jan 2024 12:08:09 +0100 Subject: [PATCH] Prevent segfault on unload_activity_actor Prevents segfault that previously happened when pouring liquid to ground from AIM using "examine" menu. Previous segfault being fixed: ``` #0 __gnu_cxx::__atomic_add_dispatch () at /usr/include/c++/13/ext/atomicity.h:111 #1 std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_add_ref_copy () at /usr/include/c++/13/bits/shared_ptr_base.h:152 #2 0x00005652b2df5540 in std::shared_ptr::shared_ptr () at /usr/include/c++/13/bits/shared_ptr.h:204 #3 item_location::item_location () at src/item_location.h:30 #4 item_location::impl::item_in_container::item_in_container () at src/item_location.cpp:612 #5 item_location::item_location () at src/item_location.cpp:767 #6 0x00005652b27fa6b1 in unload_activity_actor::unload () at src/activity_actor.cpp:3232 #7 0x00005652b32eed69 in player_activity::do_turn () at src/player_activity.cpp:383 #8 0x00005652b2b553e7 in do_turn () at src/do_turn.cpp:487 #9 0x00005652b25ea589 in main () at src/main.cpp:798 ``` The problem before was that the reference to `target` in `unload_activity_actor::unload` was an invalid reference. This was caused by: 1. `unload_activity_actor::finish` calls `act.set_to_null()`. 2. That sets the player activity to `null` type (=no current activity) 3. `unload_activity_actor::unload` is `static`, and takes `target` as a reference. 4. `::unload` calls `Character::add_or_drop_with_msg` which leads to call chain: `liquid_handler::consume_liquid` -> `get_liquid_target` -> `choose_adjacent` -> `choose_direction` -> `temp_hide_advanced_inv` -> `advanced_inventory::temp_hide` -> `advanced_inventory::do_return_entry` 5. `advanced_inventory::do_return_entry` assigns a new `ACT_ADV_INVENTORY` activity. This invalidates the previous `unload_activity_actor` because of pt 2 above. 6. When static method `unload_activity_actor::unload` resumes after its call to `add_or_drop_with_msg`, the reference to `target` is invalid because of pt 5. This commit attempt to fix the issue with invalidated `target` reference by copying its value before invalidating the activity. --- src/activity_actor.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/activity_actor.cpp b/src/activity_actor.cpp index 90e4939e5b3d8..1a997455e0d0b 100644 --- a/src/activity_actor.cpp +++ b/src/activity_actor.cpp @@ -3204,8 +3204,11 @@ void unload_activity_actor::start( player_activity &act, Character & ) void unload_activity_actor::finish( player_activity &act, Character &who ) { + // Need to copy `target` here because `::unload` is static and `this` might be invalidated + // after `act.set_to_null()` has been called (so `this.target` might also be invalidated) + item_location target_copy = target; act.set_to_null(); - unload( who, target ); + unload( who, target_copy ); } void unload_activity_actor::unload( Character &who, item_location &target )