View Issue Details

IDProjectCategoryView StatusLast Update
0003077FSSCPSEXPspublic2014-08-30 04:42
ReporterParias Assigned Tokarajorma  
PriorityhighSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
PlatformPCOSWindowsOS Version7 (x64)
Product Version3.7.1 
Target Version3.7.2 
Summary0003077: show-subtitle-text SEXP causes MP clients (but not host) to crash if message over 32 characters is sent
Descriptionshow-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):

===
Debug error!

Program: E:\Freespace2\fs2_open_3_7_1-DEBUG.exe
Module: E:\Freespace2\fs2_open_3_7_1-DEBUG.exe
File: d:\temp\fs2source\code\parse\sexp.cpp
Line: 21119

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)

Easiest repro:
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 InformationI'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[32]" 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.
TagsNo tags attached.

Activities

Parias

2014-07-22 02:14

developer  

Blank.fs2 (4,687 bytes)

Parias

2014-07-22 02:15

developer  

subtitle-crash-debug.PNG (98,490 bytes)   
subtitle-crash-debug.PNG (98,490 bytes)   

Parias

2014-07-22 02:15

developer  

subtitle-crash-2.PNG (64,113 bytes)   
subtitle-crash-2.PNG (64,113 bytes)   

Parias

2014-07-22 02:17

developer   ~0016116

Last edited: 2014-07-22 02: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.

Parias

2014-07-22 03:34

developer   ~0016117

Last edited: 2014-07-22 03: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 [256] 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.

Goober5000

2014-07-25 03:44

administrator   ~0016132

Pinging karajorma for his multiplayer expertise...

karajorma

2014-07-28 15:02

administrator   ~0016140

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.

Parias

2014-07-29 02:10

developer   ~0016142

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 [256] 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.

karajorma

2014-08-15 01:34

administrator   ~0016216

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.

Parias

2014-08-15 02:06

developer   ~0016217

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.

Echelon9

2014-08-15 10:50

developer   ~0016218

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;
    char text[256];

...
}

and

void multi_sexp_show_subtitle_text()
{
    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.

Goober5000

2014-08-16 01:20

administrator   ~0016220

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.

karajorma

2014-08-30 04:42

administrator   ~0016264

Fix committed to trunk@11034.

Related Changesets

fs2open: trunk r11034

2014-08-30 01:13

karajorma


Ported: N/A

Details Diff
Fix Mantis 3077 (Subtitles including a message will crash multiplayer games). Affected Issues
0003077
mod - /trunk/fs2_open/code/parse/sexp.cpp Diff File

Issue History

Date Modified Username Field Change
2014-07-22 02:14 Parias New Issue
2014-07-22 02:14 Parias File Added: Blank.fs2
2014-07-22 02:15 Parias File Added: subtitle-crash-debug.PNG
2014-07-22 02:15 Parias File Added: subtitle-crash-2.PNG
2014-07-22 02:17 Parias Note Added: 0016116
2014-07-22 02:18 Parias Note Edited: 0016116
2014-07-22 03:34 Parias Note Added: 0016117
2014-07-22 03:35 Parias Note Edited: 0016117
2014-07-22 03:45 Parias Note Edited: 0016117
2014-07-25 03:44 Goober5000 Note Added: 0016132
2014-07-25 03:44 Goober5000 Assigned To => karajorma
2014-07-25 03:44 Goober5000 Status new => assigned
2014-07-25 03:44 Goober5000 Target Version => 3.7.2
2014-07-28 15:02 karajorma Note Added: 0016140
2014-07-29 02:10 Parias Note Added: 0016142
2014-08-15 01:34 karajorma Note Added: 0016216
2014-08-15 02:06 Parias Note Added: 0016217
2014-08-15 10:50 Echelon9 Note Added: 0016218
2014-08-16 01:20 Goober5000 Note Added: 0016220
2014-08-30 04:42 karajorma Changeset attached => fs2open trunk r11034
2014-08-30 04:42 karajorma Note Added: 0016264
2014-08-30 04:42 karajorma Status assigned => resolved
2014-08-30 04:42 karajorma Resolution open => fixed