Skip to content

Commit

Permalink
Prevent segfault on unload_activity_actor
Browse files Browse the repository at this point in the history
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<item_location::impl>::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.
  • Loading branch information
inogenous committed Jan 1, 2024
1 parent ff71dff commit 398f701
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion src/activity_actor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 )
Expand Down

0 comments on commit 398f701

Please sign in to comment.