Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segfault when sleeping with an alarm set #30522

Closed
DeeUnderscore opened this issue May 14, 2019 · 4 comments · Fixed by #30830
Closed

Segfault when sleeping with an alarm set #30522

DeeUnderscore opened this issue May 14, 2019 · 4 comments · Fixed by #30830
Labels
<Crash / Freeze> Fatal bug that results in hangs or crashes. (S2 - Confirmed) Bug that's been confirmed to exist

Comments

@DeeUnderscore
Copy link
Contributor

Describe the bug

Going to sleep and setting an alarm (via internal chronometer) can cause the game to segfault when waking up.

Steps To Reproduce

  1. Have internal chronometer? (not sure if reproducible without it)
  2. Press the sleep button
  3. Select yes at prompt
  4. Select 8 hours
  5. Wait

Expected behavior

For the game to not crash

Versions and configuration

  • OS: Ubuntu 19.04 x86_64
  • Game Version: 0.D-2935-g4da91581d1 (built with CLANG=1 TILES=1 RUNTESTS=0 LINTJSON=0 BACKTRACE=0 DEBUG_SYMBOLS=1)
  • Graphics Version: Tiles
  • Mods loaded:
    [
    Dark Days Ahead [dda],
    Disable NPC Needs [no_npc_food],
    Aftershock [aftershock],
    Bens GF recipes [Tolerate_This],
    Craftable Gun Pack [craftgp],
    DeadLeaves' Fictional Guns [FIC_Weapons],
    Garden Pots [growable-pots],
    Icecoon's Arsenal [ew_pack],
    Makeshift Items Mod [makeshift],
    Medieval and Historic Content [Medieval_Stuff],
    More Survival Tools [More_Survival_Tools],
    Mythological Replicas [nw_pack],
    DinoMod [DinoMod],
    Modular Turrets [modular_turrets],
    Salvaged Robots [Salvaged_Robots],
    Hydroponics [hydroponics],
    Mutant NPCs [mutant_npcs],
    Beta National Guard Camp [national_guard_camp],
    More Locations [more_locations],
    Urban Development [Urban_Development],
    Mapgen Demo [mapgen_demo],
    Fuji's More Buildings [FujiStruct],
    Boats [boats],
    Folding Parts pack [deoxymod],
    Vehicle Additions Pack [blazemod],
    Tanks and Other Vehicles [Tanks],
    Necromancy [necromancy],
    No Fungal Monsters [No_Fungi],
    Classes and Scenarios Mod [more_classes_scenarios],
    Classic Roguelike Classes [RL_Classes],
    Manual Bionic Installation [manualbionicinstall],
    No Filthy Clothing [no_filthy_clothing],
    Prevent Zombie Revivication [no_reviving_zombies],
    Safe Autodoc [safeautodoc],
    Simplified Nutrition [novitamins],
    Sleep Deprivation [sleepdeprivation],
    StatsThroughSkills [StatsThroughSkills]
    ]

Additional context

I've experienced this issue before independently, and also found it reproducible on the save provided in #30216. That issue was marked as solved, and the original reporter did not comment further, but it is still reproducible for me with the same save.

I've been able to reproduce it over several versions, compiling with both gcc and clang.

Save

Backtrace

From gdb

#0  0x00007f5e3aabdde1 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#1  0x00007f5e3aabecad in __gnu_debug::_Error_formatter::_M_error() const () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
#2  0x00000000007643ae in __gnu_debug::_Safe_iterator<std::__detail::_Node_iterator<std::pair<body_part const, effect>, false, false>, std::__debug::unordered_map<body_part, effect, std::hash<int>, std::equal_to<body_part>, std::allocator<std::pair<body_part const, effect> > > >::operator++ (
    this=0x7ffc21337d90) at /usr/bin/../lib/gcc/x86_64-linux-gnu/8/../../../../include/c++/8/debug/safe_iterator.h:295
#3  0x00000000011060b8 in player::process_effects (this=0x521d5f0) at src/player.cpp:5362
#4  0x000000000075dff6 in Creature::process_turn (this=0x521d5f0) at src/creature.cpp:138
#5  0x00000000010e5bb1 in player::process_turn (this=0x521d5f0) at src/player.cpp:764
#6  0x00000000008a42b7 in game::do_turn (this=0x3944f60) at src/game.cpp:1487
#7  0x0000000000bc13f2 in main (argc=<optimized out>, argv=<optimized out>) at src/main.cpp:689
@KorGgenT KorGgenT added the <Crash / Freeze> Fatal bug that results in hangs or crashes. label May 15, 2019
@Night-Pryanik Night-Pryanik added the (S1 - Need confirmation) Report waiting on confirmation of reproducibility label May 15, 2019
@neitsa
Copy link
Contributor

neitsa commented May 15, 2019

I was able to repro the crash.

 	Cataclysm-App-Debug-x64.exe!std::vector<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int>,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int> > >::size() Line 1180	C++
 	Cataclysm-App-Debug-x64.exe!std::vector<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int>,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int> > >::vector<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int>,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int> > >(const std::vector<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int>,std::allocator<std::pair<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,int> > > & _Right) Line 749	C++
 	Cataclysm-App-Debug-x64.exe!effect::get_miss_msgs() Line 1143	C++
>	Cataclysm-App-Debug-x64.exe!player::process_one_effect(effect & it, bool is_new) Line 5126	C++
 	Cataclysm-App-Debug-x64.exe!player::process_effects() Line 5365	C++
 	Cataclysm-App-Debug-x64.exe!Creature::process_turn() Line 141	C++
 	Cataclysm-App-Debug-x64.exe!Character::process_turn() Line 487	C++
 	Cataclysm-App-Debug-x64.exe!player::process_turn() Line 768	C++
 	Cataclysm-App-Debug-x64.exe!game::do_turn() Line 1484	C++
 	Cataclysm-App-Debug-x64.exe!SDL_main(int argc, char * * argv) Line 689	C++
 	Cataclysm-App-Debug-x64.exe!main_getcmdline(...) Line 177	C
 	[External Code]	

