2018-12-17 11:20 EST


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0003072FSSCPtablespublic2014-07-09 20:51
ReporterYarn 
Assigned ToGoober5000 
PriorityhighSeverityblockReproducibilityalways
StatusresolvedResolutionfixed 
Platformx64OSWindows 7OS Version
Product Version3.7.2 RC3 
Target Version3.7.2Fixed in Version 
Summary0003072: German section of retail strings.tbl crashes game
DescriptionStarting with revision 10753, if the German section of the retail strings.tbl is read, the game crashes with this error message:

strings.tbl(line 4226):
Error: Expecting int, found [; ------------------------------------------------].

ntdll.dll! ZwWaitForSingleObject + 21 bytes
kernel32.dll! WaitForSingleObjectEx + 67 bytes
kernel32.dll! WaitForSingleObject + 18 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! SCP_DumpStack + 354 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! Error + 229 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! error_display + 369 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! atoi2 + 109 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! stuff_int + 35 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! parse_stringstbl + 298 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! lcl_xstr_init + 131 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! game_init + 418 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! game_main + 519 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! WinMain + 328 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! __tmainCRTStartup + 296 bytes
fs2_open_3_7_1_SSE2-DEBUG.exe! WinMainCRTStartup + 13 bytes
kernel32.dll! BaseThreadInitThunk + 18 bytes
ntdll.dll! RtlInitializeExceptionChain + 99 bytes
ntdll.dll! RtlInitializeExceptionChain + 54 bytes


The problematic line in strings.tbl is 4224, which looks like this (no characters were added):

1402, "Sie haben IPX-Protokoll als Protokoll ausgewählt, aber dieses Protokoll ist auf Ihrer Maschine nicht installiert."."

