View Issue Details

IDProjectCategoryView StatusLast Update
0003128FSSCPphysicspublic2014-11-07 08:19
ReporterAxem Assigned Tom_m  
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.7.2 RC4 
Target Version3.7.2 
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: http://pastebin.com/w2BUCaCR

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.

Activities

m_m

2014-10-27 07:25

developer   ~0016359

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().

m_m

2014-10-27 07:25

developer  

dockCrash.patch (1,730 bytes)   
Index: code/object/objcollide.cpp
===================================================================
--- code/object/objcollide.cpp	(revision 11159)
+++ code/object/objcollide.cpp	(working copy)
@@ -668,15 +668,16 @@
 //			is useful if a moving object wants to prevent a collision.
 int objects_will_collide(object *A, object *B, float duration, float radius_scale)
 {
-	object	A_future;
+	vec3d	prev_pos;
 	vec3d	hitpos;
+	int ret;
 
 
-	A_future = *A;
-	vm_vec_scale_add2(&A_future.pos, &A->phys_info.vel, duration);
+	prev_pos = A->pos;
+	vm_vec_scale_add2(&A->pos, &A->phys_info.vel, duration);
 
 	if (radius_scale == 0.0f) {
-		return ship_check_collision_fast(B, &A_future, &hitpos );
+		ret = ship_check_collision_fast(B, A, &hitpos);
 	} else {
 		float		size_A, size_B, dist, r;
 		vec3d	nearest_point;
@@ -686,18 +687,23 @@
 
 		//	If A is moving, check along vector.
 		if (A->phys_info.speed != 0.0f) {
-			r = find_nearest_point_on_line(&nearest_point, &A->pos, &A_future.pos, &B->pos);
+			r = find_nearest_point_on_line(&nearest_point, &prev_pos, &A->pos, &B->pos);
 			if (r < 0) {
+				nearest_point = prev_pos;
+			} else if (r > 1) {
 				nearest_point = A->pos;
-			} else if (r > 1) {
-				nearest_point = A_future.pos;
 			}
 			dist = vm_vec_dist_quick(&B->pos, &nearest_point);
-			return (dist < size_A + size_B);
+			ret = (dist < size_A + size_B);
 		} else {
-			return vm_vec_dist_quick(&B->pos, &A->pos) < size_A + size_B;
+			ret = vm_vec_dist_quick(&B->pos, &prev_pos) < size_A + size_B;
 		}
 	}
+
+	// Reset the position to the previous value
+	A->pos = prev_pos;
+
+	return ret;
 }
 
 //	Return true if the vector from *start_pos to *end_pos is within objp->radius*radius_scale of *objp
dockCrash.patch (1,730 bytes)   

m_m

2014-10-27 08:27

developer   ~0016360

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.

niffiwan

2014-11-03 08:52

developer   ~0016374

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?)

m_m

2014-11-05 11:49

developer   ~0016375

Last edited: 2014-11-05 11: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.

m_m

2014-11-05 11:49

developer  

