Skip to content

Commit

Permalink
Prevent segfault when refitting item in spillable container
Browse files Browse the repository at this point in the history
Prevents segfault from accessing an invalidated `item_location` when
refitting an item that was stored inside a spillable container.

The previous segfault could be reproduced by having an inventory like:
```
backpack >
  aluminum can >
    work t-shirt (poor fit)
```
And then using a tailor's kit to refit the t-shirt caused a segfault.
Part of refitting will invalidate items inside the aluminum can, which
is a spillable container, and dereferencing the tshirt wil segfault.

This change instead makes sure that we do not use `fix_location` after
we have called `repair_item_actor::repair`. It also adds a unit test
that reproduced the previous segfault (but works on this branch).
  • Loading branch information
inogenous committed Aug 15, 2024
1 parent 238454f commit 7d59354
Show file tree
Hide file tree
Showing 2 changed files with 87 additions and 7 deletions.
6 changes: 6 additions & 0 deletions src/activity_handlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2482,6 +2482,11 @@ void repair_item_finish( player_activity *act, Character *you, bool no_menu )
const int old_level = you->get_skill_level( actor->used_skill );
const repair_item_actor::attempt_hint attempt = actor->repair( *you, *used_tool,
fix_location, repeat == repeat_type::REFIT_ONCE || repeat == repeat_type::REFIT_FULL );
// Warning: The above call to `repair_item_actor::repair` might
// invalidate the item and the item_location, for example when
// spilling items from spillable containers. It is therefore
// important that we don't use `fix_location` in code below
// here without first checking whether it is still valid.

// If the item being repaired has been destroyed stop further
// processing in case the items being used for the repair was
Expand Down Expand Up @@ -2512,6 +2517,7 @@ void repair_item_finish( player_activity *act, Character *you, bool no_menu )
// But only if we didn't destroy the item (because then it's obvious)
const bool destroyed = attempt == repair_item_actor::AS_DESTROYED;
const bool cannot_continue_repair = attempt == repair_item_actor::AS_CANT || destroyed ||
!fix_location ||
!actor->can_repair_target( *you, *fix_location, !destroyed, true );
if( cannot_continue_repair ) {
// Cannot continue to repair target, select another target.
Expand Down
88 changes: 81 additions & 7 deletions tests/degradation_test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

#include "activity_handlers.h"
#include "cata_catch.h"
#include "flag.h"
#include "item.h"
#include "itype.h"
#include "iuse_actor.h"
Expand All @@ -17,8 +18,12 @@

static const activity_id ACT_REPAIR_ITEM( "ACT_REPAIR_ITEM" );

static const itype_id itype_backpack( "backpack" );
static const itype_id itype_can_drink( "can_drink" );
static const itype_id itype_canvas_patch( "canvas_patch" );
static const itype_id itype_leather( "leather" );
static const itype_id itype_tailors_kit( "tailors_kit" );
static const itype_id itype_technician_shirt_gray( "technician_shirt_gray" );
static const itype_id itype_test_baseball( "test_baseball" );
static const itype_id itype_test_baseball_half_degradation( "test_baseball_half_degradation" );
static const itype_id itype_test_baseball_x2_degradation( "test_baseball_x2_degradation" );
Expand Down Expand Up @@ -221,22 +226,28 @@ TEST_CASE( "Items_that_get_damaged_gain_degradation", "[item][degradation]" )
}
}

static void setup_repair( item &fix, player_activity &act, Character &u )
static item_location setup_tailorkit( Character &u )
{
map &m = get_map();
const tripoint_bub_ms pos = spawn_pos;
item &thread = m.add_item_or_charges( pos, item( itype_thread ) );
item &tailor = m.add_item_or_charges( pos, item( itype_tailors_kit ) );
thread.charges = 400;
tailor.reload( u, { map_cursor( pos ), &thread }, 400 );
REQUIRE( m.i_at( spawn_pos ).begin()->typeId() == tailor.typeId() );
return item_location( map_cursor( pos ), &tailor );
}

