Cataclysm can segfault when interrupted butchering is resumed

[mark7]

Describe the bug
I looked into this a bit a while back, so I'm going from memory as to the nature of the issue and how Cataclysm deals with it. Sorry for the lack of specific details, but I just managed to obtain a problem-reproducing saved world and wanted to get the bug filed, as I understand that there is an upcoming release.

Cataclysm can segfault when butchering is interrupted, then resumed. The problem, as I recall, is that Cataclysm records the index of the corpse being butchered. However, this index is not guaranteed to remain valid from the time that the butchering is interrupted to the time that the butchering is resumed; other items can wind up in the grid square, potentially causing the index to refer to a non-corpse item. The invalid index can be saved with the character and the crash will persist across invocations of Cataclysm.

I tend to see this when butchering a corpse, being interrupted by a monster nearing, backing off the corpse, killing the monster on the same grid square, walking forward to try to resume butchering and having the game crash.

While I have not tested to confirm this, I suspect that, additionally, if an index is saved in the character's save file and there are multiple characters being played in the game world, that if one character is interrupted, has his game saved, another player is played in the world and mutates it, then it may similarly invalidate the index of the interrupted action.

Given the latter multiple-character situation, if resumable butchering is to be retained and done properly, it might be necessary to attach a UUID to each item (or at least each item referenced by a player's save file), something that would permit player references to items in the world to remain valid while the world mutates. Given that this would change the world save format, I'd imagine that it warrants developer discussion.

To Reproduce
I'm attaching an options.json (for autobutchering) and save directory for Cataclysm hashref a36162e that permits the crash to be easily induced -- simply move one step to the northeast and the game will crash. A stack trace from such a repro:

Thread 1 "cataclysm-tiles" received signal SIGSEGV, Segmentation fault.                                                                                                      
std::operator==<char> (__lhs=<error reading variable: Cannot access memory at address 0xb0>, __rhs="mon_null") at /usr/include/c++/8/bits/basic_string.h:6049                
6049        { return (__lhs.size() == __rhs.size()                                                                                                                           
(gdb) bt                                                                                                                                                                     
#0  std::operator==<char> (__lhs=<error reading variable: Cannot access memory at address 0xb0>, __rhs="mon_null") at /usr/include/c++/8/bits/basic_string.h:6049            
#1  0x000055555561e4d1 in string_id<mtype>::operator== (rhs=..., this=0xa8) at src/string_id.h:72                                                                            
#2  set_up_butchery (act=..., u=..., action=action@entry=BUTCHER) at src/activity_handlers.cpp:389                                                                           
#3  0x0000555555621db6 in activity_handlers::butcher_finish (act=0x55555a38ffd8, p=<optimized out>) at src/activity_handlers.cpp:1225                                        
#4  0x0000555555631521 in std::_Function_handler<void (player_activity*, player*), void (*)(player_activity*, player*)>::_M_invoke(std::_Any_data const&, player_activity*&&\
, player*&&) (__functor=..., __args#0=<optimized out>, __args#1=<optimized out>) at /usr/include/c++/8/bits/std_function.h:88                                                
#5  0x00005555556610b9 in std::function<void (player_activity*, player*)>::operator()(player_activity*, player*) const (this=<optimized out>, __args#0=<optimized out>, __ar\
gs#0@entry=0x55555a38ffd8, __args#1=<optimized out>, __args#1@entry=0x55555a38e990) at /usr/include/c++/8/bits/std_function.h:260                                            
#6  0x000055555565a8cf in activity_type::call_finish (this=<optimized out>, act=act@entry=0x55555a38ffd8, p=p@entry=0x55555a38e990) at /usr/include/c++/8/ext/aligned_buffer\
.h:74                                                                                                                                                                        
#7  0x0000555556413e9d in player_activity::do_turn (this=0x55555a38ffd8, p=...) at src/string_id.h:124                                                                       
#8  0x00005555559efc48 in game::process_activity (this=this@entry=0x555559ff3ab0) at src/game.cpp:1727                                                                       
#9  0x0000555555a1565e in game::do_turn (this=0x555559ff3ab0) at src/game.cpp:1554                                                                                           
#10 0x0000555555d87e14 in main (argc=<optimized out>, argv=<optimized out>) at src/main.cpp:648

Expected behavior
I'd expect butchering to resume without the game crashing.

Screenshots
N/A

Versions and configuration(please complete the following information):

• OS: Debian Buster
• Game Version: 0.C-33119-gbd4b908efd
• Graphics version: Tiles
• Mods loaded: Disable NPC Needs, Simplified Nutrition, Craftable Gun Pack, Safe Autodoc, Bright Nights, Deadleaves' Fictional Guns, Extended Realistic Guns, Icecoon's Arsenal, Makeshift Items Mod, Medieval and Historic Content, More Survival Tools, Mythological Replicas, Alternative Map Key, Mutant NPCs, NPC traits, More Locations, Fuji's More Buildings, Tall Buildings, Urban Development, Boats, Folding Parts pack, Vehicle Additions Pack, Tanks and Other Vehicles

Additional context
Add any other context about the problem here.
E.g. A link to a savegame that allows the issue to be reproduced.
save.tar.gz

options.json.gz

[fungusakafungus]

command sequence to reproduce the crash:
9'9BBB

[fungusakafungus]

I think this is the activity being resumed in the save:

      {
        "type": "ACT_BUTCHER",
        "moves_left": 0,
        "index": -1,
        "position": -2147483648,
        "coords": [],
        "name": "",
        "targets": [],
        "placement": [
          -2147483648,
          -2147483648,
          -2147483648
        ],
        "values": [
          3
        ],
        "str_values": [],
        "auto_resume": false
      }

[fungusakafungus]

...and what the heck is multibutchering?

[kevingranade]

It's an action to butcher everything on a tile.

[fungusakafungus]

I couldn't understand the entire process, but I've noticed the following when going through the code with gdb:
player.activity.values is set to [3] initially. When starting butchering, game::butcher appends a new value, 4, to the player.activity.values list. When finishing butchering, in activity_handlers.cpp multibutchering branch is entered, which I think shouldn't happen. Then it fails in the set_up_butchering because "paper wrapper" is not a corpse.

[OzoneH3]

The activity resume feature seems to be not finished at all.

activity.position is always -2147483648 (INT_MIN)
So no matter where you continue butchering it will try to
butcher the index given in "values".

Not sure if this is even reliably doable without unique item ids because
the items on the ground could get reordered and whatever was on
index: 3 is now who knows where.

[fungusakafungus]

We could also store data regarding unfinished activity on the corpse item itself with item->set_var.

[kevingranade]

Thanks for the great details everyone, I'm pretty sure you solved it and I'm just picking up the pieces.
Near the start of set_up_butchery(), it retrieves a mtype from item::corpse without validating that it's non-null, I'm simply having it check that and bail out of butchery if that item isn't actually a corpse.