objectCopy.patch (4,756 bytes)   
Index: code/object/objcollide.cpp
===================================================================
--- code/object/objcollide.cpp	(revision 11167)
+++ code/object/objcollide.cpp	(working copy)
@@ -668,15 +668,16 @@
 //			is useful if a moving object wants to prevent a collision.
 int objects_will_collide(object *A, object *B, float duration, float radius_scale)
 {
-	object	A_future;
+	vec3d	prev_pos;
 	vec3d	hitpos;
+	int ret;
 
 
-	A_future = *A;
-	vm_vec_scale_add2(&A_future.pos, &A->phys_info.vel, duration);
+	prev_pos = A->pos;
+	vm_vec_scale_add2(&A->pos, &A->phys_info.vel, duration);
 
 	if (radius_scale == 0.0f) {
-		return ship_check_collision_fast(B, &A_future, &hitpos );
+		ret = ship_check_collision_fast(B, A, &hitpos);
 	} else {
 		float		size_A, size_B, dist, r;
 		vec3d	nearest_point;
@@ -686,18 +687,23 @@
 
 		//	If A is moving, check along vector.
 		if (A->phys_info.speed != 0.0f) {
-			r = find_nearest_point_on_line(&nearest_point, &A->pos, &A_future.pos, &B->pos);
+			r = find_nearest_point_on_line(&nearest_point, &prev_pos, &A->pos, &B->pos);
 			if (r < 0) {
+				nearest_point = prev_pos;
+			} else if (r > 1) {
 				nearest_point = A->pos;
-			} else if (r > 1) {
-				nearest_point = A_future.pos;
 			}
 			dist = vm_vec_dist_quick(&B->pos, &nearest_point);
-			return (dist < size_A + size_B);
+			ret = (dist < size_A + size_B);
 		} else {
-			return vm_vec_dist_quick(&B->pos, &A->pos) < size_A + size_B;
+			ret = vm_vec_dist_quick(&B->pos, &prev_pos) < size_A + size_B;
 		}
 	}
+
+	// Reset the position to the previous value
+	A->pos = prev_pos;
+
+	return ret;
 }
 
 //	Return true if the vector from *start_pos to *end_pos is within objp->radius*radius_scale of *objp
Index: code/object/object.cpp
===================================================================
--- code/object/object.cpp	(revision 11167)
+++ code/object/object.cpp	(working copy)
@@ -107,6 +107,36 @@
 	{OF_COLLIDES,				"collides",					1,  },
 };
 
+static dock_instance* copy_dock_list(dock_instance* other)
+{
+	dock_instance* ret = NULL;
+	dock_instance* current = NULL;
+
+	while (other != NULL)
+	{
+		// create item
+		dock_instance* item = (dock_instance *)vm_malloc(sizeof(dock_instance));
+		item->dockpoint_used = other->dockpoint_used;
+		item->docked_objp = other->docked_objp;
+		item->next = NULL;
+
+		if (ret == NULL)
+		{
+			ret = item;
+		}
+		if (current != NULL)
+		{
+			current->next = item;
+		}
+
+		current = item;
+
+		other = other->next;
+	}
+
+	return ret;
+}
+
 // all we need to set are the pointers, but type, parent, and instance are useful to set as well
 object::object()
 	: next(NULL), prev(NULL), type(OBJ_NONE), parent(-1), instance(-1), n_quadrants(0), hull_strength(0.0),
@@ -115,6 +145,48 @@
 	memset(&(this->phys_info), 0, sizeof(physics_info));
 }
 
+object::object(const object& other)
+	: next(NULL), prev(NULL), type(other.type), parent(other.parent), instance(other.instance), n_quadrants(other.n_quadrants),
+	hull_strength(other.hull_strength), sim_hull_strength(other.sim_hull_strength), net_signature(other.net_signature),
+	num_pairs(other.num_pairs), dock_list(NULL), dead_dock_list(NULL), collision_group_id(other.collision_group_id),
+	phys_info(other.phys_info)
+{
+	dock_list = copy_dock_list(other.dock_list);
+	dead_dock_list = copy_dock_list(other.dead_dock_list);
+}
+
+object& object::operator=(const object& other)
+{
+	if (this != &other) // protect against invalid self-assignment
+	{
+		// Free the old dock lists
+		dock_free_dock_list(this);
+		dock_free_dead_dock_list(this);
+
+		next = NULL;
+		prev = NULL;
+		type = other.type;
+		parent = other.parent;
+		instance = other.instance;
+		n_quadrants = other.n_quadrants;
+		hull_strength = other.hull_strength;
+		sim_hull_strength = other.sim_hull_strength;
+		net_signature = other.net_signature;
+		num_pairs = other.num_pairs;
+		dock_list = NULL;
+		dead_dock_list = NULL;
+		collision_group_id = other.collision_group_id;
+		phys_info = other.phys_info;
+
+		// And allocate the new ones
+		dock_list = copy_dock_list(other.dock_list);
+		dead_dock_list = copy_dock_list(other.dead_dock_list);
+	}
+
+	// by convention, always return *this
+	return *this;
+}
+
 object::~object()
 {
 	objsnd_num.clear();
Index: code/object/object.h
===================================================================
--- code/object/object.h	(revision 11167)
+++ code/object/object.h	(working copy)
@@ -168,6 +168,10 @@
 	int				collision_group_id; // This is a bitfield. Collision checks will be skipped if A->collision_group_id & B->collision_group_id returns nonzero
 
 	object();
+	object(const object& other);
+	
+	object& operator= (const object & other);
+
 	~object();
 	void clear();
 };
