View Issue Details

IDProjectCategoryView StatusLast Update
0003069FSSCPtablespublic2014-07-19 13:09
ReporterMjnMixael Assigned ToMageKing17  
PrioritynormalSeveritycrashReproducibilityalways
Status resolvedResolutionfixed 
PlatformPCOSWindowsOS VersionWin7
Product Version3.7.2 
Target Version3.7.2 
Summary0003069: ASSERTION when help.tbl has errors
DescriptionThis should be an Error that is actually a little helpful.. handled in the same was as other tables' errors.
Steps To Reproducewrite +resolution instead of +resolutions in help.tbl.
TagsNo tags attached.

Activities

MageKing17

2014-06-29 05:05

developer  

contexthelp.cpp.patch (481 bytes)   
Index: code/gamehelp/contexthelp.cpp
===================================================================
--- code/gamehelp/contexthelp.cpp	(revision 10846)
+++ code/gamehelp/contexthelp.cpp	(working copy)
@@ -482,7 +482,7 @@
 
 			} else {
 				// help.tbl is corrupt
-				Assert(0);
+				Error(LOCATION, "help.tbl appears to be corrupt; expected [+pline], [+text], [+right_bracket], [+left_bracket], or [$end], found [%.32s] instead.\n", next_tokens());
 
 			}		// end if
 
contexthelp.cpp.patch (481 bytes)   

MageKing17

2014-06-29 05:06

developer  

contexthelp2.patch (6,175 bytes)   
Index: code/gamehelp/contexthelp.cpp
===================================================================
--- code/gamehelp/contexthelp.cpp	(revision 10855)
+++ code/gamehelp/contexthelp.cpp	(working copy)
@@ -411,11 +411,13 @@
 			help_overlaylist[overlay_id].lbracketlist.push_back(lbracket_temp);
 		}
 		
+		int type;
 		// read in all elements for this overlay
-		while (!(optional_string("$end")))  {
+		while ((type = required_string_5("+pline", "+text", "+right_bracket", "+left_bracket", "$end")) != 4) {	// Doing it this way means an error lists "$end" at the end, which seems appropriate. -MageKing17
 
-			if (optional_string("+pline")) {
-
+			switch (type) {
+			case 0:	// +pline
+				required_string("+pline");
 				currcount = help_overlaylist[overlay_id].plinecount;
 				int a, b;		// temp vars to read in int before cast to float;
 
@@ -422,9 +424,9 @@
 				// read number of pline vertices
 				stuff_int(&vtxcount);
 				// get vertex coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].plinelist.at(i).push_back(pline_temp2);
-					for (j=0; j<vtxcount; j++) {
+					for (j = 0; j < vtxcount; j++) {
 						help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtx.push_back(vec3d_temp);
 						help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtxcount = vtxcount;
 						stuff_int(&a);
@@ -436,13 +438,13 @@
 				}
 
 				help_overlaylist[overlay_id].plinecount++;
-
-			} else if (optional_string("+text")) {
-
+				break;
+			case 1:	// +text
+				required_string("+text");
 				currcount = help_overlaylist[overlay_id].textcount;
 
 				// get coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].textlist.at(i).push_back(text_temp2);
 					stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).x_coord));
 					stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).y_coord));
@@ -453,13 +455,13 @@
 				help_overlaylist[overlay_id].textlist.at(0).at(currcount).string = vm_strdup(buf);
 
 				help_overlaylist[overlay_id].textcount++;
-
-			} else if (optional_string("+right_bracket")) {
-
+				break;
+			case 2: // +right_bracket
+				required_string("+right_bracket");
 				currcount = help_overlaylist[overlay_id].rbracketcount;
 
 				// get coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].rbracketlist.at(i).push_back(rbracket_temp2);
 					stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).x_coord));
 					stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).y_coord));
@@ -466,13 +468,13 @@
 				}
 
 				help_overlaylist[overlay_id].rbracketcount++;
-
-			} else if (optional_string("+left_bracket")) {
-
+				break;
+			case 3: // +left_bracket
+				required_string("+left_bracket");
 				currcount = help_overlaylist[overlay_id].lbracketcount;
 
 				// get coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].lbracketlist.at(i).push_back(lbracket_temp2);
 					stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).x_coord));
 					stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).y_coord));
