From 4a878ffb8a2ce27b98df25a8ef18d75534652aac Mon Sep 17 00:00:00 2001 From: Paul Fenwick Date: Sun, 11 Aug 2019 10:08:47 -0700 Subject: [PATCH] Apply extra care with monster dragging code to avoid segfaults Based upon my interpretation of the stack-trace in #33104, it appears that `monster::store` is trying to use a dragged_foe that's not null, but is no longer valid, either. This may happen if the foe is dead, or any other condition that may cause the dragged critter object to be destroyed. This commit: - Always tracks the `dragged_foe_id` (an integer) rather than `dragged_foe` (a pointer) directly. - Uses `critter_by_id()` to check the validity of the `dragged_foe_id` on the occasions when it needed. This is the same function used by the savegame loading code to restore the dragged foe. - Moves the dragged_foe-finding functionality to its own method for clarity. - Ensures the dragged_foe-finding code clears relevant status effects and members if we're no longer dragging a valid critter. - Moves the nursebot operation code to its own method, also for clarity. Note that I don't have a reproducable way to induce the bug, so I don't have a great test for this. However it's hard to have a segfault if you don't have a stale pointer to deference in the first place. Fixes #33051. Fixes #33141. --- src/monattack.cpp | 2 +- src/monmove.cpp | 126 ++++++++++++++++++++++++++---------------- src/monster.h | 6 +- src/savegame_json.cpp | 6 +- 4 files changed, 85 insertions(+), 55 deletions(-) diff --git a/src/monattack.cpp b/src/monattack.cpp index 2090faf6e7879..faddf803f2f3a 100644 --- a/src/monattack.cpp +++ b/src/monattack.cpp @@ -2951,7 +2951,7 @@ bool mattack::nurse_operate( monster *z ) } else { grab( z ); if( target->has_effect( effect_grabbed ) ) { // check if we succesfully grabbed the target - z->dragged_foe = target; + z->dragged_foe_id = target->getID(); z->add_effect( effect_dragging, 1_turns, num_bp, true ); return true; } diff --git a/src/monmove.cpp b/src/monmove.cpp index 9cfd16a0f6c97..a8d1e7b750559 100644 --- a/src/monmove.cpp +++ b/src/monmove.cpp @@ -601,54 +601,11 @@ void monster::move() } } - // dragged_foe: restore pointer by saved id if required - if( dragged_foe_id >= 0 ) { - dragged_foe = g->critter_by_id( dragged_foe_id ); - dragged_foe_id = 0; - } - // defective nursebot surgery code - if( type->has_special_attack( "OPERATE" ) && has_effect( effect_dragging ) && - dragged_foe != nullptr ) { - - if( rl_dist( pos(), goal ) == 1 && g->m.furn( goal ) == furn_id( "f_autodoc_couch" ) && - !has_effect( effect_operating ) ) { - if( dragged_foe->has_effect( effect_grabbed ) && !has_effect( effect_countdown ) && - ( g->critter_at( goal ) == nullptr || g->critter_at( goal ) == dragged_foe ) ) { - add_msg( m_bad, _( "The %1$s slowly but firmly puts %2$s down onto the autodoc couch." ), name(), - dragged_foe->disp_name() ); - - dragged_foe->setpos( goal ); - - add_effect( effect_countdown, 2_turns );// there's still time to get away - add_msg( m_bad, _( "The %s produces a syringe full of some translucent liquid." ), name() ); - } else if( g->critter_at( goal ) != nullptr && has_effect( effect_dragging ) ) { - sounds::sound( pos(), 8, sounds::sound_t::speech, - string_format( - _( "a soft robotic voice say, \"Please step away from the autodoc, this patient needs immediate care.\"" ) ) ); - // TODO: Make it able to push NPC/player - push_to( goal, 4, 0 ); - } - } - if( get_effect_dur( effect_countdown ) == 1_turns && !has_effect( effect_operating ) ) { - if( dragged_foe->has_effect( effect_grabbed ) ) { - - bionic_collection collec = *dragged_foe->my_bionics; - int index = rng( 0, collec.size() - 1 ); - bionic target_cbm = collec[index]; - - //8 intelligence*4 + 8 first aid*4 + 3 computer *3 + 4 electronic*1 = 77 - float adjusted_skill = static_cast( 77 ) - std::min( static_cast( 40 ), - static_cast( 77 ) - static_cast( 77 ) / static_cast( 10.0 ) ); - - g->u.uninstall_bionic( target_cbm, *this, *dragged_foe, adjusted_skill ); + // Check if they're dragging a foe and find their hapless victim + player *dragged_foe = find_dragged_foe(); - dragged_foe->remove_effect( effect_grabbed ); - remove_effect( effect_dragging ); - dragged_foe = nullptr; - - } - } - } + // Give nursebots a chance to do surgery. + nursebot_operate( dragged_foe ); // The monster can sometimes hang in air due to last fall being blocked const bool can_fly = has_flag( MF_FLIES ); @@ -929,6 +886,81 @@ void monster::move() } } +player *monster::find_dragged_foe() +{ + // Make sure they're actually dragging someone. + if( dragged_foe_id < 0 || !has_effect( effect_dragging ) ) { + dragged_foe_id = -1; + return nullptr; + } + + // Dragged critters may die or otherwise become invalid, which is why we look + // them up each time. Luckily, monsters dragging critters is relatively rare, + // so this check should happen infrequently. + player *dragged_foe = g->critter_by_id( dragged_foe_id ); + + if( dragged_foe == nullptr ) { + // Target no longer valid. + dragged_foe_id = -1; + remove_effect( effect_dragging ); + } + + return dragged_foe; +} + +// Nursebot surgery code +void monster::nursebot_operate( player *dragged_foe ) +{ + // No dragged foe, nothing to do. + if( dragged_foe == nullptr ) { + return; + } + + // Nothing to do if they can't operate, or they don't think they're dragging. + if( !( type->has_special_attack( "OPERATE" ) && has_effect( effect_dragging ) ) ) { + return; + } + + if( rl_dist( pos(), goal ) == 1 && g->m.furn( goal ) == furn_id( "f_autodoc_couch" ) && + !has_effect( effect_operating ) ) { + if( dragged_foe->has_effect( effect_grabbed ) && !has_effect( effect_countdown ) && + ( g->critter_at( goal ) == nullptr || g->critter_at( goal ) == dragged_foe ) ) { + add_msg( m_bad, _( "The %1$s slowly but firmly puts %2$s down onto the autodoc couch." ), name(), + dragged_foe->disp_name() ); + + dragged_foe->setpos( goal ); + + add_effect( effect_countdown, 2_turns );// there's still time to get away + add_msg( m_bad, _( "The %s produces a syringe full of some translucent liquid." ), name() ); + } else if( g->critter_at( goal ) != nullptr && has_effect( effect_dragging ) ) { + sounds::sound( pos(), 8, sounds::sound_t::speech, + string_format( + _( "a soft robotic voice say, \"Please step away from the autodoc, this patient needs immediate care.\"" ) ) ); + // TODO: Make it able to push NPC/player + push_to( goal, 4, 0 ); + } + } + if( get_effect_dur( effect_countdown ) == 1_turns && !has_effect( effect_operating ) ) { + if( dragged_foe->has_effect( effect_grabbed ) ) { + + bionic_collection collec = *dragged_foe->my_bionics; + int index = rng( 0, collec.size() - 1 ); + bionic target_cbm = collec[index]; + + //8 intelligence*4 + 8 first aid*4 + 3 computer *3 + 4 electronic*1 = 77 + float adjusted_skill = static_cast( 77 ) - std::min( static_cast( 40 ), + static_cast( 77 ) - static_cast( 77 ) / static_cast( 10.0 ) ); + + g->u.uninstall_bionic( target_cbm, *this, *dragged_foe, adjusted_skill ); + + dragged_foe->remove_effect( effect_grabbed ); + remove_effect( effect_dragging ); + dragged_foe_id = -1; + + } + } +} + // footsteps will determine how loud a monster's normal movement is // and create a sound in the monsters location when they move void monster::footsteps( const tripoint &p ) diff --git a/src/monster.h b/src/monster.h index aded048161aff..2a1a6760bf546 100644 --- a/src/monster.h +++ b/src/monster.h @@ -419,8 +419,7 @@ class monster : public Creature tripoint wander_pos; // Wander destination - Just try to move in that direction int wandf; // Urge to wander - Increased by sound, decrements each move std::vector inv; // Inventory - player *dragged_foe; // player being dragged by the monster - int dragged_foe_id; // id of player being dragged by the monster (for save/load) + int dragged_foe_id = -1; // id of player being dragged by the monster cata::optional tied_item; // item used to tie the monster cata::optional battery_item; // item to power mechs // DEFINING VALUES @@ -525,6 +524,9 @@ class monster : public Creature std::bitset effect_cache; cata::optional summon_time_limit = cata::nullopt; + player *find_dragged_foe(); + void nursebot_operate( player *dragged_foe ); + protected: void store( JsonOut &jsout ) const; void load( JsonObject &jsin ); diff --git a/src/savegame_json.cpp b/src/savegame_json.cpp index ec2e46b68df26..295e6be9ff154 100644 --- a/src/savegame_json.cpp +++ b/src/savegame_json.cpp @@ -1811,7 +1811,6 @@ void monster::load( JsonObject &data ) data.read( "inv", inv ); data.read( "dragged_foe_id", dragged_foe_id ); - dragged_foe = nullptr; if( data.has_int( "ammo" ) && !type->starting_ammo.empty() ) { // Legacy loading for ammo. @@ -1889,10 +1888,7 @@ void monster::store( JsonOut &json ) const } json.member( "inv", inv ); - if( dragged_foe ) { - json.member( "dragged_foe_id", dragged_foe->getID() ); - } - + json.member( "dragged_foe_id", dragged_foe_id ); json.member( "path", path ); }