Fix crash going on tracks.

[shantonusen]

The attached area ride-track.zip and track item Climbing_stick_inc_leader.zip demonstrates a crash I've run into. If you move the leader to the base of the climbing stick, or throw a Pikmin to the base of the climbing stick, you'll get an Asan crash like the following respectively:

=================================================================
==9854==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000384798 at pc 0x00010058ace9 bp 0x7000095c0710 sp 0x7000095c0708
READ of size 8 at 0x602000384798 thread T3
    #0 0x10058ace8 in mob::tick_track_ride() mob.cpp:2704
    #1 0x100af8753 in leader_fsm::tick_track_ride(mob*, void*, void*) leader_fsm.cpp:2194
    #2 0x101ab0920 in mob_action_call::run(mob*, void*, void*) mob_script_action.cpp:214
    #3 0x1009a7952 in mob_event::run(mob*, void*, void*) mob_script.cpp:299
    #4 0x1009a7ccc in mob_fsm::run_event(unsigned long, void*, void*) mob_script.cpp:365
...
0x602000384798 is located 0 bytes to the right of 8-byte region [0x602000384790,0x602000384798)
allocated by thread T3 here:
...
    #7 0x100afb380 in std::__1::vector<unsigned long, std::__1::allocator<unsigned long> >::vector(std::__1::vector<unsigned long, std::__1::allocator<unsigned long> > const&) vector:1251
    #8 0x1017f069d in track_info_struct::track_info_struct(mob*, std::__1::vector<unsigned long, std::__1::allocator<unsigned long> >, float) mob_utils.cpp:1002
    #9 0x1017ec882 in track_info_struct::track_info_struct(mob*, std::__1::vector<unsigned long, std::__1::allocator<unsigned long> >, float) mob_utils.cpp:1005
    #10 0x100af7f5e in leader_fsm::start_riding_track(mob*, void*, void*) leader_fsm.cpp:2067
    #11 0x101ab0920 in mob_action_call::run(mob*, void*, void*) mob_script_action.cpp:214
    #12 0x1009a7952 in mob_event::run(mob*, void*, void*) mob_script.cpp:299

If I throw a Pikmin, it's basically the same:

=================================================================
==9890==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6020003cbb58 at pc 0x00010058ace9 bp 0x7000004f9710 sp 0x7000004f9708
READ of size 8 at 0x6020003cbb58 thread T3
    #0 0x10058ace8 in mob::tick_track_ride() mob.cpp:2704
    #1 0x1011403f7 in pikmin_fsm::tick_track_ride(mob*, void*, void*) pikmin_fsm.cpp:3585
    #2 0x101ab0920 in mob_action_call::run(mob*, void*, void*) mob_script_action.cpp:214
    #3 0x1009a7952 in mob_event::run(mob*, void*, void*) mob_script.cpp:299
    #4 0x1009a7ccc in mob_fsm::run_event(unsigned long, void*, void*) mob_script.cpp:365
    #5 0x100588f07 in mob::tick_script(float) mob.cpp:2670
...
0x6020003cbb58 is located 0 bytes to the right of 8-byte region [0x6020003cbb50,0x6020003cbb58)
allocated by thread T3 here:
...
    #7 0x100afb380 in std::__1::vector<unsigned long, std::__1::allocator<unsigned long> >::vector(std::__1::vector<unsigned long, std::__1::allocator<unsigned long> > const&) vector:1251
    #8 0x1017f069d in track_info_struct::track_info_struct(mob*, std::__1::vector<unsigned long, std::__1::allocator<unsigned long> >, float) mob_utils.cpp:1002
    #9 0x1017ec882 in track_info_struct::track_info_struct(mob*, std::__1::vector<unsigned long, std::__1::allocator<unsigned long> >, float) mob_utils.cpp:1005
    #10 0x10116611b in pikmin_fsm::start_riding_track(mob*, void*, void*) pikmin_fsm.cpp:3454
    #11 0x101ab0920 in mob_action_call::run(mob*, void*, void*) mob_script_action.cpp:214
    #12 0x1009a7952 in mob_event::run(mob*, void*, void*) mob_script.cpp:299
    #13 0x1009a7ccc in mob_fsm::run_event(unsigned long, void*, void*) mob_script.cpp:365

Both of these crashes happen in mob::tick_track_ride when a vector is indexed past its bounds

bool mob::tick_track_ride() {
    track_info->cur_cp_progress +=
        track_info->ride_speed * game.delta_t;

    if(track_info->cur_cp_progress >= 1.0f) {
        //Next checkpoint.
        track_info->cur_cp_nr++;
        track_info->cur_cp_progress -= 1.0f;

        if(
            track_info->cur_cp_nr ==
            track_info->checkpoints.size() - 1
        ) {
            stop_track_ride();
            return true;
        }
    }

    //Teleport to the right spot.
    hitbox* cur_cp =
        track_info->m->get_hitbox(
            track_info->checkpoints[track_info->cur_cp_nr]
        );
    hitbox* next_cp =
        track_info->m->get_hitbox(
            track_info->checkpoints[track_info->cur_cp_nr + 1]
        );

This code expects to access both start and end checkpoints, but the array only has one entry, so track_info->checkpoints[track_info->cur_cp_nr + 1] results in a read past the end of the vector.

I see an assertion that a track type has >=2 parts

void track_type::load_resources(data_node* file) {
    //We don't actually need to load any, but we know that if this function
    //is run, then the animations are definitely loaded.
    //Now's a good time to check if the track has 2+ checkpoints.
    if(anims.body_parts.size() < 2) {
        log_error(
            "The track type \"" + name + "\" needs to have at least 2 "
            "checkpoints (body parts), but it only has " +
            i2s(anims.body_parts.size()) + "!"
        );
    }
}

I believe the problem here is an off-by-one error when copying the track parts

    vector<size_t> checkpoints;
    for(size_t c = 0; c < tra_ptr->type->anims.body_parts.size() - 1; ++c) {
        checkpoints.push_back(c);
    }
    m->track_info =
        new track_info_struct(
        tra_ptr, checkpoints, tra_ptr->tra_type->ride_speed
    );

If this is copying the checkpoints from the track, it's not conventional to use < and also size() - 1. This would create a checkpoints vector of size 1 for the track, when the assertion during loading is that it's >=2 and the runtime code in mob::tick_track_ride() assumes it can access the next checkpoint. Changing the size() - 1 -> size()` fixes this for both leaders and Pikmin, and they now smoothly go up the track and hop off the end.

[Espyo]

I have no idea what that -1 was doing there. Using blame, 6a247cbe8b6bba817d664a948ca90a88f2d76772 is the culprit, where I changed the track travelling information from accepting a pointer to a track object, to instead accepting a list of checkpoints. There was no change in the way it iterates or makes use of the list of checkpoints/body parts, so I've got no clue as to why that weird -1 was added. Thanks for the correction!