cataclysmbnteam / Cataclysm-BN

Cataclysm: Bright Nights, A fork/variant of Cataclysm:DDA by CleverRaven.
https://docs.cataclysmbn.org
Other
696 stars 272 forks source link

Reproducible crash when activating grenades #1416

Closed Coolthulhu closed 2 years ago

Coolthulhu commented 2 years ago

Describe the bug

Activating multiple grenades in a row will crash the game.

Steps To Reproduce

  1. Spawn 10 grenades
  2. Activate them one by one
  3. Crash happens

Expected behavior

Crashes are obviously unacceptable.

Screenshots

Versions and configuration

Reproduces both on Windows releases and git-compiled versions.

Additional context

Relevant parts of crash log/stacktrace:

    ./cataclysm-tiles(_ZNK4item11stacks_withERKS_bb+0x27) [0x1020627]
    ./cataclysm-tiles(_ZN9inventory8add_itemE4itembbb+0x123) [0xfe9143]
    ./cataclysm-tiles(_ZN9inventory7restackER6player+0x344) [0xfea8b4]
    ./cataclysm-tiles() [0xf01e80]
    ./cataclysm-tiles(_ZN10game_menus3inv3useER6avatar+0x1a2) [0xf05d62]
    ./cataclysm-tiles(_ZN13avatar_action8use_itemER6avatarR13item_location+0x50) [0xabd800]
    ./cataclysm-tiles(_ZN13avatar_action8use_itemER6avatar+0x22) [0xabd6c2]
    ./cataclysm-tiles(_ZN4game13handle_actionEv+0x213e) [0xf4424e]
    ./cataclysm-tiles(_ZN4game7do_turnEv+0x978) [0xe79ab8]
    …/src/crash.cpp:110
    ??
    ??:0
    std::__uniq_ptr_impl<relic, std::default_delete<relic> >::_M_ptr() const
    /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:173
    std::unique_ptr<relic, std::default_delete<relic> >::get() const
    /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:422
    std::unique_ptr<relic, std::default_delete<relic> >::operator bool() const
    /usr/bin/../lib/gcc/x86_64-linux-gnu/11/../../../../include/c++/11/bits/unique_ptr.h:436
    item::is_relic() const
    …/src/item.cpp:6668
    item::stacks_with(item const&, bool, bool) const
    …/src/item.cpp:878
    inventory::add_item(item, bool, bool, bool)
    …/src/inventory.cpp:343
    inventory::restack(player&)
    …/src/inventory.cpp:437
    inv_internal(player&, inventory_selector_preset const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, int, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)
    …/src/game_inventory.cpp:186

My suspicion: In #1386, an item type cache was added, but from what I can tell, it assumes an item will keep its type forever. item::convert changes the type of an item, but doesn't change its location in the inventory, so it might try to do something on an empty list. For example, line inventory.cpp::429

other = items.erase( other, iter->front().typeId() );

If iter->front().typeId() doesn't point to a list containing other, the return value of erase will be return std::list<std::list<item> >::erase( stack_iter );. So other probably becomes this new iterator, which seems to be an iterator to a temporary element. There really, really should be at least a debugmsg for this case.

SaintCirno9 commented 2 years ago

I've noticed there are crashes related to #1386, but failed to reproduce that. Thanks for the help. I will fix it soon.