objectCopy.patch (4,756 bytes)   

MageKing17

2014-11-07 02:47

developer  

3128.patch (6,771 bytes)   
Index: code/fred2/management.cpp
===================================================================
--- code/fred2/management.cpp	(revision 11169)
+++ code/fred2/management.cpp	(working copy)
@@ -1473,28 +1473,6 @@
 	return 0;
 }
 
-// What does this do?
-void add_ship_to_wing()
-{
-	int		org_object = cur_object_index;
-	vec3d	tvec;
-
-	set_cur_object_index();
-	if (Objects[org_object].type == OBJ_NONE) {
-		vm_vec_make(&tvec, 10.0f, 10.0f, 10.0f);
-		create_object(&tvec);
-	
-	} else {
-		Objects[cur_object_index] = Objects[org_object];
-		Objects[cur_object_index].pos.xyz.x += 3.0f;
-		Objects[cur_object_index].pos.xyz.y += 3.0f;
-		physics_init(&Objects[cur_object_index].phys_info);
-		Objects[cur_object_index].orient = Objects[org_object].orient;
-	}
-
-	set_modified();
-}
-
 //	Return true if current object is valid and is in a wing.
 //	Else return false.
 int query_object_in_wing(int obj)
Index: code/fred2/management.h
===================================================================
--- code/fred2/management.h	(revision 11169)
+++ code/fred2/management.h	(working copy)
@@ -90,7 +90,6 @@
 void	delete_reinforcement(int num);
 int	delete_ship_from_wing(int ship = cur_ship);
 int	find_free_wing();
-void	add_ship_to_wing();
 int	query_object_in_wing(int obj = cur_object_index);
 void	mark_object(int obj);
 void	unmark_object(int obj);
Index: code/fred2/wing.cpp
===================================================================
--- code/fred2/wing.cpp	(revision 11169)
+++ code/fred2/wing.cpp	(working copy)
@@ -512,108 +512,3 @@
 
 	return -1;
 }
