Source Code Project Mantis - FSSCP
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0002901FSSCPPilot datapublic2013-07-08 01:112013-08-14 09:50
Reporterniffiwan 
Assigned Toniffiwan 
PriorityhighSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version3.7.0 RC2 
Target Version3.7.0Fixed in Version3.7.0 
Summary0002901: pilot conversion failures
DescriptionAs reported here:
http://www.hard-light.net/forums/index.php?topic=84445.0
http://www.hard-light.net/forums/index.php?topic=83889.msg1700943#msg1700943

Multiple pilots provided fail to convert.
Each has a different error.
Occurs on both r9710 & r9584 (just after pilot code went to trunk)
Steps To ReproduceAdd relevant .pl2 & .cs2 pilotfiles to data/players/single/inferno
Start FSO Debug with no mods (or mediavps_3612) selected
Observe errors in log
Observe that pilots campaign progress is gone
Observe that pilots stats (kills/shots/etc) are gone
Additional Informationtype (d) - but custom mod in use (for both pilots)

PILOT: Beginning pilot file conversion...
  PL2 => Converting '/home/me/.fs2_open/data/players/single/inferno/[realnameremoved].pl2'...
  PLR => Conversion complete!
    CS2 => Converting '/home/me/.fs2_open/data/players/single/inferno/[realnameremoved].FreeSpace2.cs2'...
    CS2 => Import ERROR: Data mismatch (ship lookup)!

  PL2 => Converting '/home/me/.fs2_open/data/players/single/inferno/markoshark.pl2'...
  PLR => Conversion complete!
    CS2 => Converting '/home/me/.fs2_open/data/players/single/inferno/markoshark.FreeSpace2.cs2'...
    CS2 => Import ERROR: Data mismatch (weapon lookup)!

============================================
(Fixed / (a))

  PL2 => Converting '/home/me/.fs2_open/data/players/single/inferno/Sacro.pl2'...
  PLR => Conversion complete!
    CS2 => Converting '/home/me/.fs2_open/data/players/single/inferno/Sacro.FreeSpace2.cs2'...
    CS2 => Import ERROR: Data failure (RedAlert-ship)!
PILOT: Pilot file conversion complete!

This pilot is a good example of error b) & c)

============================================

status summary:

Also - the current patch in review only fixes one of potentially several pilot conversion bugs, i.e.

a) complete conversion fail due to error reading the mainhall

There's still these to investigate/fix:

b) loss of kills/hit% stats during conversion
c) loss of medals during conversion
d) complete conversion fail with ship/weapon reference error
TagsNo tags attached.
Relationships
related to 0002874resolved z64555 Pilot conversion not preserving axis mappings 
Attached Filespatch mantis2901-svn.patch (397) 2013-07-09 05:47
http://scp.indiegames.us/mantis/file_download.php?file_id=2239&type=bug
patch mantis2901b-svn.patch (1,489) 2013-08-05 04:00
http://scp.indiegames.us/mantis/file_download.php?file_id=2246&type=bug
patch mantis2901c-svn.patch (1,527) 2013-08-09 22:45
http://scp.indiegames.us/mantis/file_download.php?file_id=2247&type=bug
zip 2901d.zip (8,797) 2013-08-10 00:28
http://scp.indiegames.us/mantis/file_download.php?file_id=2248&type=bug

Notes
(0015173)
Echelon9   
2013-07-08 04:01   
Will take a look this week/weekend.
(0015174)
niffiwan   
2013-07-08 05:12   
If you want - but I'm already having a look as well :)
(0015175)
Echelon9   
2013-07-08 10:47   
(Last edited: 2013-07-08 11:06)
Sacro's is caused by this tripping when ras.name is read as a null from the file, thus causing (ras.ship_class >= csg->ship_list.size()) because ras.ship_class is reading garbage values

Ras.name is read as null because the count variable is ridiculously large, i.e. count = 167772160.

The actual corruption may be introduced even earlier, as the temp value in pilotfile_convert::csg_import() is corrupt before entering pilotfile_convert::csg_import_red_alert()

void pilotfile_convert::csg_import_red_alert()
{
    ...
    int ship_list_size = (int)csg->ship_list.size();
        ...

    for (idx = 0; idx < count; idx++) {
        red_alert_ship_status ras;
        ...
        ras.ship_class = cfread_int(cfp);

        if (ras.ship_class >= ship_list_size) {
            throw "Data failure (RedAlert-ship)!";
        }
        ...
        }
}
(0015178)
niffiwan   
2013-07-08 16:57   
(Last edited: 2013-07-08 17:15)
Yeah, I suspect that there's something in the 3.6.18 .cs2 not being read correctly by something before this section:

        cfread_string(temp, NAME_LENGTH, cfp);
        csg->main_hall = temp;

        csg_import_red_alert();

and that leaves the cfp file offset in the "wrong place" == bad data reads into temp, count, etc.

I recall that some commits removed various inferno related code, I wonder if the inferno var now isn't being set correctly and thus this .cs2 is being treated like a non-inferno pilot.