@@ -479,14 +481,17 @@
 				}
 
 				help_overlaylist[overlay_id].lbracketcount++;
-
-			} else {
-				// help.tbl is corrupt
-				Assert(0);
-
-			}		// end if
-
+				break;
+			case -1:
+				// -noparseerrors is set
+				break;
+			case 4: // $end
+			default:
+				Assertion(false, "This should never happen.\n");
+				break;
+			}
 		}		// end while
+		required_string("$end");
 	}		// end while
 
 	// close localization
Index: code/parse/parselo.cpp
===================================================================
--- code/parse/parselo.cpp	(revision 10855)
+++ code/parse/parselo.cpp	(working copy)
@@ -740,6 +740,42 @@
 	return -1;
 }
 
+int required_string_5(char *str1, char *str2, char *str3, char *str4, char *str5)
+{
+	int	count = 0;
+	
+	ignore_white_space();
+	
+	while (count < RS_MAX_TRIES) {
+		if (strnicmp(str1, Mp, strlen(str1)) == 0) {
+			// Mp += strlen(str1);
+			diag_printf("Found required string [%s]\n", token_found = str1);
+			return 0;
+		} else if (strnicmp(str2, Mp, strlen(str2)) == 0) {
+			// Mp += strlen(str2);
+			diag_printf("Found required string [%s]\n", token_found = str2);
+			return 1;
+		} else if (strnicmp(str3, Mp, strlen(str3)) == 0) {
+			diag_printf("Found required string [%s]\n", token_found = str3);
+			return 2;
+		} else if (strnicmp(str4, Mp, strlen(str4)) == 0) {
+			diag_printf("Found required string [%s]\n", token_found = str4);
+			return 3;
+		} else if (strnicmp(str5, Mp, strlen(str5)) == 0) {
+			diag_printf("Found required string [%s]\n", token_found = str5);
+			return 4;
+		}
+		
+		error_display(1, "Required token = [%s], [%s], [%s], [%s], or [%s], found [%.32s].\n", str1, str2, str3, str4, str5, next_tokens());
+		
+		advance_to_eoln(NULL);
+		ignore_white_space();
+		count++;
+	}
+	
+	return -1;
+}
+
 int required_string_either_fred(char *str1, char *str2)
 {
 	ignore_white_space();
Index: code/parse/parselo.h
===================================================================
--- code/parse/parselo.h	(revision 10855)
+++ code/parse/parselo.h	(working copy)
@@ -113,6 +113,7 @@
 extern int required_string_either(char *str1, char *str2);
 extern int required_string_3(char *str1, char *str2, char *str3);
 extern int required_string_4(char *str1, char *str2, char *str3, char *str4);
+extern int required_string_5(char *str1, char *str2, char *str3, char *str4, char *str5);
 
 // stuff
 extern void copy_to_eoln(char *outstr, char *more_terminators, char *instr, int max);
contexthelp2.patch (6,175 bytes)   

MageKing17

2014-06-29 05:11

developer   ~0015932

Attached three patches with varying levels of impact.

The first patch just changes "Assert(0)" into a descriptive Error().

The second patch rewrites the logic, adding required_string_5() to match required_string_3() and required_string_4().

The third patch is the same as the second, but instead of the silly required_string_5(), it uses a new required_string_any_of() with a variable number of arguments that can be used as a drop-in replacement for required_string_3() or required_string_4() in the future. Most of it was written by ngld during a conversation on #scp as we were discussing this bug.

MageKing17

2014-06-29 17:57

developer  

contexthelp3.patch (6,320 bytes)   
Index: code/gamehelp/contexthelp.cpp
===================================================================
--- code/gamehelp/contexthelp.cpp	(revision 10856)
+++ code/gamehelp/contexthelp.cpp	(working copy)
@@ -411,11 +411,13 @@
 			help_overlaylist[overlay_id].lbracketlist.push_back(lbracket_temp);
 		}
 		
+		int type;
 		// read in all elements for this overlay