-
-//	Create a wing.
-//	wing_type is the type of wing from the Wing_formations array to create.
-//	leader_index is the index in Objects of the leader object.  This object must
-//	have a position and an orientation.
-//	*wingmen is a list of indices of existing ships to be added to the wing.
-//	The wingmen list is terminated by -1.
-//	max_size is the maximum number of ships to add to the wing
-//	fill_flag is set if more ships are to be added to fill out the wing to max_size
-void create_wing(int wing_type, int leader_index, int *wingmen, int max_size, int fill_flag)
-{
-	int			num_placed, num_vectors, cur_vec_index;
-	object		*lobjp = &Objects[leader_index];
-	formation	*wingp;
-	object		*parent;
-	int			wing_list[MAX_OBJECTS];
-	matrix		rotmat;
-
-	initialize_wings();
-
-	Assert((wing_type >= 0) && (wing_type < MAX_WING_FORMATIONS));
-	Assert(Wing_formations[wing_type].num_vectors > 0);
-	Assert(Wing_formations[wing_type].num_vectors < MAX_WING_VECTORS);
-
-	Assert(Objects[leader_index].type != OBJ_NONE);
-	Assert(max_size < MAX_SHIPS_PER_WING);
-
-	num_placed = 0;
-	wingp = &Wing_formations[wing_type];
-	num_vectors = wingp->num_vectors;
-	cur_vec_index = 0;
-	parent = lobjp;
-	vm_copy_transpose_matrix(&rotmat, &lobjp->orient);
-
-	while (num_placed < max_size) {
-		vec3d	wvec;
-		int		curobj;
-
-		if (*wingmen == -1) {
-			if (!fill_flag)
-				break;
-			else {
-				curobj = get_free_objnum();
-				Assert(curobj != -1);
-				Objects[curobj].type = lobjp->type;
-				Assert(Wings[cur_wing].wave_count < MAX_SHIPS_PER_WING);
-// JEH		Wings[cur_wing].ship_list[Wings[cur_wing].count] = curobj;
-				Wings[cur_wing].wave_count++;
-			}
-		} else
-			curobj = *wingmen++;
-
-		Objects[curobj] = *lobjp;
-		vm_vec_rotate(&wvec, &wingp->offsets[cur_vec_index], &rotmat);
-		cur_vec_index = (cur_vec_index + 1) % num_vectors;
-		
-		if (num_placed < num_vectors)
-			parent = lobjp;
-		else
-			parent = &Objects[wing_list[num_placed - num_vectors]];
-		
-		wing_list[num_placed] = curobj;
-
-		vm_vec_add(&Objects[curobj].pos, &parent->pos, &wvec);
-
-		num_placed++;
-	}
-
-}
-
-//	Create a wing.
-//	cur_object_index becomes the leader.
-//	If count == -1, then all objects of wing cur_wing get added to the wing.
-//	If count == +n, then n objects are added to the wing.
-void test_form_wing(int count)
-{
-	int	i, wingmen[MAX_OBJECTS];
-	int	j, fill_flag;
-
-	j = 0;
-
-	Assert(cur_object_index != -1);
-	Assert(Objects[cur_object_index].type != OBJ_NONE);
-	Assert(get_wingnum(cur_object_index) != -1);
-	get_wingnum(cur_object_index);
-
-	wingmen[0] = -1;
-
-	for (i=1; i<MAX_OBJECTS; i++)
-		if ((get_wingnum(i) == cur_wing) && (Objects[i].type != OBJ_NONE))
-			if (i != cur_object_index)
-				wingmen[j++] = i;
-	
-	wingmen[j] = -1;
-
-	fill_flag = 0;
-
-	if (count > 0) {
-		fill_flag = 1;
-		j += count;
-	}
-
-	set_wingnum(cur_object_index, cur_wing);
-	create_wing(0, cur_object_index, wingmen, j, fill_flag);
-}
Index: code/object/objcollide.cpp
===================================================================
--- code/object/objcollide.cpp	(revision 11169)
+++ code/object/objcollide.cpp	(working copy)
@@ -668,15 +668,16 @@
 //			is useful if a moving object wants to prevent a collision.
 int objects_will_collide(object *A, object *B, float duration, float radius_scale)
 {
-	object	A_future;
+	vec3d	prev_pos;
 	vec3d	hitpos;
+	int ret;
 
 
-	A_future = *A;
-	vm_vec_scale_add2(&A_future.pos, &A->phys_info.vel, duration);
+	prev_pos = A->pos;
+	vm_vec_scale_add2(&A->pos, &A->phys_info.vel, duration);
 
 	if (radius_scale == 0.0f) {
-		return ship_check_collision_fast(B, &A_future, &hitpos );
+		ret = ship_check_collision_fast(B, A, &hitpos);
 	} else {
 		float		size_A, size_B, dist, r;
 		vec3d	nearest_point;
@@ -686,18 +687,23 @@
 
 		//	If A is moving, check along vector.
 		if (A->phys_info.speed != 0.0f) {
-			r = find_nearest_point_on_line(&nearest_point, &A->pos, &A_future.pos, &B->pos);
+			r = find_nearest_point_on_line(&nearest_point, &prev_pos, &A->pos, &B->pos);
 			if (r < 0) {
+				nearest_point = prev_pos;
+			} else if (r > 1) {
 				nearest_point = A->pos;
-			} else if (r > 1) {
-				nearest_point = A_future.pos;
 			}
 			dist = vm_vec_dist_quick(&B->pos, &nearest_point);
-			return (dist < size_A + size_B);
+			ret = (dist < size_A + size_B);
 		} else {
-			return vm_vec_dist_quick(&B->pos, &A->pos) < size_A + size_B;
+			ret = vm_vec_dist_quick(&B->pos, &prev_pos) < size_A + size_B;
 		}
 	}
+
+	// Reset the position to the previous value
+	A->pos = prev_pos;
+
+	return ret;
 }
 
 //	Return true if the vector from *start_pos to *end_pos is within objp->radius*radius_scale of *objp
