View Issue Details

IDProjectCategoryView StatusLast Update
0001931FSSCPuser interfacepublic2022-06-11 00:59
ReporterFUBAR-BDHR Assigned To 
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.6.9 
Fixed in Version22.2.0 
Summary0001931: Having more than 39 ship classes in loadout causes 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.

Activities

karajorma

2009-05-30 07:25

administrator   ~0010921

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?

FUBAR-BDHR

2009-05-30 07:34

developer   ~0010922

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.

karajorma

2009-06-02 21:13

administrator   ~0010934

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.

FUBAR-BDHR

2009-06-03 03:24

developer   ~0010935

Last edited: 2009-06-03 05: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.

2009-06-03 03:46

 

FUBAR_MG03.FS2 (98,748 bytes)

karajorma

2009-06-03 10:06

administrator   ~0010936

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.

Echelon9

2009-06-03 11:38

developer   ~0010937

Last edited: 2009-06-03 11: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.

2009-06-03 11:38

 

mantis-1931.diff (437 bytes)   
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);
 
mantis-1931.diff (437 bytes)   

karajorma

2009-06-03 20:04

administrator   ~0010939

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.

Echelon9

2012-04-03 13:42

developer   ~0013448

Any updates on the network packet change?

karajorma

2012-12-30 07:09

administrator   ~0014588

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.

niffiwan

2014-08-26 11:16

developer   ~0016262

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.

Goober5000

2014-09-24 06:40

administrator   ~0016293

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?

karajorma

2014-10-08 02:06

administrator   ~0016330

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

FSCyborg

2022-06-10 20:58

developer   ~0017134

Close as this section of code was redone during the multi packet fixes.

Issue History

Date Modified Username Field Change
2009-05-30 00:22 FUBAR-BDHR New Issue
2009-05-30 07:25 karajorma Note Added: 0010921
2009-05-30 07:26 karajorma Status new => assigned
2009-05-30 07:26 karajorma Assigned To => karajorma
2009-05-30 07:34 FUBAR-BDHR Note Added: 0010922
2009-06-02 21:13 karajorma Note Added: 0010934
2009-06-03 03:24 FUBAR-BDHR Note Added: 0010935
2009-06-03 03:44 FUBAR-BDHR Note Edited: 0010935
2009-06-03 03:46 FUBAR-BDHR File Added: FUBAR_MG03.FS2
2009-06-03 05:00 FUBAR-BDHR Note Edited: 0010935
2009-06-03 10:06 karajorma Note Added: 0010936
2009-06-03 11:38 Echelon9 Note Added: 0010937
2009-06-03 11:38 Echelon9 File Added: mantis-1931.diff
2009-06-03 11:41 Echelon9 Note Edited: 0010937
2009-06-03 11:41 Echelon9 Note Edited: 0010937
2009-06-03 20:04 karajorma Note Added: 0010939
2012-04-03 13:42 Echelon9 Note Added: 0013448
2012-04-03 13:42 Echelon9 Status assigned => feedback
2012-12-30 07:09 karajorma Note Added: 0014588
2012-12-30 07:09 karajorma Target Version => 3.7.2
2012-12-30 07:10 karajorma Assigned To karajorma =>
2014-08-26 11:16 niffiwan Note Added: 0016262
2014-09-24 06:40 Goober5000 Note Added: 0016293
2014-09-24 06:40 Goober5000 Target Version 3.7.2 =>
2014-10-08 02:06 karajorma Note Added: 0016330
2022-06-10 20:58 FSCyborg Note Added: 0017134
2022-06-10 20:58 FSCyborg Status feedback => closed
2022-06-11 00:59 Goober5000 Status closed => resolved
2022-06-11 00:59 Goober5000 Resolution open => fixed
2022-06-11 00:59 Goober5000 Fixed in Version => 22.2.0
2022-06-11 00:59 Goober5000 Summary Having more then 39 ship classes in loadout casues crash => Having more than 39 ship classes in loadout causes crash