Notice how the period and quotation mark at the end are doubled. This is what is causing the crash in revision 10753 and later. Since this is a retail file, FSO needs to cope with this table error to avoid breaking the "don't break retail" rule.
Steps To ReproduceSet the language to German and run FSO with no mods. In revision 10753 and later (but not earlier revisions), the game will crash with the error message above.
TagsNo tags attached.
Attached Files
  • patch file icon Mantis3072-parselo.cpp.patch (1,142 bytes) 2014-07-09 20:03 -
    Index: code/parse/parselo.cpp
    ===================================================================
    --- code/parse/parselo.cpp	(revision 10907)
    +++ code/parse/parselo.cpp	(working copy)
    @@ -2123,6 +2123,20 @@
     	while ( (num_chars_read = parse_get_line(outbuf, PARSE_BUF_SIZE, raw_text, raw_text_len, mp_raw)) != 0 ) {
     		mp_raw += num_chars_read;
     
    +		// stupid hacks to make retail data work with fixed parser, per Mantis #3072
    +		if (!strcmp(outbuf, "1402, \"Sie haben IPX-Protokoll als Protokoll ausgew\xE4hlt, aber dieses Protokoll ist auf Ihrer Maschine nicht installiert.\".\"")) {
    +			outbuf[121] = ' ';
    +			outbuf[122] = ' ';
    +		} else if (!strcmp(outbuf, "1117, \"\\r\\n\"Aucun web browser trouva. Del\xE0 isn't on emm\xE9nagea ou if \\r\\non est emm\xE9nagea, ca isn't set pour soient la default browser.\\r\\n\\r\\n\"")) {
    +			char *ch = &outbuf[11];
    +			do {
    +				*ch = *(ch+1);
    +				++ch;
    +			} while (*ch);
    +		} else if (!strcmp(outbuf, "1337, \"(fr)Loading\"")) {
    +			outbuf[3] = '6';
    +		}
    +
     		strip_comments(outbuf, in_quote, in_multiline_comment_a, in_multiline_comment_b);
     
     		maybe_convert_foreign_characters(outbuf);
    
    patch file icon Mantis3072-parselo.cpp.patch (1,142 bytes) 2014-07-09 20:03 +

-Relationships
related to 0003001resolvedGoober5000 Multiline comments problems 
+Relationships

-Notes

~0016016

Goober5000 (administrator)

Ew, yuck.

I think the best way to solve this is to add a "magic number" check that will treat that line as a special case and truncate it before the parser chokes on it.

~0016018

Yarn (developer)

The game did not truncate that line, at least not at the stage that removes comments.

I noticed that line 5929 (in the French section) also has this problem, and it causes a similar crash.

I think the best way to fix this is to handle Mantis 0003001 only in real XSTR structures, not just anywhere that double quotes exist. That way, you won't need a hack for two (or perhaps more) specific lines in the game data.

~0016020

MageKing17 (developer)

Except that if we encountered this exact problem in a mod, we would want an error to be thrown in that situation; it's a data error. Unfortunately, it's a data error present in the retail data, hence why the best way to handle it probably is to special-case these two (or more) lines.

~0016021

Yarn (developer)

Now that I think about it, my suggestion is a bad idea since it wouldn't allow !* and *! to be used in translated strings. So, it looks like this does have to be treated as a special case after all.

However it's handled, though, the strings in question should not be truncated; rather, if a quoted string begins on one of those lines, and there are three quotation marks on that line, then the string should end at the third quotation mark, not the second. This is important for line 5929, where the second quotation mark is placed before the actual message.

I also noticed that having an odd number of double quotes in a mission note (yes, double quotes are allowed there) breaks the mission in current revisions, even though it didn't happen in retail and older revisions. Multi-line comment delimiters and quotation marks must not have any effect on the comment system if they're placed in the mission notes. (To be clear, I'm talking about the mission notes, not the description.)

~0016022

Goober5000 (administrator)

By truncation, I meant that I would treat this line...
1402, "Sie haben IPX-Protokoll als Protokoll ausgewählt, aber dieses Protokoll ist auf Ihrer Maschine nicht installiert."."
...as if it were this...
1402, "Sie haben IPX-Protokoll als Protokoll ausgewählt, aber dieses Protokoll ist auf Ihrer Maschine nicht installiert."

But it wouldn't necessarily have to be truncation. Since it's a special case, I could apply whatever special processing needed to be done on a line-by-line basis.

The reason the game didn't choke prior to r10753 is that the parser forgot all about quotes once it moved to a new line. If it hadn't, this bug would have been present in retail.

Can you attach an example of a problematic Mission Notes section? Are there any examples in released missions?

~0016023

Yarn (developer)

Last edited: 2014-07-04 17:26

View 2 revisions

Yes, I know what you meant by truncation. I was saying that the strings shouldn't be truncated like that, at least not before or during comment removal.

I took a look at all the missions that ever shipped with the game (as far as I know), and the only ones that use double quotes in the notes are X_One_Step_Too_Far.fs2 and X_Rites_of_Passage.fs2, but those aren't problematic since they use an even number of double quotes in their notes.

Since XSTR structures actually are supported in mission notes (but aren't normally used), even in retail, I suggest the following: Don't use special treatment for mission notes during comment removal, but do have FRED save mission notes in XSTR structures in the same manner as all other mission strings (including the conversion of semicolons and double quotes to $semicolon and $quote). This way, FREDders can't "break" a mission just by adding the wrong characters to the notes. (EDIT: Actually, scratch everything in this paragraph; FRED never actually converts semicolons and double quotes to $semicolon and $quote. It's probably better to tell mission designers not to use double quotes in mission notes, even if they see them used in some existing missions.)

~0016024

MageKing17 (developer)

Well, if you don't truncate that string, you get two full stops.

FRED most certainly does convert semicolons and quotes into $semicolon and $quote. In the events editor's message editing it does, at least, and I believe the briefing editors as well. Of course, the $semicolon conversion isn't actually necessary anymore due to some of these changes in comment parsing.

~0016034

Goober5000 (administrator)

Okay, have a look at the patch I uploaded. Seems to fix the issues for me (the two you mentioned plus a third I discovered while testing).

~0016037

Yarn (developer)

For the German string at least, I would have it check the whole string (including the extra period and quotation mark) rather than just the stuff before the first non-ASCII character. That way, if a mod changes that string in a way that makes it longer (unlikely, but still possible), the line will not be truncated too early. (You can use \xE4 in the place of ä.)

I should also note that the way your patch fixes the German string strips out any X-offset information that might exist on that line. It's probably not a big deal, though, since that string is used only in a pop-up message, and thus the offset information is not used.

~0016041

Goober5000 (administrator)

Thanks for the character tip. I've modified the patch. If you can tell me the character codes for the French string, I can change that too.

What do you mean by X-offset information?

~0016044

Yarn (developer)

The codes are \xE0 for à and \xE9 for é.

By X-offset information, I mean the one or two integers that can appear after the string of a strings.tbl entry. It's generally used in languages other than English where text is supposed to be right-aligned or centered on something, like a button. More information can be found in the strings.tbl page of the wiki. (That documentation was only recently added to that page despite the fact that this feature has been supported since retail.)

~0016053

Goober5000 (administrator)

Patch revised again. Unfortunately, it looks like those characters won't work for string matching, even though they appear to match when printed to the log. I may have to go back to matching only the non-ASCII characters.

~0016054

Yarn (developer)

I figured out how the problem with your patch: You aren't looking for the newline characters. You need to include them in your searches. For example, with the last string in your patch (since it's short), you need to search for this:

"1337, \"(fr)Loading\"\n"

This fixed it for me at least. Note that you need to use \n, not \\n.

~0016055

Goober5000 (administrator)

Derp. You are correct. Well, that's what I get for trying to code on four hours of sleep.

I think this covers all the issues then. Committing.

~0016056

Goober5000 (administrator)

Fix committed to trunk@10908.
+Notes

+Related Changesets

-Issue History
Date Modified Username Field Change
2014-07-03 01:15 Yarn New Issue
2014-07-03 01:15 Yarn Relationship added related to 0003001
2014-07-03 20:29 Goober5000 Note Added: 0016016
2014-07-03 20:29 Goober5000 Assigned To => Goober5000
2014-07-03 20:29 Goober5000 Status new => assigned
2014-07-04 00:24 Yarn Note Added: 0016018
2014-07-04 01:50 MageKing17 Note Added: 0016020
2014-07-04 13:22 Yarn Note Added: 0016021
2014-07-04 14:53 Goober5000 Note Added: 0016022
2014-07-04 17:18 Yarn Note Added: 0016023
2014-07-04 17:26 Yarn Note Edited: 0016023 View Revisions
2014-07-04 18:01 MageKing17 Note Added: 0016024
2014-07-07 04:02 Goober5000 File Added: Mantis3072-parselo.cpp.patch
2014-07-07 04:03 Goober5000 Note Added: 0016034
2014-07-07 04:03 Goober5000 Status assigned => code review
2014-07-07 15:35 Yarn Note Added: 0016037
2014-07-09 02:41 Goober5000 File Deleted: Mantis3072-parselo.cpp.patch
2014-07-09 02:42 Goober5000 File Added: Mantis3072-parselo.cpp.patch
2014-07-09 02:44 Goober5000 Note Added: 0016041
2014-07-09 12:52 Yarn Note Added: 0016044
2014-07-09 20:03 Goober5000 File Deleted: Mantis3072-parselo.cpp.patch
2014-07-09 20:03 Goober5000 File Added: Mantis3072-parselo.cpp.patch
2014-07-09 20:04 Goober5000 Note Added: 0016053
2014-07-09 20:36 Yarn Note Added: 0016054
2014-07-09 20:51 Goober5000 Note Added: 0016055
2014-07-09 20:51 Goober5000 Changeset attached => fs2open trunk r10908
2014-07-09 20:51 Goober5000 Note Added: 0016056
2014-07-09 20:51 Goober5000 Status code review => resolved
2014-07-09 20:51 Goober5000 Resolution open => fixed
+Issue History