Segfault when mininuking a motorcycle

[ianestrachan]

I realize this is a somewhat obscure bug, but...

• Debug spawn a motorcycle
• Debug give yourself a mininuke
• Set the timer to 15, drop it into the motorcycle's box
• Run like hell
• Explosion will either cause segfault immediately, or there'll be another explosion which does

Successfully reproduced this several times in a row. Doesn't happen if the mininuke is dropped on the ground next to the motorcycle, or if a bigger vehicle (such as a car) is used.

[BevapDin]

processing active items is bugged

map::process_active_items_in_vehicles goes over the vehicles of a submap (why in reverse order?) and over all the cargo parts and over all the items in it. But one of these items (mininuke, other explosives) may destroy the the cargo part/the vehicle.
That means each of these three loops above might have to canceled/restarted.
To clarify this: the problem is removing the exploded mininuke from the cargo part (which does not exist anymore) and the access of the non-existing item-vector of that vehicle_part.

The same happens when you drop 1000 glass items and an active mininuke (in that order!) on a tile and wait for it to explode. The explosion destroys the glass items and thereby changes the items vector, the mininuke is now not the 1001. item in that vector but more like the 750. item.

[ianestrachan]

I believe it goes over the vehicles in reverse order in case a vehicle gets destroyed. It doesn't seem to properly handle parts being removed, though.

It's C++'s version of a ConcurrentModificationException, in that we're essentially modifying a list being iterated over and Bad Things™ happen as a result.

[kevingranade]

It's also possible that the problem is due to my recent fix that made explosions call vehicle->damage() as well as map->bash().

[VampyreLord]

I can't seem to reproduce this, I've dropped the mininuke inside the motorcycle box 2 in a row but no crash happened after the explosions.

But I've tried dropping 1000 glass items and activating a mininuke after setting the timer to 15 and a segfault did happen when it exploded.

[BevapDin]

Apparently it doesn't happen all the time. But I can still sometimes trigger it. The problematic code is still there.
It seems to be triggered more often when I use a single-square vehicle, like wheelbarrow.

[kevingranade]

I think this is a new version of the "explosion displaces the explosive" bug we keep getting. How about we pop active items off the map before evaluating them to protect them from side effects like this? It shouldn't hurt non-explosives, and explosives can just be discarded since they destroy themselves.
Btw, backtrace:
Program received signal SIGSEGV, Segmentation fault.
0x000000000041875f in __gnu_cxx::new_allocator::destroy (this=0x29ea970, __p=0x29e7440) at /usr/include/c++/4.8.2/ext/new_allocator.h:133
133 destroy(pointer __p) { __p->~_Tp(); }
(gdb) bt
#0 0x000000000041875f in __gnu_cxx::new_allocator::destroy (this=0x29ea970, __p=0x29e7440) at /usr/include/c++/4.8.2/ext/new_allocator.h:133
#1 0x000000000041735f in __gnu_cxx::__alloc_traitsstd::allocator::destroy (__a=..., __p=0x29e7440) at /usr/include/c++/4.8.2/ext/alloc_traits.h:219
#2 0x0000000000413a1c in std::vector<item, std::allocator >::erase (this=0x29ea970, __position=...) at /usr/include/c++/4.8.2/bits/vector.tcc:140
#3 0x0000000000871580 in vehicle::remove_item (this=0x29ea500, part=2, itemdex=0) at src/vehicle.cpp:3125
#4 0x00000000008080c1 in map::process_active_items_in_vehicles (this=0x7fc1b7db7158, nonant=61) at src/map.cpp:2571
#5 0x0000000000807b30 in map::process_active_items (this=0x7fc1b7db7158) at src/map.cpp:2516
#6 0x0000000000761f8c in game::do_turn (this=0x7fc1b7db7010) at src/game.cpp:985
#7 0x00000000004c552b in main (argc=0, argv=0x7fff8d3800a0) at src/main.cpp:147

I actually got a segfault out of vector.size() once >_<

[Dyrewulfe]

You know, I learned something today. You see, BevapDin's offhand parenthetical question was the answer to our dilemma. After researching the various properties of std::vector, I was informed that erasing an element from a vector invalidates any iterators that point to elements AFTER that element. So, we all should make our vector iterators run backwards, and they will get along better with each other.

PR #5792

(Edit: I'm not actually trying to say EVERY iterator on a vector should be reversed. I'm a little out of it from an extended caffeine and code binge, and it sounded funny when I wrote it.)

[ianestrachan]

@Graywolfe813 Yes, that's correct - reverse-order iterators are generally done for two reasons:

1. To avoid doing a vector.size() on every loop iteration (it's the tiniest of optimizations). You always compare the index with 0, instead.
2. When elements are going to be deleted from the vector, doing a reverse-order loop avoids skipping elements or tripping over invalidated pointers.

As such, the rule of thumb is to use a reverse-order iterator when potentially erasing data structure elements.

[Dyrewulfe]

I suppose I should have noted it here, too. That PR only fixes the crash with putting it INSIDE the box, as detailed in the initial issue report.

[kevingranade]

I thought I noted this here before. I think a good, permanent solution to this problem will be to pull the item currently being processed off the map, process it (including triggering an explosion, which is what causes problems), then re-insert it if it's still supposed to exist instead of removing it if it doesn't.

[KA101]

You did. If I had any idea how to accomplish that, I'd implement it. ;-/

Update: 15 glass items and an explosive (in that order) still segfaults.

[KA101]

One less blocker in the queue.