-		while (!(optional_string("$end")))  {
+		while ((type = required_string_one_of(5, "+pline", "+text", "+right_bracket", "+left_bracket", "$end")) != 4) {	// Doing it this way means an error lists "$end" at the end, which seems appropriate. -MageKing17
 
-			if (optional_string("+pline")) {
-
+			switch (type) {
+			case 0:	// +pline
+				required_string("+pline");
 				currcount = help_overlaylist[overlay_id].plinecount;
 				int a, b;		// temp vars to read in int before cast to float;
 
@@ -422,9 +424,9 @@
 				// read number of pline vertices
 				stuff_int(&vtxcount);
 				// get vertex coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].plinelist.at(i).push_back(pline_temp2);
-					for (j=0; j<vtxcount; j++) {
+					for (j = 0; j < vtxcount; j++) {
 						help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtx.push_back(vec3d_temp);
 						help_overlaylist[overlay_id].plinelist.at(i).at(currcount).vtxcount = vtxcount;
 						stuff_int(&a);
@@ -436,13 +438,13 @@
 				}
 
 				help_overlaylist[overlay_id].plinecount++;
-
-			} else if (optional_string("+text")) {
-
+				break;
+			case 1:	// +text
+				required_string("+text");
 				currcount = help_overlaylist[overlay_id].textcount;
 
 				// get coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].textlist.at(i).push_back(text_temp2);
 					stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).x_coord));
 					stuff_int(&(help_overlaylist[overlay_id].textlist.at(i).at(currcount).y_coord));
@@ -453,13 +455,13 @@
 				help_overlaylist[overlay_id].textlist.at(0).at(currcount).string = vm_strdup(buf);
 
 				help_overlaylist[overlay_id].textcount++;
-
-			} else if (optional_string("+right_bracket")) {
-
+				break;
+			case 2: // +right_bracket
+				required_string("+right_bracket");
 				currcount = help_overlaylist[overlay_id].rbracketcount;
 
 				// get coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].rbracketlist.at(i).push_back(rbracket_temp2);
 					stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).x_coord));
 					stuff_int(&(help_overlaylist[overlay_id].rbracketlist.at(i).at(currcount).y_coord));
@@ -466,13 +468,13 @@
 				}
 
 				help_overlaylist[overlay_id].rbracketcount++;
-
-			} else if (optional_string("+left_bracket")) {
-
+				break;
+			case 3: // +left_bracket
+				required_string("+left_bracket");
 				currcount = help_overlaylist[overlay_id].lbracketcount;
 
 				// get coordinates for each resolution
-				for (i=0; i<help_overlaylist[overlay_id].num_resolutions; i++) {
+				for (i = 0; i < help_overlaylist[overlay_id].num_resolutions; i++) {
 					help_overlaylist[overlay_id].lbracketlist.at(i).push_back(lbracket_temp2);
 					stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).x_coord));
 					stuff_int(&(help_overlaylist[overlay_id].lbracketlist.at(i).at(currcount).y_coord));
@@ -479,14 +481,17 @@
 				}
 
 				help_overlaylist[overlay_id].lbracketcount++;
-
-			} else {
-				// help.tbl is corrupt
-				Assert(0);
-
-			}		// end if
-
+				break;
+			case -1:
+				// -noparseerrors is set
+				break;
+			case 4: // $end
+			default:
+				Assertion(false, "This should never happen.\n");
+				break;
+			}
 		}		// end while
+		required_string("$end");
 	}		// end while
 
 	// close localization
Index: code/parse/parselo.cpp
===================================================================
--- code/parse/parselo.cpp	(revision 10856)
+++ code/parse/parselo.cpp	(working copy)
@@ -740,6 +740,56 @@
 	return -1;
 }
 
