View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0001320 | FSSCP | gameplay | public | 2007-03-08 05:21 | 2010-12-07 17:19 |
| Reporter | cmb | Assigned To | |||
| Priority | low | Severity | crash | Reproducibility | always |
| Status | closed | Resolution | no change required | ||
| Platform | i686 (Athlon XP) | OS | Debian GNU/Linux | OS Version | unstable |
| Summary | 0001320: Unstable: Retail pilot file crash: too many subsys_aggregate_current_hits | ||||
| Description | Both debug and non-debug builds crashed trying to import my retail pilot file which has finished the main FS2 campaign. Looking at it in gdb, missionui/redalert.cpp:red_alert_read_wingman_status expects SUBSYSTEM_MAX (13) floats for each wingman's subsys_aggregate_current_hits. My file appears to have only 12, seemingly lacking SUBSYSTEM_UNKNOWN. The symptom is a crash slightly later in the file. My analysis, which could of course be rubbish: it appeared to be trying to read 65000 or so weapon techroom entries, having read 4 bytes too much from red alert bits for the final wingman and thus just read two techroom counts and the first few techroom values instead of the three techroom counts it was looking for. It had only survived up to that point because reading too much red alert data on the end of each non-final wingman just meant that only the name of the next wingman was affected. The version variable was 140; my previous retail install that wrote this file was 1.2, but I don't think I had flown a mission with that pilot since patching it from what I think was 1.0 or 1.01. My patch, which could also be rubbish, will follow. It just reads 12 subsystems rather than 13 if version is less than 142, which made everything work for me. | ||||
| Steps To Reproduce | Place attached pilot in data/players/single. Launch fs2_open_r (or _d). Choose this pilot. Answer yes to the upgrade check. Segfault. | ||||
| Tags | No tags attached. | ||||
|
2007-03-08 05:21
|
|
|
|
Also meant to say, I only found this project a few days ago, but I have to say, it's great so far. Thanks to all involved, let me know if there's anything else you need to know. :-) |
|
|
SUBSYSTEM_MAX is still 12 in the stable code branch, but apparently it got bumped in HEAD so that's something that we need to address as a bigger deal. The pilot file version is going to have to be bumped to maintain compatibility with current pilot files, or the code is going to have to be removed for the time being. Goober made the change, so let me check with him about it. If you want to submit a patch though, that would be great. Just set the version check for >242 instead, and bump CURRENT_PLAYER_FILE_VERSION in managepilot.cpp from 242 to 243. EDIT: Oh, and CAMPAIGN_FILE_VERSION also needs to be bumped. Also red_alert_read_wingman_status_campaign() will need to be changed so that it can take a version #, as well as check against it to handle this problem. |
|
2007-03-08 14:45
|
pilot-file-subsys.diff (1,255 bytes)
--- code/missionui/redalert.cpp.orig 2007-03-06 00:58:52.108008322 +0000
+++ code/missionui/redalert.cpp 2007-03-08 09:22:29.800096703 +0000
@@ -1078,6 +1078,8 @@
int i, j;
red_alert_ship_status *ras;
char tname[NAME_LENGTH];
+ int s = ( version < 243 ) ? 12
+ : SUBSYSTEM_MAX;
Red_alert_num_slots_used = cfread_int(fp);
@@ -1102,9 +1104,12 @@
ras->subsys_current_hits[j] = cfread_float(fp);
}
- for ( j = 0; j < SUBSYSTEM_MAX; j++ ) {
+ for ( j = 0; j < s; j++ ) {
ras->subsys_aggregate_current_hits[j] = cfread_float(fp);
}
+ for ( j = s; j < SUBSYSTEM_MAX; j++ ) {
+ ras->subsys_aggregate_current_hits[j] = 0.0f;
+ }
for ( j = 0; j < MAX_SHIP_WEAPONS; j++ ) {
if (version >= 142) {
--- code/playerman/managepilot.cpp.orig 2007-03-08 09:23:37.977453386 +0000
+++ code/playerman/managepilot.cpp 2007-03-08 09:24:04.822836652 +0000
@@ -365,7 +365,7 @@
// update this when altering data that is read/written to .PLR file
-#define CURRENT_PLAYER_FILE_VERSION 242
+#define CURRENT_PLAYER_FILE_VERSION 243
#define CURRENT_MULTI_PLAYER_FILE_VERSION 142
#define FS2_DEMO_PLAYER_FILE_VERSION 135
#define LOWEST_COMPATIBLE_PLAYER_FILE_VERSION 140 // compatible with release - Goober5000
|
|
|
Oops, my earlier patch didn't make it as I didn't notice that this page has *two* forms on it, not one. :-) Anyway, the patch you described is now attached. |
|
2007-03-25 05:17
|
1320.patch (6,061 bytes)
Index: code/mission/missioncampaign.cpp
===================================================================
RCS file: /home/fs2source/cvsroot/fs2_open/code/mission/missioncampaign.cpp,v
retrieving revision 2.51
diff -u -p -r2.51 missioncampaign.cpp
--- code/mission/missioncampaign.cpp 21 Mar 2007 21:06:54 -0000 2.51
+++ code/mission/missioncampaign.cpp 24 Mar 2007 23:18:48 -0000
@@ -477,7 +477,8 @@ campaign Campaign;
// bumped to 13 by Goober5000 for persistent variables
// bumped to 14 by taylor for ship/weapon table handling
// bumped to 15 by taylor to move stuff from pilot file
-#define CAMPAIGN_FILE_VERSION 15
+// bumped to 16 by cmb for SUBSYSTEM_MAX increment
+#define CAMPAIGN_FILE_VERSION 16
//#define CAMPAIGN_FILE_COMPATIBLE_VERSION CAMPAIGN_INITIAL_RELEASE_FILE_VERSION
#define CAMPAIGN_FILE_COMPATIBLE_VERSION 12 // 12 is the version of the original FS2
#define CAMPAIGN_FILE_ID 0xbeefcafe
@@ -1752,7 +1753,7 @@ int mission_campaign_savefile_load( char
pl->main_hall = cfread_ubyte(fp);
// red-alert wingman status
- red_alert_read_wingman_status_campaign(fp, s_name, w_name);
+ red_alert_read_wingman_status_campaign(fp, s_name, w_name, version);
// begin techroom data -------------------------------------------------
int intel_count;
Index: code/missionui/redalert.cpp
===================================================================
RCS file: /home/fs2source/cvsroot/fs2_open/code/missionui/redalert.cpp,v
retrieving revision 2.24
diff -u -p -r2.24 redalert.cpp
--- code/missionui/redalert.cpp 22 Mar 2007 20:47:33 -0000 2.24
+++ code/missionui/redalert.cpp 24 Mar 2007 23:18:48 -0000
@@ -1001,7 +1001,7 @@ void red_alert_bash_wingman_status()
}
// write wingman status out to the specified file
-void red_alert_write_wingman_status(CFILE *fp)
+void red_alert_write_wingman_status(CFILE *fp, ubyte is_multi)
{
int i, j;
red_alert_ship_status *ras;
@@ -1025,7 +1025,7 @@ void red_alert_write_wingman_status(CFIL
cfwrite_float(ras->subsys_current_hits[j], fp);
}
- for ( j = 0; j < SUBSYSTEM_MAX; j++ ) {
+ for ( j = 0; j < (is_multi ? 12 : SUBSYSTEM_MAX); j++ ) {
cfwrite_float(ras->subsys_aggregate_current_hits[j], fp);
}
@@ -1078,6 +1078,8 @@ void red_alert_read_wingman_status(CFILE
int i, j;
red_alert_ship_status *ras;
char tname[NAME_LENGTH];
+ int s = ( version < 243 ) ? 12
+ : SUBSYSTEM_MAX;
Red_alert_num_slots_used = cfread_int(fp);
@@ -1102,9 +1104,12 @@ void red_alert_read_wingman_status(CFILE
ras->subsys_current_hits[j] = cfread_float(fp);
}
- for ( j = 0; j < SUBSYSTEM_MAX; j++ ) {
+ for ( j = 0; j < s; j++ ) {
ras->subsys_aggregate_current_hits[j] = cfread_float(fp);
}
+ for ( j = s; j < SUBSYSTEM_MAX; j++ ) {
+ ras->subsys_aggregate_current_hits[j] = 0.0f;
+ }
for ( j = 0; j < MAX_SHIP_WEAPONS; j++ ) {
if (version >= 142) {
@@ -1119,11 +1124,13 @@ void red_alert_read_wingman_status(CFILE
}
// same basic thing as the above but for read from the campaign file - taylor
-void red_alert_read_wingman_status_campaign(CFILE *fp, char ships[][NAME_LENGTH], char weapons[][NAME_LENGTH])
+void red_alert_read_wingman_status_campaign(CFILE *fp, char ships[][NAME_LENGTH], char weapons[][NAME_LENGTH], int version)
{
int i, j;
red_alert_ship_status *ras;
int ras_tmp = -1;
+ int s = ( version < 16 ) ? 12
+ : SUBSYSTEM_MAX;
Red_alert_num_slots_used = cfread_int(fp);
@@ -1150,9 +1157,12 @@ void red_alert_read_wingman_status_campa
ras->subsys_current_hits[j] = cfread_float(fp);
}
- for ( j = 0; j < SUBSYSTEM_MAX; j++ ) {
+ for ( j = 0; j < s; j++ ) {
ras->subsys_aggregate_current_hits[j] = cfread_float(fp);
}
+ for ( j = s; j < SUBSYSTEM_MAX; j++ ) {
+ ras->subsys_aggregate_current_hits[j] = 0.0f;
+ }
for ( j = 0; j < MAX_SHIP_WEAPONS; j++ ) {
ras_tmp = cfread_int(fp) ;
Index: code/missionui/redalert.h
===================================================================
RCS file: /home/fs2source/cvsroot/fs2_open/code/missionui/redalert.h,v
retrieving revision 2.7
diff -u -p -r2.7 redalert.h
--- code/missionui/redalert.h 25 Jul 2005 05:24:16 -0000 2.7
+++ code/missionui/redalert.h 24 Mar 2007 23:18:48 -0000
@@ -83,6 +83,7 @@
#define __REDALERT_H__
#include "globalincs/globals.h"
+#include "globalincs/pstypes.h"
struct CFILE;
@@ -97,12 +98,12 @@ int red_alert_check_status();
void red_alert_store_wingman_status();
void red_alert_bash_wingman_status();
-void red_alert_write_wingman_status(CFILE *fp);
+void red_alert_write_wingman_status(CFILE *fp, ubyte is_multi = 0);
void red_alert_read_wingman_status(CFILE *fp, int version);
// campaign savefile versions
void red_alert_write_wingman_status_campaign(CFILE *fp);
-void red_alert_read_wingman_status_campaign(CFILE *fp, char ships[][NAME_LENGTH], char weapons[][NAME_LENGTH]);
+void red_alert_read_wingman_status_campaign(CFILE *fp, char ships[][NAME_LENGTH], char weapons[][NAME_LENGTH], int version);
void red_alert_voice_pause();
void red_alert_voice_unpause();
Index: code/playerman/managepilot.cpp
===================================================================
RCS file: /home/fs2source/cvsroot/fs2_open/code/playerman/managepilot.cpp,v
retrieving revision 2.28
diff -u -p -r2.28 managepilot.cpp
--- code/playerman/managepilot.cpp 11 Feb 2007 09:31:12 -0000 2.28
+++ code/playerman/managepilot.cpp 24 Mar 2007 23:18:49 -0000
@@ -365,7 +365,7 @@
// update this when altering data that is read/written to .PLR file
-#define CURRENT_PLAYER_FILE_VERSION 242
+#define CURRENT_PLAYER_FILE_VERSION 243
#define CURRENT_MULTI_PLAYER_FILE_VERSION 142
#define FS2_DEMO_PLAYER_FILE_VERSION 135
#define LOWEST_COMPATIBLE_PLAYER_FILE_VERSION 140 // compatible with release - Goober5000
@@ -1399,7 +1399,7 @@ int write_pilot_file_core(player *p)
}
if ( is_multi ) {
- red_alert_write_wingman_status(file);
+ red_alert_write_wingman_status(file, is_multi);
}
pilot_write_techroom_data(file, is_multi);
|
|
|
New patch (1320.patch): adds the campaign file bump in the note edit (hadn't seen that when I sent the earlier one); also fixes multiplayer pilots, which are still written at version 142, so should only write 12 subsystems. EDIT: Also, any pilot/campaign files already written with current CVS will need their version manually increasing to reflect the fact that they already have 13 subsystems, or this patch will crash on them. I couldn't see an easy way to detect such files. |
|
|
I've had the same kind of problem loading a save at the end of the Freespace2 retail campaign. I probably should have checked here first, but instead it turns out I followed almost exactly the same logic as cmb to find this bug. Here's what I first noticed: - Segfault at loading techroom data (which appeared to be caused by the garbage 65793 value in intel_count) - My debugger says ship_count = 113 and weapon_count = 89 in pilot_read_loadout(), but inside of pilot_read_techroom_data() ship_count = 89 and weapon_count = 10. intel_count = 10 seemed to make sense, so I suspect there was a 4-byte offset at some point while reading the file - I poked around in redalert.cpp for a while, noticing that every ship name after the first had gotten corrupted ("Bastion" showed up fine, but "Templar" appeared as "lar" -- confirms the 4-byte offset) - Eventually after randomly hacking away for a while, using "SUBSYSTEM_MAX - 1" in the loop inside red_alert_read_wingman_status() seemed to do the trick. I'm writing this here since unless I've screwed up exactly the same way, cmb's report is not "rubbish" at all... -sl |
|
|
Not going to work on this. |
|
|
This is a problem with code that's in fs2_open-unstable and not the main trunk right? |
|
|
Correct. Pilot files are so broken in the unstable code tree that it's just plain sad. Just like 0001227, this bug is more of a reminder that said code in unstable should be worked on before it ever gets merged into the trunk. |
|
|
Closing this issue, as it doesn't seem to exist in current builds. |
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2007-03-08 05:21 | cmb | New Issue | |
| 2007-03-08 05:21 | cmb | File Added: cmb.plr | |
| 2007-03-08 05:23 | cmb | Note Added: 0007823 | |
| 2007-03-08 06:46 | taylor | Note Added: 0007824 | |
| 2007-03-08 07:03 | taylor | Note Edited: 0007824 | |
| 2007-03-08 14:45 | cmb | File Added: pilot-file-subsys.diff | |
| 2007-03-08 14:45 | cmb | Note Added: 0007825 | |
| 2007-03-09 21:50 | taylor | Status | new => assigned |
| 2007-03-09 21:50 | taylor | Assigned To | => taylor |
| 2007-03-25 05:17 | cmb | File Added: 1320.patch | |
| 2007-03-25 05:18 | cmb | Note Added: 0007902 | |
| 2007-03-25 14:20 | cmb | Note Edited: 0007902 | |
| 2007-05-14 10:03 | shenlong | Note Added: 0008083 | |
| 2008-07-17 16:27 | taylor | Note Added: 0009458 | |
| 2008-07-17 16:27 | taylor | Assigned To | taylor => |
| 2008-07-17 16:27 | taylor | Status | assigned => confirmed |
| 2008-07-17 16:27 | taylor | Product Version | 3.6.10 => |
| 2008-10-09 04:58 | chief1983 | Note Added: 0009881 | |
| 2008-10-09 05:19 | taylor | Note Added: 0009884 | |
| 2008-10-09 05:33 | chief1983 | Priority | normal => low |
| 2008-10-09 05:33 | chief1983 | Summary | Retail pilot file crash: too many subsys_aggregate_current_hits => Unstable: Retail pilot file crash: too many subsys_aggregate_current_hits |
| 2010-12-07 17:19 | The_E | Note Added: 0012511 | |
| 2010-12-07 17:19 | The_E | Status | confirmed => closed |
| 2010-12-07 17:19 | The_E | Resolution | open => no change required |