Hmmm... well that inferno code wasn't removed, but this looks slightly odd & needs a closer look - is Get_file_list_child reset by cf_get_file_list?

void convert_pilot_files()
        ...
        // get list of old pilots which may need converting, starting with inferno pilots
        Get_file_list_child = "inferno";
        cf_get_file_list(old_files, CF_TYPE_SINGLE_PLAYERS, "*.pl2");
        inf_count = old_files.size();
        cf_get_file_list(old_files, CF_TYPE_SINGLE_PLAYERS, "*.pl2");


Yes, it is reset to NULL. Hmmm...
(0015180)
niffiwan   
2013-07-09 05:37   
(Last edited: 2013-07-09 05:44)
OK - I think this commit is the problem: r8453

It changes the behaviour of csg_import to treat the mainhall as a string, not as the previous ubyte. Given that the import needs to read data created by 3.6.18 & prior (i.e. before modular mainhalls) I think this small section needs to be reverted. I'll test that now.

    csg->main_hall = cfread_ubyte(cfp);
    //cfread_string(temp, NAME_LENGTH, cfp);
    //csg->main_hall = temp;

And yep - that looks like it :D
(0015181)
niffiwan   
2013-07-09 05:48   
Patch for one issue is ready - the pilot conversion process needs to use the old way of reading the mainhall ubyte, not the new string.
(0015182)
niffiwan   
2013-07-09 21:38   
Too hasty...

1) need to remove a now unused variable (temp)

2) need to check that the csg->main_hall will handle being assigned a ubyte without side-effects. Worked in my test with Sacro's pilot, but I need to double check to be sure.

Also added CommanderDJ to the monitor list to get his opinion on 2) given that he implemented modular mainhalls
(0015187)
CommanderDJ   
2013-07-14 01:38   
Apologies for my tardiness. At a glance this looks okay to me, but I'll take a proper look at this in the the next day or two.
(0015190)
CommanderDJ   
2013-07-17 02:37   
I've had a proper look at this, and I think this should be okay!
(0015194)
Goober5000   
2013-07-18 18:45   
Well, now, wait a minute. In order for both the old main hall code and the new modular main hall code to work, the code has to be able to handle reading both ubytes and strings from that field. N'est-ce pas?
(0015195)
niffiwan   
2013-07-18 19:10   
Well, I'm pretty sure it doesn't.

The old main hall code is <= 3.6.18 only (ubyte / old pilotfile format).
The new mainhall code is >= 3.6.19 only (string / new pilotfile format).

The pilot conversion is the only code that needs to read a ubyte & output a string. The normal pilot reading code in both <= 3.6.18 & >= 3.6.19 just need to deal with their respective / independent formats.

Have I missed something here?
(0015196)
CommanderDJ   
2013-07-18 21:15   
This was my thinking as well. Assuming the conversion only happens once per pilot and that it only happens on old pilots (someone correct me if these assumptions do not hold), the mainhall field will always be a ubyte during the conversion process.
(0015197)
Goober5000   
2013-07-18 21:37   
Oh, so the conversion code is distinct from the regular pilot code? I thought they overlapped. Separating them does make more sense.
(0015198)
niffiwan   
2013-07-19 23:06   
Fix committed to trunk@9730.
(0015199)
niffiwan   
2013-07-19 23:07   
Hmmm.. I was trying to avoid the auto-close hook :)
(0015207)
niffiwan   
2013-08-05 04:13   
mantis2901b-svn.patch should fix the medals/stats conversion issue. I've successfully tested with three pilots, Sacro + two of mine. Further testing & review would be great.
(0015215)
z64555   
2013-08-09 12:40   
(Last edited: 2013-08-09 12:41)
With the patches in place, the conversion completes for my pilot as well. I'm getting some parsing errors/warnings later on, though. I was just messing about in the mainhall and the barracks to check the stats.

MVE: Buffer underun (First is normal)
Got event GS_EVENT_GAME_INIT (49) in state NOT A VALID STATE (0)
PLR => Unable to open 'z64555S.plr' for reading!
PLR => Verifying 'Default.plr' with version 1...
PLR => Parsing: Flags..
PLR => (0x0001) Attempted to read 4-byte(s) beyond length limit

CSG => Loading 'z64555.FreeSpace2.csg' with version 2...
CSG => Parsing: Flags...
CSG => Warning: (0x0001) Short read, information may have been lost!
(0015218)
niffiwan   
2013-08-09 22:50   
(Last edited: 2013-08-09 22:51)
OK - patch to remove those warnings is attached.

a) mainhall was written to CSG during conversion, but is not written to CSG for normal save/load (which seems a bit odd, but it seems to work OK regardless)

b) cutscenes were written to CSG flags section AND cutscenes section during conversion, but only the cutscenes section for normal save/load

c) the is_multi flag was written for normal PLR save/load but not for the conversion (flag is now used from the previous pilot format)
(0015219)
z64555   
2013-08-10 00:27   
(Last edited: 2013-08-10 08:11)
Ok, those fixes worked for me, no longer getting the errors/warnings.

Weakly related, a corrupted .cs2 file creates a nonsensical error message. I've uploaded the files in question as well as the fs2_log. Not sure how much you'll be able to garn from it, though.

