View Issue Details

IDProjectCategoryView StatusLast Update
0002601FSSCPuser interfacepublic2012-02-15 16:10
Reporterjon118 Assigned ToCommanderDJ  
PriorityhighSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
PlatformIntel-based MacOSOSX "Snow Leopard"OS Version10.6.8
Product Version3.6.14 RC4 
Summary0002601: 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.
Tagsadd, crash, create, pilot, pilots, remove

Activities

jon118

2012-02-08 15:41

reporter  

jon118

2012-02-08 16:45

reporter  

Apple-Crash-Reports.zip (21,363 bytes)

niffiwan

2012-02-09 11:04

developer   ~0013269

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!

CommanderDJ

2012-02-11 12:53

developer   ~0013279

Last edited: 2012-02-11 12:54

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.

m_m

2012-02-11 13:17

developer   ~0013280

The patch fixes the assert for me

Echelon9

2012-02-11 15:39

developer   ~0013281

Does player() have a constructor? That would be the proper fix here.

CommanderDJ

2012-02-12 01:13

developer   ~0013283

Last edited: 2012-02-12 01:26

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)

Echelon9

2012-02-12 02:55

developer   ~0013284

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.

niffiwan

2012-02-12 02:55

developer   ~0013285

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

CommanderDJ

2012-02-12 03:50

developer   ~0013287

Last edited: 2012-02-12 14:34

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.

jon118

2012-02-13 19:22

reporter   ~0013297

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.

niffiwan

2012-02-13 21:25

developer   ~0013303

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

CommanderDJ

2012-02-13 22:04

developer   ~0013304

Last edited: 2012-02-13 22:56

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.

CommanderDJ

2012-02-13 22:05

developer  

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];
mantis2601.patch (4,669 bytes)   

niffiwan

2012-02-14 10:26

developer   ~0013325

Fix committed to trunk@8505.

chief1983

2012-02-15 16:10

administrator   ~0013330

Fix committed to fs2_open_3_6_14@8511.

Related Changesets

fs2open: trunk r8505

2012-02-14 05:27

niffiwan


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

chief1983


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

Issue History

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