2020-05-30 20:30 EDT


View Issue Details Jump to Notes ]
IDProjectCategoryView StatusLast Update
0001931FSSCPuser interfacepublic2014-10-07 22:06
ReporterFUBAR-BDHR 
Assigned To 
PrioritynormalSeveritycrashReproducibilityalways
StatusfeedbackResolutionopen 
Product Version3.6.9 
Target VersionFixed in Version 
Summary0001931: Having more then 39 ship classes in loadout casues crash
DescriptionTBP now has 40 player allowed ships and one multiplayer mission was set up for all 40. No problems adding them in FRED but every time you try to select a ship in loadout you crash. 39 ships = no problem and it doesn't matter which ship I remove.

Crash is at line 1700 in missionscreencommon.cpp
Additional Information3.6.10 RC2 running Ultimate Gauntlet. Bad version is in Zathras RC3. I fixed it by removing the maint bot so if you are using a newer version just allow it in FRED.
TagsNo tags attached.
Attached Files
  • ? file icon FUBAR_MG03.FS2 (98,748 bytes) 2009-06-02 23:46
  • diff file icon mantis-1931.diff (437 bytes) 2009-06-03 07:38 -
    Index: code/missionui/missionscreencommon.cpp
    ===================================================================
    --- code/missionui/missionscreencommon.cpp	(revision 5327)
    +++ code/missionui/missionscreencommon.cpp	(working copy)
    @@ -1712,6 +1712,7 @@
     	}
     
     	player_id = INTEL_SHORT( player_id );
    +	Assert( offset += sizeof(player_id) < max_size );
     	memcpy(block+offset,&player_id,sizeof(player_id));
     	offset += sizeof(player_id);
     
    
    diff file icon mantis-1931.diff (437 bytes) 2009-06-03 07:38 +

-Relationships
+Relationships

-Notes

~0010921

karajorma (administrator)

Client or server side crash? (Or client selection causing a server side crash?).

At first glance I suspect that this is either due to the number of ship or weapon classes in TBP rather than an odd limit of 39 ships. What happens if you leave the mainbot in and take out the Starfury then put it back?

~0010922

FUBAR-BDHR (developer)

Host definitely. Didn't try client myself but Andicirk said clients crashed too either at loadout or mission start. Guessing they crash at mission start if they don't try to change ships.

~0010934

karajorma (administrator)

Works fine for me with the 3.6.11 build. This could be due to something that was fixed when I edited things for team loadout.

~0010935

FUBAR-BDHR (developer)

Last edited: 2009-06-03 01:00

Still getting it. Just compiled 3.6.11 inferno. It actually gave me some debug info.

Run-time check failure 0000002 - stack around the variable 'wss_data' was corrupted.

And from the log:

ASSERTION: "offset < max_size" at missionscreencommon.cpp:1718

Actually getting it even with the 39 ship version that worked in 3.6.10 RC3

More testing 38 now seems to be the limit.

If it still works for you can you get the 3.6.11 Inferno for me to test with.

~0010936

karajorma (administrator)

Works just fine for me. This may be due to a packet being sent from client to server (I was trying a multiplayer coop on my own to test.) I'll try with a client present later today.

~0010937

Echelon9 (developer)

Last edited: 2009-06-03 07:41

Based on the debug info FUBAR provided, I'd say this crash is caused by the memcpy() at missionscreencommon.cpp:1715.

memcpy(block+offset,&player_id,sizeof(player_id));

The 'offset' is used internally in store_wss_data() to keep track of how far past the start of the 'block' passed into store_wss_data() we should be writing, but it seems there's no check done before each memcpy that 'offset' isn't already past, or will go past, the 'max_size' passed in.

While we would still have to trust that the 'max_size' passed is correct, there's at least one spot in store_wss_data() where the Assert() should go before the memory operation.

This attached patch won't solve the underlying cause - the inputs to store_wss_data() can't accommodate all the data already and about to be written to the block - but it should stop the memory corruption, with an Assert() instead.

~0010939

karajorma (administrator)

The problem is that the Assert only solves one problem while leaving a whole load more open and waiting to bite us on the arse. And it doesn't do anything to solve the problem in release builds.

Since the offset is also increased in the loops for ships and weapons there is a chance that it could go over the packet size limit in them too. Admittedly it's unlikely to do so in ship unless we bump the number of ships allowed again but it's very likely to happen in the weapons loop. Since ships add a ubyte while weapons add a short a single extra weapon would have corrupted the mission in a different place.

One solution I can see to the problem is to break the update into two packets (Like I did with the ship update packet that used to break Inferno multiplayer). Another would simply be to bump MAX_PACKET_SIZE. Although I'm not certain what else that could break.

Either way, whatever we do here is going to break compatibility with older builds unless we simply make it error out.

~0013448

Echelon9 (developer)

Any updates on the network packet change?

~0014588

karajorma (administrator)

I think I'm going to add this to my proposed rewrite of the loadout code. There are just too many things wrong with it to waste time patching it now when the entire thing really needs to be rewritten without the assumptions that Volition made when they originally wrote the code.

~0016262

niffiwan (developer)

I think this this thread probably hit the same issue?
http://www.hard-light.net/forums/index.php?topic=88136.0

I had a look in TBP's ship tables to see if there were any other fighter-class ships that I could add to the mission FUBAR_MG3.fs2, but I couldn't see anything obvious. I'll instead have a go at reproducing it in the Aerotech TC.

~0016293

Goober5000 (administrator)

Since Karajorma wants to rewrite the loadout code, this shouldn't be targeted for 3.7.2.

Karajorma, is there any sort of error condition you can add in the meantime to have the builds fail gracefully, rather than crashing?

~0016330

karajorma (administrator)

I can definitely look into a better solution than a crash.
+Notes

-Issue History
Date Modified Username Field Change
2009-05-29 20:22 FUBAR-BDHR New Issue
2009-05-30 03:25 karajorma Note Added: 0010921
2009-05-30 03:26 karajorma Status new => assigned
2009-05-30 03:26 karajorma Assigned To => karajorma
2009-05-30 03:34 FUBAR-BDHR Note Added: 0010922
2009-06-02 17:13 karajorma Note Added: 0010934
2009-06-02 23:24 FUBAR-BDHR Note Added: 0010935
2009-06-02 23:44 FUBAR-BDHR Note Edited: 0010935
2009-06-02 23:46 FUBAR-BDHR File Added: FUBAR_MG03.FS2
2009-06-03 01:00 FUBAR-BDHR Note Edited: 0010935
2009-06-03 06:06 karajorma Note Added: 0010936
2009-06-03 07:38 Echelon9 Note Added: 0010937
2009-06-03 07:38 Echelon9 File Added: mantis-1931.diff
2009-06-03 07:41 Echelon9 Note Edited: 0010937
2009-06-03 07:41 Echelon9 Note Edited: 0010937
2009-06-03 16:04 karajorma Note Added: 0010939
2012-04-03 09:42 Echelon9 Note Added: 0013448
2012-04-03 09:42 Echelon9 Status assigned => feedback
2012-12-30 02:09 karajorma Note Added: 0014588
2012-12-30 02:09 karajorma Target Version => 3.7.2
2012-12-30 02:10 karajorma Assigned To karajorma =>
2014-08-26 07:16 niffiwan Note Added: 0016262
2014-09-24 02:40 Goober5000 Note Added: 0016293
2014-09-24 02:40 Goober5000 Target Version 3.7.2 =>
2014-10-07 22:06 karajorma Note Added: 0016330
+Issue History