Cataclysm-dda: Crash in mapgen.cpp line 7070 // Out of bounds add_vehicle

[PartyAlias]

Describe the bug
Crash!

To Reproduce
Steps to reproduce the behavior (semi-random reproduces):

1. Load provided save
2. Go to sleep on where you are
3. May crash
4. I usually crash there once in game day while i'm managing my ne APC there and transfer stuff from old car.

Expected behavior
App should not crash.

Screenshots
In attaced link below.

Versions and configuration(please complete the following information):

• OS: Windows 10
• Game Version #8514 (10-Feb-2019 09:15:50)
• Graphics version Tiles
• Mods loaded: Arcana, PK Rebalance, other flavor ones

Additional context
Link to screen, crash.log & save game: https://drive.google.com/open?id=1btcpZkkYqY3ElR6XaR643g1bqA_INEXK

[anubiann00b]

I got this issue too, on Build 8550. Triggered while walking through a new area. Doesn't seem reproducible (and the save I have is a while before the crash).

debug.log:

src/mapgen.cpp:7070 [vehicle* map::add_vehicle(const vproto_id&, const tripoint&, int, int, int, bool)] Out of bounds add_vehicle t=none d=0 p=107,-2,0

crash.log:

CRASH LOG FILE: config/crash.log
VERSION: 0.C-37544-gc172ff6
TYPE: Signal
MESSAGE: SIGSEGV: Segmentation fault
STACK TRACE:
    @0x5D5955[cataclysm-tiles.exe+0x1D5955]
    @0x5D6502[cataclysm-tiles.exe+0x1D6502]
    SMPEG_error+0x4B034@0xE8CBD0[cataclysm-tiles.exe+0xA8CBD0]
    _C_specific_handler+0x98@0x7FFD55E27C58[msvcrt.dll+0x27C58]
    _chkstk+0x11D@0x7FFD5884F7DD[ntdll.dll+0x9F7DD]
    RtlWalkFrameChain+0x13F6@0x7FFD587BD856[ntdll.dll+0xD856]
    KiUserExceptionDispatcher+0x2E@0x7FFD5884E70E[ntdll.dll+0x9E70E]
    IMG_LoadWEBP_RW+0x3F1CE5@0x1390FB5[cataclysm-tiles.exe+0xF90FB5]
    @0xD72EF1[cataclysm-tiles.exe+0x972EF1]
    @0xD7327A[cataclysm-tiles.exe+0x97327A]
    @0xD73B5C[cataclysm-tiles.exe+0x973B5C]
    @0xD74297[cataclysm-tiles.exe+0x974297]
    @0xD7468D[cataclysm-tiles.exe+0x97468D]
    @0xD74C5B[cataclysm-tiles.exe+0x974C5B]
    @0x8C9C3C[cataclysm-tiles.exe+0x4C9C3C]
    @0x8D71C5[cataclysm-tiles.exe+0x4D71C5]
    @0xA190EB[cataclysm-tiles.exe+0x6190EB]
    @0xA1D594[cataclysm-tiles.exe+0x61D594]
    @0x6FF3F9[cataclysm-tiles.exe+0x2FF3F9]
    @0x7014D3[cataclysm-tiles.exe+0x3014D3]
    IMG_LoadWEBP_RW+0x4E88C9@0x1487B99[cataclysm-tiles.exe+0x1087B99]
    @0x4013ED[cataclysm-tiles.exe+0x13ED]
    @0x4014FB[cataclysm-tiles.exe+0x14FB]
    BaseThreadInitThunk+0x14@0x7FFD55F73034[KERNEL32.DLL+0x13034]
    RtlUserThreadStart+0x21@0x7FFD58823691[ntdll.dll+0x73691]

Mods: Boats, Folding Parts Pack, Vehicle Additions, More Locations, More Survival Tools, Makeshift Items

[WhitePaperWraper]

I had similar crashes twice, though it says about "line 6906" id imagine it's the same problem
First - crash shortly after starting new game in zoo
Second - crash when exploring new territory
(With the savegame which is probably useless)

• OS: [Debian 9.8]
• Game Version: 0.D
• Graphics version: [Tiles]
• Mods loaded: dda, no_npc_food,
  Tolerate_This, growable-pots, makeshift,
  More_Survival_Tools, modular_turrets,
  Salvaged_Robots, alt_map_key,
  mutant_npcs, more_locations,
  FujiStruct, mapgen_demo,
  Urban_Development,boats,
  deoxymod, blazemod, Tanks,
  No_Anthills, more_classes_scenarios,
  RL_Classes, safeautodoc,
  novitamins, StatsThroughSkills,
  DP_REMIX_INDICATORS,zets_hair_extensions,
  No_Fungi, no_reviving_zombies

[neitsa]

Thanks to @ymber, the save file provided in the post here allows for an easy repro.

TL;DR

