View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0003128 | FSSCP | physics | public | 2014-10-27 00:14 | 2014-11-07 08:19 |
Reporter | Axem | Assigned To | m_m | ||
Priority | normal | Severity | crash | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Product Version | 3.7.2 RC4 | ||||
Target Version | 3.7.2 | ||||
Summary | 0003128: Docking related crash - dock_list isn't initialized | ||||
Description | There'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. | ||||
Tags | No tags attached. | ||||
|
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(). |
|
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 |
|
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. |
|
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?) |
|
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. |
|
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(); }; |
|
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 { |
|
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). |
|
That's a much better solution and it also removes unused code! If there are no objections I will commit it later. |
|
sounds good to me. |
|
Fix committed to trunk@11170. |
fs2open: trunk r11170 2014-11-07 04:02 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 |
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 |