+// Generic version of old required_string_3 and required_string_4; written by ngld, with some tweaks by MageKing17
+int required_string_one_of(int arg_count, ...)
+{
+	Assertion(arg_count > 0, "required_string_one_of() called with arg_count of %d; get a coder!\n", arg_count);
+	int count = 0;
+	int idx;
+	char *expected;
+	SCP_string message = "";
+	va_list vl;
+
+	ignore_white_space();
+
+	while (count < RS_MAX_TRIES) {
+		va_start(vl, arg_count);
+		for (idx = 0; idx < arg_count; idx++) {
+			expected = va_arg(vl, char*);
+			if (strnicmp(expected, Mp, strlen(expected)) == 0) {
+				diag_printf("Found required string [%s]", token_found = expected);
+				return idx;
+			}
+		}
+		va_end(vl);
+
+		if (!message.compare("")) {
+			va_start(vl, arg_count);
+			message = "Required token = ";
+			for (idx = 0; idx < arg_count; idx++) {
+				message += "[";
+				message += va_arg(vl, char*);
+				message += "]";
+				if (arg_count == 2 && idx == 0) {
+					message += " or ";
+				} else if (idx == arg_count - 2) {
+					message += ", or ";
+				} else if (idx < arg_count - 2) {
+					message += ", ";
+				}
+			}
+			va_end(vl);
+		}
+
+		error_display(1, "%s, found [%.32s]\n", message.c_str(), next_tokens());
+		advance_to_eoln(NULL);
+		ignore_white_space();
+		count++;
+	}
+
+	return -1;
+}
+
 int required_string_either_fred(char *str1, char *str2)
 {
 	ignore_white_space();
Index: code/parse/parselo.h
===================================================================
--- code/parse/parselo.h	(revision 10856)
+++ code/parse/parselo.h	(working copy)
@@ -113,6 +113,7 @@
 extern int required_string_either(char *str1, char *str2);
 extern int required_string_3(char *str1, char *str2, char *str3);
 extern int required_string_4(char *str1, char *str2, char *str3, char *str4);
+extern int required_string_one_of(int arg_count, ...);
 
 // stuff
 extern void copy_to_eoln(char *outstr, char *more_terminators, char *instr, int max);
contexthelp3.patch (6,320 bytes)   

Goober5000

2014-06-29 22:58

administrator   ~0015936

In the third patch, the message variable is not assigned anything, so the "if (!message.compare(""))" block will never trigger.

MageKing17

2014-06-29 23:03

developer   ~0015937

It's declared with 'SCP_string message = "";' I did test all three patches, you know. ;)

Goober5000

2014-06-29 23:07

administrator   ~0015938

What I mean is that it isn't modified after its initialization. It's a logic error, not a crash. :)

MageKing17

2014-06-29 23:12

developer   ~0015940

If it wasn't modified, my test wouldn't have worked. '!message.compare("")' is checking to see if message is not different from "", which it isn't at first, so the block executes.

Goober5000

2014-06-30 02:59

administrator   ~0015943

Ack. Never mind what I said; I forgot that .compare() works like strcmp() and returns a 0 if the strings match. >.<

The_E

2014-07-19 13:09

administrator   ~0016095

Fix committed in rev 10925

Issue History

Date Modified Username Field Change
2014-06-29 02:30 MjnMixael New Issue
2014-06-29 02:30 MjnMixael Status new => assigned
2014-06-29 02:30 MjnMixael Assigned To => MageKing17
2014-06-29 05:05 MageKing17 File Added: contexthelp.cpp.patch
2014-06-29 05:06 MageKing17 File Added: contexthelp2.patch
2014-06-29 05:06 MageKing17 File Added: contexthelp3.patch
2014-06-29 05:11 MageKing17 Note Added: 0015932
2014-06-29 05:11 MageKing17 Status assigned => code review
2014-06-29 16:47 MageKing17 File Deleted: contexthelp3.patch
2014-06-29 16:47 MageKing17 File Added: contexthelp3.patch
2014-06-29 17:42 MageKing17 File Deleted: contexthelp3.patch
2014-06-29 17:42 MageKing17 File Added: contexthelp3.patch
2014-06-29 17:55 MageKing17 File Deleted: contexthelp3.patch
2014-06-29 17:56 MageKing17 File Added: contexthelp3.patch
2014-06-29 17:57 MageKing17 File Deleted: contexthelp3.patch
2014-06-29 17:57 MageKing17 File Added: contexthelp3.patch
2014-06-29 22:58 Goober5000 Note Added: 0015936
2014-06-29 23:03 MageKing17 Note Added: 0015937
2014-06-29 23:07 Goober5000 Note Added: 0015938
2014-06-29 23:12 MageKing17 Note Added: 0015940
2014-06-30 02:59 Goober5000 Note Added: 0015943
2014-07-19 13:09 The_E Note Added: 0016095
2014-07-19 13:09 The_E Status code review => resolved
2014-07-19 13:09 The_E Resolution open => fixed