Source Code Project Mantis - FSSCP
View Issue Details
0003128FSSCPphysicspublic2014-10-26 20:142014-11-07 03:19
Assigned Tom_m 
PlatformOSOS Version
Product Version3.7.2 RC4 
Target Version3.7.2Fixed in Version 
Summary0003128: Docking related crash - dock_list isn't initialized
DescriptionThere's some issue with docked ships and physics. I brought this up on IRC and m|m was looking into it. IRC log for reference:

tl;dr: Game is crashing at dock_find_max_speed_helper(), m|m says dock_list isn't initialized and something about the object class is not copyable.
TagsNo tags attached.
Attached Filespatch dockCrash.patch (1,730) 2014-10-27 03:25
patch objectCopy.patch (4,756) 2014-11-05 06:49
patch 3128.patch (6,771) 2014-11-06 21:47

2014-10-27 03:25   
As Axem mentioned, the object class contains allocated memory which is not properly copied when an object instance is copied as there is no copy constructor in the class.
That means that the new instance will have a pointer that is shared with the old instance. When the destructor of one of these instances is called it deallocates the memory for dock_list which also means that the pointer in the other instance points to deallocated memory.

The correct way to do this is to either disallow copying (which I did but that breaks FRED compilation) or to properly implement the copy constructor. I'll try implementing the copy constructor but as MageKing pointed out in IRC it is sufficient to only fix objects_will_collide() so it doesn't call the copy-constructor anymore.

That will fix this issue but there are still several occurrences in FRED code that copy an object.

I have attached a patch that fixes objects_will_collide().
2014-10-27 04:27   
I have uploaded another patch that adds a copy constructor to the object class.
Copying an object is still something that shouldn't be done as an object that is not in the Objects array will cause a lot of problems if used incorrectly but the new patch makes sure that it doesn't free memory used by something else.
2014-11-03 03:52   
I've had a look over the code and it looks good. Just a few comments:

1) I think nullptr is a c++11 construct, until we enable that I think we should continue to use the NULL macro
2) According to the rule-of-3 I think a copy-assignment operator would also be needed? :nervous: (or maybe just disable it if there's no instances of it?)
2014-11-05 06:49   
(Last edited: 2014-11-05 06:50)
Thanks for the review,
1) Forgot about that no c++11 rule, replaced nullptr with NULL
2) I think it's better to be on the safe side and have both.

I have uploaded a new patch with the necessary changes.

2014-11-06 21:47   
Upon closer inspection of the code that r11158 broke FRED compilation on, the functions in question don't actually seem to be called anywhere. Even if they were, the copy-assignment operator would probably screw something up somewhere because it sets prev and next to NULL, and all objects in the Objects[] array are in one of three linked lists: obj_free_list, obj_used_list, and obj_create_list.

I'm thinking the proper solution here is to disallow copying objects altogether, as r11158 did, and just remove the unused functions to fix FRED compilation. Attached patch is literally just r11158 with the offending code removed (well, and one comment changed).

2014-11-07 02:58   
That's a much better solution and it also removes unused code!
If there are no objections I will commit it later.
2014-11-07 03:12   
sounds good to me.
2014-11-07 03:19   
Fix committed to trunk@11170.

Issue History
2014-10-26 20:14AxemNew Issue
2014-10-26 20:15ZacamAssigned To => m_m
2014-10-26 20:15ZacamStatusnew => assigned
2014-10-27 03:25m_mNote Added: 0016359
2014-10-27 03:25m_mStatusassigned => code review
2014-10-27 03:25m_mFile Added: dockCrash.patch
2014-10-27 04:24m_mFile Added: objectCopy.patch
2014-10-27 04:27m_mNote Added: 0016360
2014-11-03 03:52niffiwanNote Added: 0016374
2014-11-05 06:49m_mNote Added: 0016375
2014-11-05 06:49m_mFile Deleted: objectCopy.patch
2014-11-05 06:49m_mFile Added: objectCopy.patch
2014-11-05 06:50m_mNote Edited: 0016375bug_revision_view_page.php?bugnote_id=16375#r941
2014-11-05 07:00m_mTarget Version => 3.7.2
2014-11-06 21:47MageKing17File Added: 3128.patch
2014-11-06 21:47MageKing17Note Added: 0016377
2014-11-06 21:47MageKing17Note Edited: 0016377bug_revision_view_page.php?bugnote_id=16377#r943
2014-11-07 02:58m_mNote Added: 0016378
2014-11-07 03:12niffiwanNote Added: 0016379
2014-11-07 03:19m_mChangeset attached => fs2open trunk r11170
2014-11-07 03:19m_mNote Added: 0016380
2014-11-07 03:19m_mStatuscode review => resolved
2014-11-07 03:19m_mResolutionopen => fixed