Calling process_one_effect:

Cataclysm-DDA/src/player.cpp

Lines 5362 to 5367 in b68b6ca

//Human only effects
for( auto &elem : *effects ) {
for( auto &_effect_it : elem.second ) {
process_one_effect( _effect_it.second, false );
}
}

And then get_miss_msgs():

Cataclysm-DDA/src/player.cpp

Lines 5125 to 5131 in b68b6ca

// Handle miss messages
auto msgs = it.get_miss_msgs();
if( !msgs.empty() ) {
for( const auto &i : msgs ) {
add_miss_reason( _( i.first ), static_cast<unsigned>( i.second ) );
}
}

On the auto msgs = it.get_miss_msgs(); line, it is already invalidated at this point:

>? it
{eff_type=0xdddddddddddddddd {id={_id={...} _cid=??? } max_intensity=??? max_effective_intensity=??? ...} ...}
    eff_type: 0xdddddddddddddddd {id={_id={...} _cid=??? } max_intensity=??? max_effective_intensity=??? ...}
    duration: {turns_=0xdddddddd }
    bp: 0xdddddddd
    permanent: true (0xdd)
    intensity: 0xdddddddd
    start_time: {turn_=0xdddddddd }

Crash here (bogus pointer for eff_type):

Cataclysm-DDA/src/effect.cpp

Lines 1141 to 1144 in b68b6ca

std::vector<std::pair<std::string, int>> effect::get_miss_msgs() const
{
return eff_type->miss_msgs;
}


Iterator is possibly invalidated during the hardcoded_effects( it ) line:

void player::process_one_effect( effect &it, bool is_new )
{
    bool reduced = resists_effect( it );
    double mod = 1;
    body_part bp = it.get_bp();
    int val = 0;

    hardcoded_effects( it ); // probably invalidated inside this function?

    // [ snip]

    // Handle miss messages
    auto msgs = it.get_miss_msgs();
    if( !msgs.empty() ) {
        for( const auto &i : msgs ) {
            add_miss_reason( _( i.first ), static_cast<unsigned>( i.second ) );
        }
    }

The hardcoded_effects() function is huge... 😭 Just with a quick glance at the crash, my first impression is that It's going to be hard to find where the problem lies exactly...

@Night-Pryanik Night-Pryanik added (S2 - Confirmed) Bug that's been confirmed to exist and removed (S1 - Need confirmation) Report waiting on confirmation of reproducibility labels May 15, 2019
@ejseto
Copy link
Contributor

ejseto commented May 15, 2019

process_effects() does a for loop over *effects, calling process_one_effect() for each effect. process_one_effect() calls hardcoded_effects(). When the effect is effect_sleep, hardcoded_effects() can call removes_effect() on effect_alarm_clock, e.g. you wake up before your alarm goes off. Then, process_effects() continues iterating over *effects, even though effect_alarm_clock has been removed. That's why it only happens when you set the alarm.

@neitsa
Copy link
Contributor

neitsa commented May 16, 2019

Yep you're absolutely right. I should have focused on the alarm_clock effect which is easy to follow once you know ... what you are searching for 😃.

In hardcoded_effects() the wake_up() function is called:

    } else if( id == effect_alarm_clock ) {
        if( in_sleep_state() ) {
            const bool asleep = has_effect( effect_sleep );
            if( has_bionic( bionic_id( "bio_watch" ) ) ) {
                if( dur == 1_turns ) {
                   // [snip]
                    if( !asleep ) {
                         // [snip]
                    } else if( ( !( has_trait( trait_id( "HEAVYSLEEPER" ) ) ||
                        // [snip]
                        bool slept_through = has_effect( effect_slept_through_alarm );
                        wake_up(); // *** here ***

wake_up() calls remove_effect( effect_alarm_clock ):

void player::wake_up()
{
    // [snip]

    remove_effect( effect_sleep );
    remove_effect( effect_slept_through_alarm );
    remove_effect( effect_lying_down );
    remove_effect( effect_alarm_clock ); // *** here ***

}

Which calls Creature::remove_effect which invalidate the iterator:

bool Creature::remove_effect( const efftype_id &eff_id, body_part bp )
{
    if( !has_effect( eff_id, bp ) ) {
        //Effect doesn't exist, so do nothing
        return false;
    }
    const effect_type &type = eff_id.obj();

    // [snip]

    // num_bp means remove all of a given effect id
    if( bp == num_bp ) {
        for( auto &it : ( *effects )[eff_id] ) {
            on_effect_int_change( eff_id, 0, it.first );
        }
        effects->erase( eff_id );   // *** iterator invalidation ***
    } else {
        ( *effects )[eff_id].erase( bp );
        on_effect_int_change( eff_id, 0, bp );
        if( ( *effects )[eff_id].empty() ) {
            effects->erase( eff_id );  // *** iterator invalidation ***
        }
    }
    return true;
}

No idea on how to fix this though... We would need a way to transmit the information about the iterator invalidation but also give the loop the correct iterator...

@ejseto
Copy link
Contributor

ejseto commented May 16, 2019

Is it possible to do it the way other effects are (scheduled to be) removed? By setting the duration to 0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Crash / Freeze> Fatal bug that results in hangs or crashes. (S2 - Confirmed) Bug that's been confirmed to exist
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants