View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002601 | FSSCP | user interface | public | 2012-02-08 15:39 | 2012-02-15 16:10 |
Reporter | jon118 | Assigned To | CommanderDJ | ||
Priority | high | Severity | crash | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | Intel-based Mac | OS | OSX "Snow Leopard" | OS Version | 10.6.8 |
Product Version | 3.6.14 RC4 | ||||
Summary | 0002601: Creating / Removing pilots cause: ASSERTION: "(len < n)" at cfile.cpp:1269 len: 34, n: 32 | ||||
Description | Creating and removing pilots cause crash to desktop. This seems to have started after the related issue 0002593 was fixed, and I believe that fix is likely closely related to this crash. tested on trunk SVN 8436. | ||||
Steps To Reproduce | Create and remove pilots on the opening pilots screen. I've found that creating a pilot, removing it, then creating one again that has the same name as the one just removed seems to increase the likelihood of a crash. Though, simply creating and removing pilots *at all* causes a crash after 1-2 tries, so it's practically impossible to create a pilot without it crashing to desktop. You have about a 50/50 chance at best. | ||||
Tags | add, crash, create, pilot, pilots, remove | ||||
|
|
|
|
|
I've spent about 10 mins creating and deleting pilots in the initial pilot selection screen (i.e. not the barracks) without having any crashes. The error you got is making me think that there could be a filename length issue somewhere - could you let me know what the name of the pilot was that you were testing with? It would also help if you could attach the relevant pilot files - thanks! |
|
I started getting this issue on Windows as well. Traced it to trunk rev 8427 - where the fix for 0002593 was committed. It seems zero_player() doesn't actually initialise tmp_Player (at least, that was the impression I got from looking at it in MSVC) leading to garbage values being passed later on. I've attached a patch that fixes the issue for me - perhaps jon118 could test this as well? I've only zeroed out the two things that were causing issues, but if this does turn out to be the fix, then perhaps we need to go through and perform initialisation for everything else just in case. |
|
The patch fixes the assert for me |
|
Does player() have a constructor? That would be the proper fix here. |
|
I haven't been able to find one, which leads me to believe it's using the default constructor provided by C++. The other thing is that this bug only appears in trunk. So when the antipodes pilot code is merged in this bug will be resolved regardless. Do we go through the effort even though the "fix" is already forthcoming? (I'm happy to go through and do it anyway if it's deemed necessary. Or niffiwan can do it if he wants. :P) |
|
It's triggering in the 3.6.14 RC4, so it should be fixed for that release even if the antipodes pilot code will resolve it. Setting up the proper constructor still looks the best solution. Take a look out for any other initialisation type functions in this section of code, to provide hints on what the default values for the variables should be. Volition, and the SCP until recently, used init_*_struct() functions rather than constructors widely. It's an error prone coding style, as we've seen cases of new variables being added to structs without then changing the respective init function (or functions) elsewhere. |
|
3.7 (with new pilot code) is probably a while away so I think we should fix it for 3.6.14. We could just pull the relevant code from ant8 (i.e. player::reset), and backout the 0002593 "fix"? I should have time to look at this on Tuesday, if you want to complete it before then CommanderDJ, go ahead :-) |
|
Sure thing. :) I did what you suggested and pretty much copied the player::reset() function from Ant8 and made it the player constructor. I've replaced calls to zero_player with calls to the constructor - is this the correct approach? New patch is attached. EDIT 3: New patch uploaded. I kept zero_player() in there and just wrote the player constructor. |
|
Sorry, I've been away for a bit. Yes, that patch completely resolved the crashing issue for me. I really torture-tested pilot creation and removal using revision 8466. Without the patch it crashed consistently. With the patched code from Ant8 applied to player.h, I couldn't get it to crash for anything. Fix confirmed on my end. |
|
The patch looks good to me, my only feedback is that I don't see any initialisation for these two members of the player struct - I think they should be added. num_variables = 0; player_variables[MAX_SEXP_VARIABLES] = { 0 }; I'll try to apply the patch and give it some testing today as well (not that I'm sure I trust my testing completely given that I couldn't reproduce the bug before!) |
|
Good catch. I've added them in, changing player_variables' initialisation to use memset to maintain consistency with the rest of the function. Uploading the patch shortly, just gotta test it. EDIT: New patch up. |
|
mantis2601.patch (4,669 bytes)
Index: code/menuui/playermenu.cpp =================================================================== --- code/menuui/playermenu.cpp (revision 8465) +++ code/menuui/playermenu.cpp (working copy) @@ -205,7 +205,7 @@ /** * Sets Player values to 0/NULL/whatever appropriate default value */ -void zero_player () { +void zero_player() { player *tmp_Player = new player(); memcpy(Player,tmp_Player,sizeof(player)); free (tmp_Player); Index: code/playerman/player.h =================================================================== --- code/playerman/player.h (revision 8465) +++ code/playerman/player.h (working copy) @@ -82,6 +82,7 @@ } campaign_stats; typedef struct player { + char callsign[CALLSIGN_LEN + 1]; char short_callsign[CALLSIGN_LEN + 1]; // callsign truncated to SHORT_CALLSIGN_PIXEL_W pixels int short_callsign_width; // useful for mutliplayer chat boxes. @@ -171,7 +172,7 @@ int killer_objtype; // type of object that killed player int killer_species; // Species which killed player int killer_weapon_index; // weapon used to kill player (if applicable) - char killer_parent_name[NAME_LENGTH]; // name of parent object that killed the player + char killer_parent_name[NAME_LENGTH]; // name of parent object that killed the player int check_for_all_alone_msg; // timestamp to check for playing of 'all alone' msg @@ -202,6 +203,131 @@ control_info lua_ci; // copy of control info for scripting purposes (not to disturb real controls). button_info lua_bi; // copy of button info for scripting purposes (not to disturb real controls). button_info lua_bi_full; // gets all the button controls, not just the ones usually allowed + + //CommanderDJ - constructor to initialise everything to safe defaults. + //mostly copied from player::reset() in Antipodes 8 + player() + { + memset(callsign, 0, sizeof(callsign)); + memset(short_callsign, 0, sizeof(short_callsign)); + + short_callsign_width = 0; + memset(image_filename, 0, sizeof(image_filename)); + memset(squad_filename, 0, sizeof(squad_filename)); + memset(squad_name, 0, sizeof(squad_name)); + + memset(current_campaign, 0, sizeof(current_campaign)); + + main_hall = 0; + + readyroom_listing_mode = 0; + flags = 0; + save_flags = 0; + + memset(keyed_targets, 0, sizeof(keyed_targets)); + current_hotkey_set = -1; + + lead_target_pos = vmd_zero_vector; + lead_target_cheat = 0; + lead_indicator_active = 0; + + lock_indicator_x = 0; + lock_indicator_y = 0; + lock_indicator_start_x = 0; + lock_indicator_start_y = 0; + lock_indicator_visible = 0; + lock_time_to_target = 0.0f; + lock_dist_to_target = 0.0f; + + last_ship_flown_si_index = -1; + + objnum = -1; + + memset(&bi, 0, sizeof(button_info)); + memset(&ci, 0, sizeof(control_info)); + + init_scoring_element(&stats); + + friendly_hits = 0; + friendly_damage = 0.0f; + friendly_last_hit_time = 0; + last_warning_message_time = 0; + + control_mode = 0; + saved_viewer_mode = 0; + + check_warn_timestamp = -1; + + distance_warning_count = 0; + distance_warning_time = -1; + + allow_warn_timestamp = -1; + warn_count = 0; + + damage_this_burst = 0.0f; + + repair_sound_loop = -1; + cargo_scan_loop = -1; + + praise_count = 0; + allow_praise_timestamp = -1; + praise_delay_timestamp = -1; + + ask_help_count = 0; + allow_ask_help_timestamp = -1; + + scream_count = 0; + allow_scream_timestamp = -1; + + low_ammo_complaint_count = 0; + allow_ammo_timestamp = -1; + + subsys_in_view = -1; + request_repair_timestamp = -1; + + cargo_inspect_time = -1; + target_is_dying = -1; + current_target_sx = 0; + current_target_sy = 0; + target_in_lock_cone = 0; + locking_subsys = NULL; + locking_subsys_parent = -1; + locking_on_center = 0; + + killer_objtype = -1; + killer_species = 0; + killer_weapon_index = -1; + memset(killer_parent_name, 0, sizeof(killer_parent_name)); + + check_for_all_alone_msg = -1; + + update_dumbfire_time = -1; + update_lock_time = -1; + threat_flags = 0; + auto_advance = 1; + + multi_options_set_local_defaults(&m_local_options); + multi_options_set_netgame_defaults(&m_server_options); + + insignia_texture = -1; + + tips = 1; + + shield_penalty_stamp = 0; + + failures_this_session = 0; + show_skip_popup = 0; + + num_variables = 0; + memset(player_variables, 0, sizeof(player_variables)); + + death_message = ""; + + memset(&lua_ci, 0, sizeof(control_info)); + memset(&lua_bi, 0, sizeof(button_info)); + memset(&lua_bi_full, 0, sizeof(button_info)); + } + } player; extern player Players[MAX_PLAYERS]; |
|
Fix committed to trunk@8505. |
|
Fix committed to fs2_open_3_6_14@8511. |
fs2open: trunk r8505 2012-02-14 05:27 Ported: N/A Details Diff |
From CommanderDJ, fix for mantis 0002601: create constructor for player struct |
Affected Issues 0002601 |
|
mod - /trunk/fs2_open/code/playerman/player.h | Diff File | ||
mod - /trunk/fs2_open/code/menuui/playermenu.cpp | Diff File | ||
fs2open: fs2_open_3_6_14 r8511 2012-02-15 11:11 Ported: N/A Details Diff |
Backport: Trunk 8505; From CommanderDJ, fix for mantis 0002601: create constructor for player struct |
Affected Issues 0002601 |
|
mod - /branches/fs2_open_3_6_14/code/playerman/player.h | Diff File | ||
mod - /branches/fs2_open_3_6_14/code/menuui/playermenu.cpp | Diff File |
Date Modified | Username | Field | Change |
---|---|---|---|
2012-02-08 15:39 | jon118 | New Issue | |
2012-02-08 15:40 | jon118 | Tag Attached: crash | |
2012-02-08 15:40 | jon118 | Tag Attached: add | |
2012-02-08 15:40 | jon118 | Tag Attached: create | |
2012-02-08 15:40 | jon118 | Tag Attached: pilot | |
2012-02-08 15:40 | jon118 | Tag Attached: pilots | |
2012-02-08 15:40 | jon118 | Tag Attached: remove | |
2012-02-08 15:41 | jon118 | File Added: pilot-create-remove-crash-fs2_open.log.zip | |
2012-02-08 16:45 | jon118 | File Added: Apple-Crash-Reports.zip | |
2012-02-08 21:18 | niffiwan | Assigned To | => niffiwan |
2012-02-08 21:18 | niffiwan | Status | new => assigned |
2012-02-09 11:04 | niffiwan | Note Added: 0013269 | |
2012-02-11 12:53 | CommanderDJ | Note Added: 0013279 | |
2012-02-11 12:53 | CommanderDJ | File Added: mantis2601.patch | |
2012-02-11 12:54 | CommanderDJ | Note Edited: 0013279 | |
2012-02-11 13:07 | CommanderDJ | File Deleted: mantis2601.patch | |
2012-02-11 13:08 | CommanderDJ | File Added: mantis2601.patch | |
2012-02-11 13:17 | m_m | Note Added: 0013280 | |
2012-02-11 15:39 | Echelon9 | Note Added: 0013281 | |
2012-02-12 01:13 | CommanderDJ | Note Added: 0013283 | |
2012-02-12 01:26 | CommanderDJ | Note Edited: 0013283 | |
2012-02-12 02:55 | Echelon9 | Note Added: 0013284 | |
2012-02-12 02:55 | niffiwan | Note Added: 0013285 | |
2012-02-12 03:43 | CommanderDJ | Assigned To | niffiwan => CommanderDJ |
2012-02-12 03:50 | CommanderDJ | Note Added: 0013287 | |
2012-02-12 03:50 | CommanderDJ | File Deleted: mantis2601.patch | |
2012-02-12 03:50 | CommanderDJ | File Added: mantis2601.patch | |
2012-02-12 03:51 | CommanderDJ | Note Edited: 0013287 | |
2012-02-12 05:55 | CommanderDJ | File Deleted: mantis2601.patch | |
2012-02-12 05:55 | CommanderDJ | Note Edited: 0013287 | |
2012-02-12 14:33 | CommanderDJ | File Added: mantis2601.patch | |
2012-02-12 14:34 | CommanderDJ | Note Edited: 0013287 | |
2012-02-13 19:22 | jon118 | Note Added: 0013297 | |
2012-02-13 21:25 | niffiwan | Note Added: 0013303 | |
2012-02-13 22:04 | CommanderDJ | Note Added: 0013304 | |
2012-02-13 22:05 | CommanderDJ | File Deleted: mantis2601.patch | |
2012-02-13 22:05 | CommanderDJ | File Added: mantis2601.patch | |
2012-02-13 22:07 | CommanderDJ | Note Edited: 0013304 | |
2012-02-13 22:08 | CommanderDJ | Note Edited: 0013304 | |
2012-02-13 22:56 | CommanderDJ | Note Edited: 0013304 | |
2012-02-14 10:26 | niffiwan | Changeset attached | => fs2open trunk r8505 |
2012-02-14 10:26 | niffiwan | Note Added: 0013325 | |
2012-02-14 10:26 | niffiwan | Status | assigned => resolved |
2012-02-14 10:26 | niffiwan | Resolution | open => fixed |
2012-02-15 16:10 | chief1983 | Changeset attached | => fs2open fs2_open_3_6_14 r8511 |
2012-02-15 16:10 | chief1983 | Note Added: 0013330 |