2018-08-20 07:22 EDT


View Issue Details Jump to Notes ] Related Changesets ]
IDProjectCategoryView StatusLast Update
0002688FSSCPmodular tablespublic2012-07-15 14:46
ReporterMjnMixael 
Assigned Tom_m 
PrioritynormalSeveritycrashReproducibilityalways
StatusresolvedResolutionfixed 
Product Version3.6.13 
Target VersionFixed in Version 
Summary0002688: Long credits causes stack overflow crash.
DescriptionIf the total length of credits.tbl + code credits + credits.tbms is too long, FSO will crash when you enter the credits viewer.
Steps To ReproducePlace the attached tbm into data/tables and enter the credits viewer.
Additional InformationFrom 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
TagsNo tags attached.
Attached Files
  • patch file icon credits.patch (8,708 bytes) 2012-07-14 16:14 -
    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
    
    patch file icon credits.patch (8,708 bytes) 2012-07-14 16:14 +
  • ? file icon fsu_credits-crd.tbm (7,780 bytes) 2012-07-14 16:15

-Relationships
+Relationships

-Notes

~0013859

m_m (developer)

I added a patch which implements the suggested behavior and it seems to work as expected and without any crashes.

~0013860

MjnMixael (manager)

Last edited: 2012-07-14 15:37

View 2 revisions

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:

~0013861

m_m (developer)

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...

~0013863

MjnMixael (manager)

I confirm all issues are fixed with the latest patch.

~0013864

Goober5000 (administrator)

Why does the patch remove the ability to set a default value for the optional float?

~0013865

m_m (developer)

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.

~0013867

Goober5000 (administrator)

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.

~0013868

Goober5000 (administrator)

Marking as fixed due to testing, code review, and commit.
+Notes

+Related Changesets

-Issue History
Date Modified Username Field Change
2012-07-14 13:06 MjnMixael New Issue
2012-07-14 13:06 MjnMixael File Added: fsu_credits-crd.tbm
2012-07-14 14:28 m_m Assigned To => m_m
2012-07-14 14:28 m_m Status new => assigned
2012-07-14 14:30 m_m Note Added: 0013859
2012-07-14 14:30 m_m Status assigned => code review
2012-07-14 14:30 m_m File Added: credits.patch
2012-07-14 15:08 MjnMixael Note Added: 0013860
2012-07-14 15:37 MjnMixael Note Edited: 0013860 View Revisions
2012-07-14 16:14 m_m File Deleted: fsu_credits-crd.tbm
2012-07-14 16:14 m_m File Deleted: credits.patch
2012-07-14 16:14 m_m File Added: credits.patch
2012-07-14 16:15 m_m File Added: fsu_credits-crd.tbm
2012-07-14 16:27 m_m Note Added: 0013861
2012-07-15 02:05 MjnMixael Note Added: 0013863
2012-07-15 02:18 Goober5000 Note Added: 0013864
2012-07-15 04:16 m_m Note Added: 0013865
2012-07-15 14:45 Goober5000 Note Added: 0013867
2012-07-15 14:45 Goober5000 Changeset attached => fs2open trunk r9017
2012-07-15 14:46 Goober5000 Note Added: 0013868
2012-07-15 14:46 Goober5000 Status code review => resolved
2012-07-15 14:46 Goober5000 Resolution open => fixed
+Issue History