|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0003077||FSSCP||SEXPs||public||2014-07-21 22:14||2014-08-30 00:42|
|Platform||PC||OS||Windows||OS Version||7 (x64)|
|Target Version||3.7.2||Fixed in Version|
|Summary||0003077: show-subtitle-text SEXP causes MP clients (but not host) to crash if message over 32 characters is sent|
|Description||show-subtitle-text is supposed to support messages up to 255 characters in length, but in actuality doesn't seem to properly support more than 32 characters in multiplayer. If a client tries to process this SEXP while connected to a host running the mission, the client will crash hard with an error similar to the following (note: I just compiled a vanilla 3.7.1 build from source, so some extra debug info is included):|
Run-Time Check Failure 0000002 - Stack around the variable 'x_pos' was corrupted. (In testing this might sometimes be "y_pos" instead)
The host will display the subtitle properly and continue on fine - only the client system is affected. The actual error only appears if you're running in debug mode of course - the Release build just throws a generic Windows crash report.
|Steps To Reproduce||(This assumes non-modded FS2 install running 3.7.1, but I've been able to reproduce this with all 3.7 builds I've tried so far)|
1) Grab the attached mission - blank.fs2
2) Run a dedicated server.
3) On the same system (apply custom port parameter if needed), launch a debug build of the game and join your server as a client. Launch the mission.
4) Wait about 3 seconds for the event to trigger - the game will crash.
5) Open blank.fs2 in FRED. Edit the Events list and change the "Some words" message so it has 31 or less characters.
6) Try to repro the problem and note the mission behaves normally - subtitle text appears for all relevant parties and no client crash occurs.
|Additional Information||I'm going to attach a couple of additional images -- subtitle-crash-2.PNG shows the error message appearing as a client. subtitle-crash-debug.PNG shows the debug stack from VS2013 (which I had attached to the process at the time) - you can see it jumps straight to multi_sexp_show_subtitle_text() in sexp.cpp as where the problem is occurring. You can also see the call stack at the time the problem happened.|
Stepping through the list of variables at the time, I was able to confirm that only the first 32 characters from my message were listed in the "text" array while the others didn't appear at all (consistent with how this is a "char" type). I guess if it's getting more data than it knows how to handle, the extra characters are spilling into the data space for the x and y_pos variables somehow and breaking the world. I have literally zero C++ expertise though and can postulate from what I'm seeing, so go easy =)
A patch for this would be awesome, as I'm working on a mod / campaign pack reliant on firing large subtitle strings to clients for briefing purposes; I may have a LUA-based workaround involving gr.drawString and multi-eval (which I salvaged from http://www.hard-light.net/forums/index.php?topic=86422 and integrated into my own build - I may put in a separate request to get this into trunk as I really would like to have access to it) but simply having show-subtitle-text work consistently for all clients would be a really great fix for me.
|Tags||No tags attached.|
Last edited: 2014-07-21 22:18
Wanted to mention as well - this may have some relation to issue 0003050 which is the same sort of error and problem (game crashes with a "stack was corrupted" error because a given value seems to be too long) - although I'm not sure if the relation goes any deeper than that given the disparate nature of the symptoms.
Last edited: 2014-07-21 23:45
Sorry for the spam - one more update. On a casual whim I decided to try changing char text [TOKEN_LENGTH] in multi_sexp_show_subtitle_text() to char text  instead (to match sexp_show_subtitle_text()). This fixed the client-side crashing, and even enables the text to actually appear client-side in it's entirety, which is awesome!
I'm really not sure on the implications of this though (since TOKEN_LENGTH - hard-coded at 32 - is referenced just about everywhere I'm assuming there was some multiplayer-specific significance in using it here) - the game seems to run just fine when the event triggers with this large a value, but I do notice a few formatting inconsistencies... i.e. width doesn't seem to be properly respected, so text is arranged oddly compared to the host system (and even gets completely jumbled up if you're connecting to a standalone server).
If I had to guess, I'd wonder if the check for width (which works as a percentage, I think?) is being calculated by the host system and is then fired out, rather than each client calculating it independently. I might be wrong though and need to do more testing.
I'll try to avoid rambling on for much longer, but if I find anything else that might be useful towards making this work correctly in multiplayer I'll post an update.
Edit: Yup, if I'm reading sexp.cpp right, that's exactly what happens. The pixel positions are calculated on the host in sexp_show_subtitle_text, and are then received by multi_sexp_show_subtitle_text. I'd like to think it wouldn't be a big deal to set up some local functions in multi_sexp_show_subtitle_text to check this stuff locally, but I'd like to defer to some expertise first.
|Pinging karajorma for his multiplayer expertise...|
I'm not going to be able to look at this for a couple of weeks unfortunately. However any increase in the size of TOKEN_LENGTH is likely to have a very nasty knock on effect on another (so far minor) bug if that constant is also used in multi_sexp.cpp.
Try making a single SEXP make multiple calls to the string sending fuctions so that more than 512 bytes needs to be sent. I suspect you'll crash the game. In fact I'm pretty damn sure you will.
Thanks for commenting karajorma. Sorry as I might be misunderstanding --
"....any increase in the size of TOKEN_LENGTH is likely to have a very nasty knock on effect on another (so far minor) bug if that constant is also used in multi_sexp.cpp. "
I'm actually not making any changes to the fixed TOKEN_LENGTH value in globals.h (where I think it's defined?), but instead I'm just changing the "char text" line in multi_sexp_show_subtitle_text() from [TOKEN_LENGTH] to  instead. The TOKEN_LENGTH define in itself is untouched and left at 32, so everything else calling it would be unaffected.
Is this what you're referring to as potentially causing other issues?
And could you provide an example on how I'd get the string sending functions to fire more than 512 bytes at a time? Right now I've got multiple show-subtitle-text SEXPs firing at the same time in an event (up to four, each with generally 200+ characters of text on average) and so far it seems to be working OK. I'm not sure if the way I'm doing it is breaking up the traffic into multiple packets though.
|Sorry, meant to reply to this one earlier. I think your fix can be implemented as is. I was speaking more generally about the multi_sexp code. There is a rather nasty bug in it which this may or may not expose. I suspect in this case you'll probably get away with it.|
Cool! No worries. I've been doing a lot of host + client testing (between two disparate systems on a local network) with this change in place as well, and so far have not seen any signs of stability or functionality problems caused it. My own local testlab is of course a bit limited and we may see different results over the long-term, but so far I think this should work OK.
Thanks for following up. There's still an issue I see as well with the height / width calculations for this function being done exclusively "host-side" (which leads to really weird results in terms of subtitle positioning if the host and client are running very different screen resolutions), but I'm going to submit this as a separate report after I have a chance to investigate it more thoroughly and perhaps propose a fix.
Yes, the underlying issue is that that code the stuffs a subtitle into a SEXP multiplayer packet -- sexp_show_subtitle_text() -- allows the packet to contain up to 256 chars whereas the receiving code --
void sexp_show_subtitle_text(int node)
int i, n = node;
int x_pos, y_pos, width=0, fontnum;
char text[TOKEN_LENGTH]; // Currently defined as 32 chars
Regardless of the length we choose, both the sending and receiving function should use the same #define length for the sending and receiving array.
|Based on Echelon9's detective work, I think I know what happened. Originally, the sexp could only display the text entered into the actual sexp node. This is limited to TOKEN_LENGTH. At some point, show-subtitle-text (or maybe show-subtitle before it) was modified to allow subtitles to be obtained from messages, which can be longer than TOKEN_LENGTH. But the corresponding multi sexp wasn't updated accordingly.|
|Fix committed to trunk@11034.|
fs2open: trunk r11034
Timestamp: 2014-08-30 01:13:56
|Fix Mantis 3077 (Subtitles including a message will crash multiplayer games).|
|mod - /trunk/fs2_open/code/parse/sexp.cpp|
|2014-07-21 22:14||Parias||New Issue|
|2014-07-21 22:14||Parias||File Added: Blank.fs2|
|2014-07-21 22:15||Parias||File Added: subtitle-crash-debug.PNG|
|2014-07-21 22:15||Parias||File Added: subtitle-crash-2.PNG|
|2014-07-21 22:17||Parias||Note Added: 0016116|
|2014-07-21 22:18||Parias||Note Edited: 0016116||View Revisions|
|2014-07-21 23:34||Parias||Note Added: 0016117|
|2014-07-21 23:35||Parias||Note Edited: 0016117||View Revisions|
|2014-07-21 23:45||Parias||Note Edited: 0016117||View Revisions|
|2014-07-24 23:44||Goober5000||Note Added: 0016132|
|2014-07-24 23:44||Goober5000||Assigned To||=> karajorma|
|2014-07-24 23:44||Goober5000||Status||new => assigned|
|2014-07-24 23:44||Goober5000||Target Version||=> 3.7.2|
|2014-07-28 11:02||karajorma||Note Added: 0016140|
|2014-07-28 22:10||Parias||Note Added: 0016142|
|2014-08-14 21:34||karajorma||Note Added: 0016216|
|2014-08-14 22:06||Parias||Note Added: 0016217|
|2014-08-15 06:50||Echelon9||Note Added: 0016218|
|2014-08-15 21:20||Goober5000||Note Added: 0016220|
|2014-08-30 00:42||karajorma||Changeset attached||=> fs2open trunk r11034|
|2014-08-30 00:42||karajorma||Note Added: 0016264|
|2014-08-30 00:42||karajorma||Status||assigned => resolved|
|2014-08-30 00:42||karajorma||Resolution||open => fixed|