Um... I labeled the .zip as 2901d, but it's not the same "d" mentioned in the original ticket. <.<
(0015222)
niffiwan   
2013-08-11 04:22   
OK - I've had a look at that borked error message and it is not related to 2901, it's related to r9732 & r9739. I'll create a separate mantis for that issue and commit & resolve this one.
(0015225)
niffiwan   
2013-08-11 04:24   
see r9745 & r9746

Issue History
Date ModifiedUsernameFieldChange
2013-07-08 01:11niffiwanNew Issue
2013-07-08 01:11niffiwanStatusnew => assigned
2013-07-08 01:11niffiwanAssigned To => niffiwan
2013-07-08 04:01Echelon9Note Added: 0015173
2013-07-08 05:12niffiwanNote Added: 0015174
2013-07-08 10:47Echelon9Note Added: 0015175
2013-07-08 10:57Echelon9Note Edited: 0015175bug_revision_view_page.php?bugnote_id=15175#r576
2013-07-08 11:00Echelon9Note Edited: 0015175bug_revision_view_page.php?bugnote_id=15175#r577
2013-07-08 11:06Echelon9Note Edited: 0015175bug_revision_view_page.php?bugnote_id=15175#r578
2013-07-08 16:57niffiwanNote Added: 0015178
2013-07-08 17:01niffiwanNote Edited: 0015178bug_revision_view_page.php?bugnote_id=15178#r580
2013-07-08 17:09niffiwanNote Edited: 0015178bug_revision_view_page.php?bugnote_id=15178#r581
2013-07-08 17:15niffiwanNote Edited: 0015178bug_revision_view_page.php?bugnote_id=15178#r582
2013-07-09 05:37niffiwanNote Added: 0015180
2013-07-09 05:44niffiwanNote Edited: 0015180bug_revision_view_page.php?bugnote_id=15180#r584
2013-07-09 05:47niffiwanFile Added: mantis2901-svn.patch
2013-07-09 05:48niffiwanNote Added: 0015181
2013-07-09 05:48niffiwanStatusassigned => code review
2013-07-09 21:38niffiwanNote Added: 0015182
2013-07-14 01:38CommanderDJNote Added: 0015187
2013-07-17 02:37CommanderDJNote Added: 0015190
2013-07-18 18:45Goober5000Note Added: 0015194
2013-07-18 19:10niffiwanNote Added: 0015195
2013-07-18 21:15CommanderDJNote Added: 0015196
2013-07-18 21:37Goober5000Note Added: 0015197
2013-07-19 22:57niffiwanAdditional Information Updatedbug_revision_view_page.php?rev_id=586#r586
2013-07-19 22:58niffiwanRelationship addedrelated to 0002874
2013-07-19 23:06niffiwanChangeset attached => fs2open trunk r9730
2013-07-19 23:06niffiwanNote Added: 0015198
2013-07-19 23:06niffiwanStatuscode review => resolved
2013-07-19 23:06niffiwanResolutionopen => fixed
2013-07-19 23:07niffiwanNote Added: 0015199
2013-07-19 23:07niffiwanStatusresolved => feedback
2013-07-19 23:07niffiwanResolutionfixed => reopened
2013-07-19 23:07niffiwanStatusfeedback => assigned
2013-07-19 23:11niffiwanDescription Updatedbug_revision_view_page.php?rev_id=588#r588
2013-07-19 23:11niffiwanAdditional Information Updatedbug_revision_view_page.php?rev_id=589#r589
2013-07-21 18:17niffiwanAdditional Information Updatedbug_revision_view_page.php?rev_id=592#r592
2013-08-05 04:00niffiwanFile Added: mantis2901b-svn.patch
2013-08-05 04:13niffiwanNote Added: 0015207
2013-08-05 04:13niffiwanStatusassigned => code review
2013-08-09 12:40z64555Note Added: 0015215
2013-08-09 12:41z64555Note Edited: 0015215bug_revision_view_page.php?bugnote_id=15215#r598
2013-08-09 22:45niffiwanFile Added: mantis2901c-svn.patch
2013-08-09 22:50niffiwanNote Added: 0015218
2013-08-09 22:51niffiwanNote Edited: 0015218bug_revision_view_page.php?bugnote_id=15218#r600
2013-08-10 00:27z64555Note Added: 0015219
2013-08-10 00:28z64555File Added: 2901d.zip
2013-08-10 08:04z64555Note Edited: 0015219bug_revision_view_page.php?bugnote_id=15219#r602
2013-08-10 08:11z64555Note Edited: 0015219bug_revision_view_page.php?bugnote_id=15219#r603
2013-08-11 04:22niffiwanNote Added: 0015222
2013-08-11 04:24niffiwanNote Added: 0015225
2013-08-11 04:24niffiwanStatuscode review => resolved
2013-08-11 04:24niffiwanFixed in Version => 3.7.0
2013-08-11 04:24niffiwanResolutionreopened => fixed
2013-08-14 09:50Goober5000Changeset attached => fs2open trunk r9745