static void setup_repair( item &fix, player_activity &act, Character &u )
{
// Setup character
clear_character( u, true );
u.set_skill_level( skill_tailor, 10 );
u.wield( fix );
REQUIRE( u.get_wielded_item()->typeId() == fix.typeId() );

// Setup tool
item &thread = m.add_item_or_charges( spawn_pos, item( itype_thread ) );
item &tailor = m.add_item_or_charges( spawn_pos, item( itype_tailors_kit ) );
thread.charges = 400;
tailor.reload( u, { map_cursor( tripoint_bub_ms( spawn_pos ) ), &thread }, 400 );
REQUIRE( m.i_at( spawn_pos ).begin()->typeId() == tailor.typeId() );
item_location tailorloc = setup_tailorkit( u );

// Setup materials
item leather( itype_leather );
Expand All @@ -245,7 +256,6 @@ static void setup_repair( item &fix, player_activity &act, Character &u )

// Setup activity
item_location fixloc( u, &fix );
item_location tailorloc( map_cursor( tripoint_bub_ms( spawn_pos ) ), &tailor );
act.values.emplace_back( /* repeat_type::FULL */ 3 );
act.str_values.emplace_back( "repair_fabric" );
act.targets.emplace_back( tailorloc );
Expand Down Expand Up @@ -628,3 +638,67 @@ TEST_CASE( "Gun_repair_with_degradation", "[item][degradation]" )
}
}
}

static player_activity setup_repair_activity( item_location &tailorloc, item_location &fixloc )
{
player_activity act( ACT_REPAIR_ITEM );
act.values.emplace_back( /* repeat_type::FULL */ 3 );
act.str_values.emplace_back( "repair_fabric" );
act.targets.emplace_back( tailorloc );
act.targets.emplace_back( fixloc );
return act;
}

static item_location put_in_container( item_location &container, const itype_id &type )
{
ret_val<item *> inserted = container->get_contents().insert_item( item( type ),
pocket_type::CONTAINER );
REQUIRE( inserted.success() );
return item_location( container, inserted.value() );
}

// Reproduce previous segfault from https://github.com/CleverRaven/Cataclysm-DDA/issues/74254
TEST_CASE( "refit_item_inside_spillable_container", "[item][repair][container]" )
{
clear_avatar();
clear_map();
set_time_to_day();
REQUIRE( static_cast<int>( get_map().light_at( spawn_pos.raw() ) ) > 2 );

Character &u = get_player_character();
u.set_skill_level( skill_tailor, 10 );

// Setup starting equipment
item_location tailorkit = setup_tailorkit( u );
REQUIRE( u.wear_item( item( itype_backpack ) ) );
item_location backpack = u.top_items_loc().front();
item canvas_patch( itype_canvas_patch );
u.i_add_or_drop( canvas_patch, 100 );

// Starting inventory looks like:
// backpack >
// 100 canvas patch
// aluminum can >
// work t-shirt (poor fit)
// It's a bit odd that we can put the shirt into the aluminum can, since it
// would normally reject that with a message stating that it would spill.
GIVEN( "backpack > aluminum can > work t-shirt (poor fit)" ) {
item_location aluminum_can = put_in_container( backpack, itype_can_drink );
item_location fix_tshirt = put_in_container( aluminum_can, itype_technician_shirt_gray );
WHEN( "Refitting tshirt inside spillable container" ) {
REQUIRE_FALSE( fix_tshirt->has_flag( flag_FIT ) );
player_activity act = setup_repair_activity( tailorkit, fix_tshirt );
while( !act.is_null() ) {
::repair_item_finish( &act, &u, true );
}
THEN( "tshirt should be refitted successfully" ) {
const item *refitted_tshirt = backpack->get_item_with( [&]( const item & i ) {
return i.typeId() == itype_technician_shirt_gray;
} );
REQUIRE( refitted_tshirt != nullptr );
CHECK( refitted_tshirt->has_flag( flag_FIT ) );
}
}

}
}

0 comments on commit 7d59354

Please sign in to comment.