Skip to content

Commit

Permalink
Prevent segfault when unloading from spillable container
Browse files Browse the repository at this point in the history
Prevents segfault from accessing an invalidated `item_location` when
unloading from a container that itself is inside a spillable container.

The previous segfault could be reproduced by having an inventory like:
```
backpack >
  steel_pan >
    bottle_plastic_small > antacid tablet (1)
    bottle_plastic_small > antacid tablet (4)
```
And then unloading the single pill bottle caused a segfault. What
happens is that part of unloading will trigger the steelpan to spill
its contents. Part of spilling the contents makes the `item_location`
invalidated, as previously documented in `::handle_contents_changed` in
`character.h`.

This commit instead makes sure that we do not use `item_location` or the
item after we have called `contents_change_handler::handle_by`. From the
example above, the steelpan will still spill its contents, but the
segfault can no longer be reproduced. This commmit also adds a
user-visible message when spilling non-liquid items.
  • Loading branch information
inogenous committed Jul 28, 2024
1 parent b7ae537 commit 07f3682
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 0 deletions.
6 changes: 6 additions & 0 deletions src/activity_actor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3496,6 +3496,12 @@ void unload_activity_actor::unload( Character &who, item_location &target )
it.on_contents_changed();
who.invalidate_weight_carried_cache();
handler.handle_by( who );
// Warning: the above call to `contents_change_handler::handle_by` will
// call `Character::handle_contents_changed`, which might invalidate items
// and item_locations. See description for `::handle_contents_changed`
// in character.h .
// Therefore, it is important that we don't use `target` or `it` after here.
break;
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/item_pocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,8 @@ void item_pocket::handle_liquid_or_spill( Character &guy, const item *avoid )
item i_copy( *iter );
guy.i_add_or_drop( i_copy, 1, avoid, &*iter );
iter = contents.erase( iter );
guy.add_msg_if_player( m_warning, _( "The %s falls out of the %s." ), i_copy.display_name(),
get_name() );
}
}
}
Expand Down
53 changes: 53 additions & 0 deletions tests/item_pocket_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2873,3 +2873,56 @@ TEST_CASE( "pocket_mods", "[pocket][toolmod][gunmod]" )
}
}
}

// Reproduce previous segfault from https://github.com/CleverRaven/Cataclysm-DDA/issues/75156
TEST_CASE( "unload_from_spillable_container", "[item][pocket]" )
{
clear_avatar();
clear_map();
avatar &u = get_avatar();
map &here = get_map();
item pill( "tums" ); // "antacid pill"
REQUIRE( u.wear_item( item( "backpack" ) ) );
item_location backpack_loc = u.top_items_loc().front();
item *backpack = backpack_loc.get_item();
GIVEN( "spillable container contains two pillbottles, one with 4 pills and one with 1 pill" ) {
// Starting inventory looks like:
// backpack >
// steel_pan >
// bottle_plastic_small > antacid tablet (1)
// bottle_plastic_small > antacid tablet (4)
// It's a bit odd that we managed to put bottles into the frying
// pan in the first place, since the pan would normally reject that
// with a message stating that it would spill.
REQUIRE( backpack->put_in( item( "steel_pan" ), pocket_type::CONTAINER ).success() );
item *steelpan = backpack->all_items_top().front();
REQUIRE( steelpan->put_in( pill.in_its_container( 1 ), pocket_type::CONTAINER ).success() );
REQUIRE( steelpan->put_in( pill.in_its_container( 4 ), pocket_type::CONTAINER ).success() );
WHEN( "unload the pillbottle that only has one pill" ) {
item *bottle1 = steelpan->all_items_top().front();
item_location steelpan_loc( backpack_loc, steelpan );
item_location bottle1_loc( steelpan_loc, bottle1 );
unload_activity_actor::unload( u, bottle1_loc );
THEN( "pill is unloaded into bottle that previously had 4 pills, remaining empty bottle is kept" ) {
// Expected inventory after unloading should be:
// backpack >
// steel_pan
// bottle_plastic_small
// bottle_plastic_small > antacid tablet (5)
CHECK( here.i_at( u.pos_bub() ).size() == 0 ); // no items spilled to ground

Check failure on line 2912 in tests/item_pocket_test.cpp

View workflow job for this annotation

GitHub Actions / build (other)

the 'empty' method should be used to check for emptiness instead of 'size' [readability-container-size-empty,-warnings-as-errors]
CHECK( u.top_items_loc().size() == 1 ); // backpack is still only inventory item
const std::list<item *> backpack_items_list = backpack->all_items_top();
const std::vector<item *> backpack_items_vec( backpack_items_list.begin(),
backpack_items_list.end() );
CHECK( backpack_items_vec.size() == 3 );
CHECK( backpack_items_vec[0]->typeId().str() == "steel_pan" );
CHECK( backpack_items_vec[0]->empty() );
CHECK( backpack_items_vec[1]->typeId().str() == "bottle_plastic_small" );
CHECK( backpack_items_vec[1]->empty() );
CHECK( backpack_items_vec[2]->typeId().str() == "bottle_plastic_small" );
CHECK( backpack_items_vec[2]->all_items_top().size() == 5 );
CHECK_FALSE( static_cast<bool>( bottle1_loc ) );
}
}
}
}

0 comments on commit 07f3682

Please sign in to comment.