Possible bug in commit: further accommodation of the dumb-rotation movement type, including, finally, proper collision detection!

[tomimaki]

Steps to reproduce crash:

• get campaign 'A Walk in the Sun' http://www.hard-light.net/forums/index.php?topic=91687.0
• play first mission until Typhon jumps in and launches wings of fighters

On linux I got this backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x000000000060b50b in vm_vec_sub (dest=0x7fffffffd610, src0=0x4, src1=0x10)
    at math/vecmat.cpp:170
170     dest->xyz.x = src0->xyz.x - src1->xyz.x;
(gdb) bt
#0  0x000000000060b50b in vm_vec_sub (dest=0x7fffffffd610, src0=0x4, src1=0x10)
    at math/vecmat.cpp:170
#1  0x00000000006df615 in get_submodel_delta_angle (sii=0x0)
    at model/modelread.cpp:632
#2  0x00000000006ee66d in model_get_rotating_submodel_list (
    submodel_vector=0x7fffffffd750, objp=0x1377b28 <Objects+570216>)
    at model/modelread.cpp:4427
#3  0x00000000007a496e in ship_ship_check_collision (
    ship_ship_hit_info=0x7fffffffd970, hitpos=0x7fffffffd940)
    at object/collideshipship.cpp:259
#4  0x00000000007a7f17 in collide_ship_ship (pair=0x7fffffffda70)
    at object/collideshipship.cpp:1132
#5  0x00000000007ad472 in obj_collide_pair (A=0x1376e38 <Objects+566904>,
    B=0x1377b28 <Objects+570216>) at object/objcollide.cpp:1653
#6  0x00000000007ac228 in obj_find_overlap_colliders (
    overlap_list_out=0x7fffffffdb50, list=0x7fffffffdb70, axis=2, collide=true)
    at object/objcollide.cpp:1270
#7  0x00000000007ac01f in obj_sort_and_collide () at object/objcollide.cpp:1239
#8  0x00000000007b466e in obj_move_all (frametime=0,0279998779)
    at object/object.cpp:1514
#9  0x0000000000417257 in game_simulation_frame ()
    at freespace2/freespace.cpp:4013
#10 0x0000000000417e1f in game_frame (paused=false)
---Type <return> to continue, or q <return> to quit---
    at freespace2/freespace.cpp:4405
#11 0x0000000000418af7 in game_do_frame () at freespace2/freespace.cpp:4816
#12 0x000000000041ae01 in game_do_state (state=2)
    at freespace2/freespace.cpp:6507
#13 0x00000000004e4b8c in gameseq_process_events ()
    at gamesequence/gamesequence.cpp:409
#14 0x000000000041bce1 in game_main (cmdline=0x1e2b0a0 "")
    at freespace2/freespace.cpp:7074
#15 0x000000000041bef3 in main (argc=1, argv=0x7fffffffdfa8)
    at freespace2/freespace.cpp:7209

And git bisect shows this:

975e108841e74f64ba43b1c7bbf8ba1109ec7ed2 is the first bad commit
commit 975e108841e74f64ba43b1c7bbf8ba1109ec7ed2
Author: Goober5000 <ipw47@yahoo.com>
Date:   Mon Jan 25 01:29:51 2016 -0500

    further accommodation of the dumb-rotation movement type, including, finally, proper collision detection!

[niffiwan]

I think I saw something related to this in a different mission, an address sanitiser build picked up memory corruption from a memset (IIRC), I'll have to see if I can repro & post the results.

[asarium]

I can reproduce this. Debugging shows that the submodel_instance_info pointer in modelread.cpp:4435 is nullptr. This could be a regression from the dumb rotate code where the submodel instance pointer isn't filled in correctly.

[MageKing17]

Collision detection happens in obj_move_all(). The sii attribute isn't filled in until model_update_instance() gets called, which doesn't happen until ship_model_update_instance() gets called, which doesn't happen until obj_move_all_post(). This isn't a problem for ships arriving from subspace because ships warping in don't do collision detection in the first arrival phase; arriving from a docking bay means it tries to collide the same frame it was created, so ship_model_update_instance() hasn't been called on it yet.

I see three basic solutions:

1. Disable collision detection for the first frame a ship arrives from a docking bay (silly, won't catch problems caused if a ship can be created in some other way and check collision that same frame).
2. Call ship_model_update_instance() earlier, e.g. by calling it immediately after the ship is created (I have no idea what the side effects of this could be).
3. Make model_get_rotating_submodel_list() check for sii being nullptr before calling get_submodel_delta_angle() (alternatively, make get_submodel_delta_angle() work on the angs and prev_angs attributes of the submodel_instance directly instead of the submodel_instance_info, since the former gets set from the latter anyway and will exist regardless of whether or not sii has been set).

Since the third option seems the least likely to have horrible side effects, I'm leaning towards that one... but since I'm not sure I understand this part of the code in the first place, I'd like to hear others' opinions before making a PR.

[niffiwan]

(FYI; I repro'd this with an ASAN build, and it didn't pick up any unusual memory activity prior to the segfault. Might have yet another bug lurking somewhere...)

[asarium]

@MageKing17: I tested your third option and it fixes the crash for me. Given that it can only happen in the first frame after a ship is created this shouldn't have any effects on other parts of the code. Should I submit the changes in a PR?

[MageKing17]

Sure; let's get this backported to the 3.7.4 branch ASAP so docking bays actually work in the next RC. :P

[asarium]

@tomimaki Could you check if the crash has been fixed by the recent commit to master?

[tomimaki]

Yes, it is fixed.

[asarium]

Great! I'll close this issue and create a PR for backporting this to 3.7.4.

[Goober5000]

This bug appears to be due to a combination of Volition's sequencing for rotating submodels and Swifty's conversion of the model rotation code to use instances instead of start/stop. I looked over the code in question and I agree that a simple nullptr check is the best solution.