View Issue Details
| ID | Project | Category | View Status | Date Submitted | Last Update |
|---|---|---|---|---|---|
| 0002688 | FSSCP | modular tables | public | 2012-07-14 17:06 | 2012-07-15 18:46 |
| Reporter | MjnMixael | Assigned To | m_m | ||
| Priority | normal | Severity | crash | Reproducibility | always |
| Status | resolved | Resolution | fixed | ||
| Product Version | 3.6.13 | ||||
| Summary | 0002688: Long credits causes stack overflow crash. | ||||
| Description | If the total length of credits.tbl + code credits + credits.tbms is too long, FSO will crash when you enter the credits viewer. | ||||
| Steps To Reproduce | Place the attached tbm into data/tables and enter the credits viewer. | ||||
| Additional Information | From IRC discussion with The_E... <@The_E> MjnMixael: Would you be so kind as to throw it on Mantis, for the attention of whoever coded the credits.tbl modularization? <@MjnMixael> sure <@The_E> The crash cause is a stack overflow due to excessive amounts of text <@MjnMixael> it was m|m, btw <@The_E> This can be fixed by increasing the default stack size, but that needs a project file update <@MjnMixael> waaaaaaay out of my knowledge here, but is it possible to split up tbms into separate "stacks" and just display them in order of loading, or does it just not work like that? <@The_E> ___It does not work that way _ <@The_E> Err <@The_E> It does not work that way currently, but that would indeed be another possible solution | ||||
| Tags | No tags attached. | ||||
|
|
I added a patch which implements the suggested behavior and it seems to work as expected and without any crashes. |
|
|
The patch fixes the crash and displays all the credits properly. However it also breaks the following table options, as in they parse but don't have any visible effect. $Text Scroll Rate: $Artworks Display Time: $Artworks Fade Time: |
|
|
credits.patch (8,708 bytes)
Index: code/menuui/credits.cpp
===================================================================
--- code/menuui/credits.cpp (revision 9013)
+++ code/menuui/credits.cpp (working copy)
@@ -210,7 +210,7 @@
static float Credits_artwork_display_time = 9.0f;
static float Credits_artwork_fade_time = 1.0f;
-static SCP_string Credit_text;
+static SCP_vector<SCP_string> Credit_text_parts;
static bool Credits_parsed;
@@ -309,11 +309,11 @@
}
}
- parse_optional_float("$Text scroll rate:", &Credits_scroll_rate, 15.0f, 0.0f);
+ parse_optional_float("$Text scroll rate:", &Credits_scroll_rate, 0.0f);
- parse_optional_float("$Artworks display time:", &Credits_artwork_display_time, 9.0f, 0.0f);
+ parse_optional_float("$Artworks display time:", &Credits_artwork_display_time, 0.0f);
- parse_optional_float("$Artworks fade time:", &Credits_artwork_fade_time, 1.0f, 0.0f);
+ parse_optional_float("$Artworks fade time:", &Credits_artwork_fade_time, 0.0f);
if (optional_string("$SCP Credits position:"))
{
@@ -331,6 +331,7 @@
ignore_white_space();
+ SCP_string credits_text;
SCP_string line;
SCP_vector<int> charNum;
@@ -348,7 +349,7 @@
// Also don't append the default credits anymore when there was already a parsed table
if(first_run && !Credits_parsed && !line.compare(mod_check))
{
- Credit_text.append(unmodified_credits);
+ credits_text.append(unmodified_credits);
}
first_run = false;
@@ -356,7 +357,7 @@
if (line.empty())
{
// If the line is empty then just append a newline, don't bother with splitting it first
- Credit_text.append("\n");
+ credits_text.append("\n");
}
else
{
@@ -376,12 +377,14 @@
// Now add all splitted lines to the credit text and append a newline to the end
for (int i = 0; i < numLines; i++)
{
- Credit_text.append(SCP_string(lines[i], charNum[i]));
- Credit_text.append("\n");
+ credits_text.append(SCP_string(lines[i], charNum[i]));
+ credits_text.append("\n");
}
}
}
+ Credit_text_parts.push_back(credits_text);
+
Credits_parsed = true;
}
@@ -402,7 +405,7 @@
void credits_init()
{
- int i, w, h;
+ int i;
credits_screen_buttons *b;
int credits_spooled_music_index = event_music_get_spooled_music_index("Cinema");
@@ -418,9 +421,7 @@
Credits_frametime = 0;
Credits_last_time = timer_get_milliseconds();
-
- Credit_text = "";
-
+
// this is moved up here so we can override it if desired
Credits_artwork_index = rand() % NUM_IMAGES;
@@ -429,17 +430,19 @@
credits_parse();
if (!Credits_parsed)
- Credit_text.assign("No credits available.\n");
+ {
+ Credit_text_parts.push_back(SCP_string("No credits available.\n"));
+ }
else
{
switch (SCP_credits_position)
{
case START:
- Credit_text.insert(0, fs2_open_credit_text);
+ Credit_text_parts.insert(Credit_text_parts.begin(), fs2_open_credit_text);
break;
case END:
- Credit_text.append(fs2_open_credit_text);
+ Credit_text_parts.push_back(fs2_open_credit_text);
break;
default:
@@ -449,117 +452,128 @@
}
int ch;
- for (SCP_string::iterator ii = Credit_text.begin(); ii != Credit_text.end(); ++ii)
+ for (SCP_vector<SCP_string>::iterator iter = Credit_text_parts.begin(); iter != Credit_text_parts.end(); ++iter)
{
- ch = *ii;
- switch (ch)
+ for (SCP_string::iterator ii = iter->begin(); ii != iter->end(); ++ii)
{
- case -4:
- ch = 129;
- break;
+ ch = *ii;
+ switch (ch)
+ {
+ case -4:
+ ch = 129;
+ break;
- case -28:
- ch = 132;
- break;
+ case -28:
+ ch = 132;
+ break;
- case -10:
- ch = 148;
- break;
+ case -10:
+ ch = 148;
+ break;
- case -23:
- ch = 130;
- break;
+ case -23:
+ ch = 130;
+ break;
- case -30:
- ch = 131;
- break;
+ case -30:
+ ch = 131;
+ break;
- case -25:
- ch = 135;
- break;
+ case -25:
+ ch = 135;
+ break;
- case -21:
- ch = 137;
- break;
+ case -21:
+ ch = 137;
+ break;
- case -24:
- ch = 138;
- break;
+ case -24:
+ ch = 138;
+ break;
- case -17:
- ch = 139;
- break;
+ case -17:
+ ch = 139;
+ break;
- case -18:
- ch = 140;
- break;
+ case -18:
+ ch = 140;
+ break;
- case -60:
- ch = 142;
- break;
+ case -60:
+ ch = 142;
+ break;
- case -55:
- ch = 144;
- break;
+ case -55:
+ ch = 144;
+ break;
- case -12:
- ch = 147;
- break;
+ case -12:
+ ch = 147;
+ break;
- case -14:
- ch = 149;
- break;
+ case -14:
+ ch = 149;
+ break;
- case -5:
- ch = 150;
- break;
+ case -5:
+ ch = 150;
+ break;
- case -7:
- ch = 151;
- break;
+ case -7:
+ ch = 151;
+ break;
- case -42:
- ch = 153;
- break;
+ case -42:
+ ch = 153;
+ break;
- case -36:
- ch = 154;
- break;
+ case -36:
+ ch = 154;
+ break;
- case -31:
- ch = 160;
- break;
+ case -31:
+ ch = 160;
+ break;
- case -19:
- ch = 161;
- break;
+ case -19:
+ ch = 161;
+ break;
- case -13:
- ch = 162;
- break;
+ case -13:
+ ch = 162;
+ break;
- case -6:
- ch = 163;
- break;
+ case -6:
+ ch = 163;
+ break;
- case -32:
- ch = 133;
- break;
+ case -32:
+ ch = 133;
+ break;
- case -22:
- ch = 136;
- break;
+ case -22:
+ ch = 136;
+ break;
- case -20:
- ch = 141;
- break;
+ case -20:
+ ch = 141;
+ break;
+ }
+
+ *ii = (char) ch;
}
+ }
- *ii = (char) ch;
+ int temp_h;
+ int h = 0;
+
+ for (SCP_vector<SCP_string>::iterator iter = Credit_text_parts.begin(); iter != Credit_text_parts.end(); ++iter)
+ {
+ gr_get_string_size(NULL, &temp_h, iter->c_str(), iter->length());
+
+ h = h + temp_h;
}
- gr_get_string_size(&w, &h, Credit_text.c_str(), Credit_text.length());
-
Credit_start_pos = i2fl(Credits_text_coords[gr_screen.res][CREDITS_H_COORD]);
Credit_stop_pos = -i2fl(h);
Credit_position = Credit_start_pos;
@@ -611,7 +625,7 @@
credits_stop_music();
- Credit_text = "";
+ Credit_text_parts.clear();
if (Background_bitmap){
bm_release(Background_bitmap);
@@ -738,16 +752,29 @@
gr_set_clip(Credits_text_coords[gr_screen.res][CREDITS_X_COORD], Credits_text_coords[gr_screen.res][CREDITS_Y_COORD], Credits_text_coords[gr_screen.res][CREDITS_W_COORD], Credits_text_coords[gr_screen.res][CREDITS_H_COORD]);
gr_set_font(FONT1);
gr_set_color_fast(&Color_normal);
-
- int sy;
+
+ int sy; // The current position of the first text part
if ( Credit_position > 0 ) {
sy = fl2i(Credit_position+0.5f);
} else {
sy = fl2i(Credit_position-0.5f);
}
- gr_string(0x8000, sy, Credit_text.c_str());
+ for (SCP_vector<SCP_string>::iterator iter = Credit_text_parts.begin(); iter != Credit_text_parts.end(); ++iter)
+ {
+ int height;
+ gr_get_string_size(NULL, &height, iter->c_str(), iter->length());
+
+ // Check if the text part is actually visible
+ if (sy + height > 0)
+ {
+ gr_string(0x8000, sy, iter->c_str());
+ }
+
+ sy = sy + height;
+ }
+
int temp_time;
temp_time = timer_get_milliseconds();
Index: code/parse/parselo.cpp
===================================================================
--- code/parse/parselo.cpp (revision 9013)
+++ code/parse/parselo.cpp (working copy)
@@ -4055,7 +4055,7 @@
return num_files;
}
-bool parse_optional_float(const char *tag, float *destination, float def, float min, float max)
+bool parse_optional_float(const char *tag, float *destination, float min, float max)
{
Assert(tag != NULL);
Assert(destination != NULL);
@@ -4079,8 +4079,6 @@
}
else
{
- *destination = def;
-
return false;
}
}
Index: code/parse/parselo.h
===================================================================
--- code/parse/parselo.h (revision 9013)
+++ code/parse/parselo.h (working copy)
@@ -269,11 +269,10 @@
*
* @param tag The optional tag that should be checked
* @param destination The destination float where the value should be stored
- * @param def The default value which should be used when the tag isn't present
* @param min The minimum value which will be accepted. Optional parameter; defaults to @c FLT_MIN
* @param max The maximum value which will be accepted. Optional parameter; defaults to @c FLT_MAX
* @return
*/
-bool parse_optional_float(const char *tag, float *destination, float def, float min = FLT_MIN, float max = FLT_MAX);
+bool parse_optional_float(const char *tag, float *destination, float min = FLT_MIN, float max = FLT_MAX);
#endif
|
|
|
|
|
|
I updated the patch with another bugfix where values set in a table got overwritten with the default values when a table didn't contain the options. Also restored the sample table which I accidentally deleted... |
|
|
I confirm all issues are fixed with the latest patch. |
|
|
Why does the patch remove the ability to set a default value for the optional float? |
|
|
Because there is not way to know whether the variable has been set before so the value which got set before gets overwritten every time the function doesn't find the optional tag. |
|
|
I see what you mean. It also looks like whoever wrote parse_optional_float wasn't familiar with the parsing functions in general. I've rewritten that section slightly to use the conventional methods. I've also added better minimum values, because we don't want the credits to freeze or crash with a div-0 exception due to entering a value of 0. |
|
|
Marking as fixed due to testing, code review, and commit. |
|
fs2open: trunk r9017 2012-07-15 14:48 Ported: N/A Details Diff |
this is a slightly modified version of m!m's patch for Mantis 0002688 |
Affected Issues 0002688 |
|
| mod - /trunk/fs2_open/code/menuui/credits.cpp | Diff File | ||
| mod - /trunk/fs2_open/code/parse/parselo.cpp | Diff File | ||
| mod - /trunk/fs2_open/code/parse/parselo.h | Diff File | ||
| Date Modified | Username | Field | Change |
|---|---|---|---|
| 2012-07-14 17:06 | MjnMixael | New Issue | |
| 2012-07-14 17:06 | MjnMixael | File Added: fsu_credits-crd.tbm | |
| 2012-07-14 18:28 | m_m | Assigned To | => m_m |
| 2012-07-14 18:28 | m_m | Status | new => assigned |
| 2012-07-14 18:30 | m_m | Note Added: 0013859 | |
| 2012-07-14 18:30 | m_m | Status | assigned => code review |
| 2012-07-14 18:30 | m_m | File Added: credits.patch | |
| 2012-07-14 19:08 | MjnMixael | Note Added: 0013860 | |
| 2012-07-14 19:37 | MjnMixael | Note Edited: 0013860 | |
| 2012-07-14 20:14 | m_m | File Deleted: fsu_credits-crd.tbm | |
| 2012-07-14 20:14 | m_m | File Deleted: credits.patch | |
| 2012-07-14 20:14 | m_m | File Added: credits.patch | |
| 2012-07-14 20:15 | m_m | File Added: fsu_credits-crd.tbm | |
| 2012-07-14 20:27 | m_m | Note Added: 0013861 | |
| 2012-07-15 06:05 | MjnMixael | Note Added: 0013863 | |
| 2012-07-15 06:18 | Goober5000 | Note Added: 0013864 | |
| 2012-07-15 08:16 | m_m | Note Added: 0013865 | |
| 2012-07-15 18:45 | Goober5000 | Note Added: 0013867 | |
| 2012-07-15 18:45 | Goober5000 | Changeset attached | => fs2open trunk r9017 |
| 2012-07-15 18:46 | Goober5000 | Note Added: 0013868 | |
| 2012-07-15 18:46 | Goober5000 | Status | code review => resolved |
| 2012-07-15 18:46 | Goober5000 | Resolution | open => fixed |