The bug is not really hard to understand, especially with the given error message: a vehicle is split (in this save file, it's probably because Zs are smashing a vehicle) into multiple "pieces" (technically this creates a new vehicle), but it happens that one of those "pieces" is just out of the map...

Analysis

Full stack trace (up to generic_inbounds, not a time of crash):

>   Cataclysm.exe!generic_inbounds(const tripoint & p, const box & boundaries, const box & clearance) Line 379  C++
    Cataclysm.exe!map::inbounds(const tripoint & p) Line 7391   C++
    Cataclysm.exe!map::add_vehicle(const string_id<vehicle_prototype> & type, const tripoint & p, const int dir, const int veh_fuel, const int veh_status, const bool merge_wrecks) Line 7128   C++
    Cataclysm.exe!vehicle::split_vehicles(const std::vector<std::vector<int,std::allocator<int> >,std::allocator<std::vector<int,std::allocator<int> > > > & new_vehs, const std::vector<vehicle *,std::allocator<vehicle *> > & new_vehicles, const std::vector<std::vector<point,std::allocator<point> >,std::allocator<std::vector<point,std::allocator<point> > > > & new_mounts) Line 1920 C++
    Cataclysm.exe!vehicle::split_vehicles(const std::vector<std::vector<int,std::allocator<int> >,std::allocator<std::vector<int,std::allocator<int> > > > & new_vehs) Line 2031    C++
    Cataclysm.exe!vehicle::find_and_split_vehicles(int exclude) Line 1849   C++
    Cataclysm.exe!vehicle::break_off(int p, int dmg) Line 5107  C++
    Cataclysm.exe!vehicle::damage_direct(int p, int dmg, damage_type type) Line 5159    C++
    Cataclysm.exe!vehicle::damage(int p, int dmg, damage_type type, bool aimed) Line 4943   C++
    Cataclysm.exe!map::bash_vehicle(const tripoint & p, bash_params & params) Line 3396 C++
    Cataclysm.exe!map::bash(const tripoint & p, int str, bool silent, bool destroy, bool bash_floor, const vehicle * bashing_vehicle) Line 3352 C++
    Cataclysm.exe!monster::bash_at(const tripoint & p) Line 973 C++
    Cataclysm.exe!monster::move() Line 743  C++
    Cataclysm.exe!game::monmove() Line 4188 C++
    Cataclysm.exe!game::do_turn() Line 1504 C++
    Cataclysm.exe!SDL_main(int argc, char * * argv) Line 688    C++
    Cataclysm.exe!main_getcmdline(...) Line 177 C
    [External Code] 

So for each of the pieces of the old vehicle, the code tries to create a new vehicle, this happens here:

https://github.com/CleverRaven/Cataclysm-DDA/blob/e169df3755f3dc775d4c6518c87bafeee9aa53ca/src/vehicle.cpp#L1918-L1920

• new_v_pos3 is a tripoint giving the location of the new vehicle (from a vehicle part).
• g->m.add_vehicle() will try to create a new vehicle from this part.

Now for the beginning of the map::add_vehicle() function:

https://github.com/CleverRaven/Cataclysm-DDA/blob/e169df3755f3dc775d4c6518c87bafeee9aa53ca/src/mapgen.cpp#L7121-L7131

the map::inbounds() function verifies if the vehicle part location is inside the current map. For this it calls another helper function called generic_inbounds():

https://github.com/CleverRaven/Cataclysm-DDA/blob/e169df3755f3dc775d4c6518c87bafeee9aa53ca/src/enums.h#L374-L385

In our case, this function takes the following 3 parameters:

• The vehicle part location.
• The boundaries of the current map.
• A clearance (2 tripoints) which basically gives offsets inside the map, telling up to which point the p location is valid.

Here are some of the crashing elements:

>? p
{x=0x00000085 y=0x00000057 z=0x00000000 }
    x: 0x00000085
    y: 0x00000057
    z: 0x00000000

>? boundaries
{p_min={x=0x00000000 y=0x00000000 z=0xfffffff6 } p_max={x=0x00000084 y=0x00000084 z=0x0000000a } }
    p_min: {x=0x00000000 y=0x00000000 z=0xfffffff6 }
    p_max: {x=0x00000084 y=0x00000084 z=0x0000000a }

>? clearance
{p_min={x=0x00000000 y=0x00000000 z=0x00000000 } p_max={x=0x00000001 y=0x00000001 z=0x00000000 } }
    p_min: {x=0x00000000 y=0x00000000 z=0x00000000 }
    p_max: {x=0x00000001 y=0x00000001 z=0x00000000 }

Now if we check with the above elements, we can clearly see that the vehicle part is outside the map:

{
    return p.x >= boundaries.p_min.x + clearance.p_min.x &&
           p.x <= boundaries.p_max.x - clearance.p_max.x &&  // 0x85 <= 0x84 - 1 -> False!
           p.y >= boundaries.p_min.y + clearance.p_min.y &&
           p.y <= boundaries.p_max.y - clearance.p_max.y &&
           p.z >= boundaries.p_min.z + clearance.p_min.z &&
           p.z <= boundaries.p_max.z - clearance.p_max.z;
}

Crash

inbounds() returns false the debug message is displayed and a null vehicle is returned:

    if( !inbounds( p ) ) {
        debugmsg( "Out of bounds add_vehicle t=%s d=%d p=%d,%d,%d", type.c_str(), dir, p.x, p.y, p.z );
        return nullptr;
    }

There's no safeguard for a null vehicle:

            new_vehicle = g->m.add_vehicle( vproto_id( "none" ), new_v_pos3, face.dir() ); // returns null
            new_vehicle->name = name; // huho...
            new_vehicle->move = move;

Fix

Simplest fix possible (not necessarily the best one though...):

• Check for nullptr on add_vehicle() return
• if true then:
  
  
  
  • Return false from split_vehicles
  
  
  • Let find_and_split_vehicles() handle the return false, there's already a check for that:
  
  

https://github.com/CleverRaven/Cataclysm-DDA/blob/e55833534133562369d12338383576b1680d35e5/src/vehicle.cpp#L1849-L1856

Calling the vehicle specialist (sorry for the ping) @mlangsdorf :

• What's your view on patching this bug?
• It's technically possible to know that the vehicle is out of bound earlier than that, should we do the check earlier?
• Should we "register" (keep track of the vehicle parts) in the event the part(s) outside the map become visible?
  
  
  
  • seems pretty complicated to keep track of vehicle parts that are outside the map.