Index: code/object/object.h
===================================================================
--- code/object/object.h	(revision 11169)
+++ code/object/object.h	(working copy)
@@ -170,6 +170,11 @@
 	object();
 	~object();
 	void clear();
+
+private:
+	// An object should never be copied; there are allocated pointers, and linked list shenanigans.
+	object(const object& other); // no implementation
+	object& operator= (const object & other); // no implementation
 };
 
 struct object_h {
3128.patch (6,771 bytes)   

MageKing17

2014-11-07 02:47

developer   ~0016377

Last edited: 2014-11-07 02: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).

m_m

2014-11-07 07:58

developer   ~0016378

That's a much better solution and it also removes unused code!
If there are no objections I will commit it later.

niffiwan

2014-11-07 08:12

developer   ~0016379

sounds good to me.

m_m

2014-11-07 08:19

developer   ~0016380

Fix committed to trunk@11170.

Related Changesets

fs2open: trunk r11170

2014-11-07 04:02

m_m


Ported: N/A

Details Diff
From MageKing17 and me: Fix for Mantis 3128: Docking related crash - dock_list isn't initialized
This patch makes the copy constructor and the assignment operator of the object class private as it contains members that should not be copied.
Affected Issues
0003128
mod - /trunk/fs2_open/code/object/objcollide.cpp Diff File
mod - /trunk/fs2_open/code/fred2/management.cpp Diff File
mod - /trunk/fs2_open/code/object/object.h Diff File
mod - /trunk/fs2_open/code/fred2/management.h Diff File
mod - /trunk/fs2_open/code/fred2/wing.cpp Diff File

Issue History

Date Modified Username Field Change
2014-10-27 00:14 Axem New Issue
2014-10-27 00:15 Zacam Assigned To => m_m
2014-10-27 00:15 Zacam Status new => assigned
2014-10-27 07:25 m_m Note Added: 0016359
2014-10-27 07:25 m_m Status assigned => code review
2014-10-27 07:25 m_m File Added: dockCrash.patch
2014-10-27 08:24 m_m File Added: objectCopy.patch
2014-10-27 08:27 m_m Note Added: 0016360
2014-11-03 08:52 niffiwan Note Added: 0016374
2014-11-05 11:49 m_m Note Added: 0016375
2014-11-05 11:49 m_m File Deleted: objectCopy.patch
2014-11-05 11:49 m_m File Added: objectCopy.patch
2014-11-05 11:50 m_m Note Edited: 0016375
2014-11-05 12:00 m_m Target Version => 3.7.2
2014-11-07 02:47 MageKing17 File Added: 3128.patch
2014-11-07 02:47 MageKing17 Note Added: 0016377
2014-11-07 02:47 MageKing17 Note Edited: 0016377
2014-11-07 07:58 m_m Note Added: 0016378
2014-11-07 08:12 niffiwan Note Added: 0016379
2014-11-07 08:19 m_m Changeset attached => fs2open trunk r11170
2014-11-07 08:19 m_m Note Added: 0016380
2014-11-07 08:19 m_m Status code review => resolved
2014-11-07 08:19 m_m Resolution open => fixed