P1: Fix various crash potentials in table and mission parsing

code/mission/missionparse.cpp

@@ -3431,7 +3431,7 @@ int parse_object(mission *pm, int  /*flag*/, p_object *p_objp)
 	while (optional_string("$Alt Ship Class:")) {	
 		alt_class new_alt_class; 
 
-		is_variable = get_string_or_variable(alt_ship_class); 
+		is_variable = get_string_or_variable(alt_ship_class, static_cast<int>(sizeof(alt_ship_class))); 
 
 		if (is_variable) {
 			new_alt_class.variable_index = get_index_sexp_variable_name(alt_ship_class);

code/missionui/missionweaponchoice.cpp

@@ -2499,6 +2499,7 @@ void wl_render_weapon_desc(float frametime)
 		gr_string(weapon_desc_coords[0], weapon_desc_coords[1] + line_height, Weapon_desc_lines[3], GR_RESIZE_MENU);
 		gr_string(weapon_desc_coords[0], weapon_desc_coords[1] + line_height * 2, Weapon_desc_lines[4], GR_RESIZE_MENU);
 		gr_string(weapon_desc_coords[0], weapon_desc_coords[1] + line_height * 3, Weapon_desc_lines[5], GR_RESIZE_MENU);
+		gr_string(weapon_desc_coords[0], weapon_desc_coords[1] + line_height * 4, Weapon_desc_lines[6], GR_RESIZE_MENU);
 	}
 }
 
@@ -2524,16 +2525,16 @@ void wl_weapon_desc_start_wipe()
 	if (w > Weapon_title_max_width[gr_screen.res]) {
 		// split
 		currchar_src = (size_t)(((float)title_len / (float)w) * Weapon_title_max_width[gr_screen.res]);			// char to start space search at
-		while (Weapon_desc_lines[0][currchar_src] != ' ') {
+		while ((currchar_src > 0) && (Weapon_desc_lines[0][currchar_src] != ' ')) {
 			currchar_src--;
-			if (currchar_src <= 0) {
-				currchar_src = title_len;
-				break;
-			}
 		}
 
-		Weapon_desc_lines[0][currchar_src] = '\0';										// shorten line 0
-		strcpy_s(Weapon_desc_lines[1], &(Weapon_desc_lines[0][currchar_src+1]));		// copy remainder into line 1
+		if (currchar_src > 0) {
+			Weapon_desc_lines[0][currchar_src] = '\0';										// shorten line 0
+			strcpy_s(Weapon_desc_lines[1], &(Weapon_desc_lines[0][currchar_src+1]));		// copy remainder into line 1
+		} else {
+			Weapon_desc_lines[1][0] = '\0';
+		}
 	} else {
 		// entire title in line 0, thus line 1 is empty
 		Weapon_desc_lines[1][0] = '\0';
@@ -2548,10 +2549,13 @@ void wl_weapon_desc_start_wipe()
 				// break here
 				if (currchar_src != 0) {					// protect against leading /n's
 					Weapon_desc_lines[currline_dest][currchar_dest] = '\0';
+					if (currline_dest + 1 >= WEAPON_DESC_MAX_LINES) {
+						break;
+					}
 					currline_dest++;
 					currchar_dest = 0;
 				}
-			} else {
+			} else if (currchar_dest < WEAPON_DESC_MAX_LENGTH - 1) {
 				// straight copy
 				Weapon_desc_lines[currline_dest][currchar_dest] = desc[currchar_src];
 				currchar_dest++;

code/parse/parselo.cpp

@@ -1215,7 +1215,7 @@ char* alloc_block(const char* startstr, const char* endstr, int extra_chars)
 
 // Karajorma - Stuffs the provided char array with either the contents of a quoted string or the name of a string
 // variable. Returns PARSING_FOUND_STRING if a string was found or PARSING_FOUND_VARIABLE if a variable was present.
-int get_string_or_variable (char *str)
+int get_string_or_variable (char *str, int max)
 {
 	int result = -1;
 
@@ -1226,7 +1226,7 @@ int get_string_or_variable (char *str)
 	{
 		auto saved_Mp = Mp;
 		Mp++;
-		stuff_string_white(str);
+		stuff_string_white(str, max - 1);
 		int sexp_variable_index = get_index_sexp_variable_name(str);
 
 		// We only want String variables
@@ -1235,19 +1235,19 @@ int get_string_or_variable (char *str)
 		else
 		{
 			Mp = saved_Mp;
-			stuff_string_white(str);
+			stuff_string_white(str, max - 1);
 			error_display(1, "Expected \"%s\" to be a variable", str);
 		}
 	}
 	// Quoted string
 	else if (*Mp == '"')
 	{
-		get_string(str);
+		get_string(str, max);
 		result =  PARSING_FOUND_STRING;
 	}
 	else
 	{
-		get_string(str);
+		get_string(str, max);
 		error_display(1, "Invalid entry \"%s\" found in get_string_or_variable. Must be a quoted string or a string variable name.", str);
 	}
 
@@ -1300,27 +1300,46 @@ int get_string_or_variable (SCP_string &str)
  */
 int get_string(char *str, int max)
 {
+	if (*Mp == '\0') {
+		if (max != 0)
+			str[0] = '\0';
+		return 0;
+	}
+
 	auto len = strcspn(Mp + 1, "\"");
 
-	if (max >= 0 && len >= (size_t)max)
-		error_display(0, "String too long.  Length = " SIZE_T_ARG ".  Max is %i.\n", len, max);
+	auto copy_len = len;
+	if (max >= 0 && len >= (size_t)max) {
+		error_display(0, "String too long.  Length = " SIZE_T_ARG ".  Max is %i.\n", len, max - 1);
+		copy_len = (max > 0) ? (size_t)(max - 1) : 0;
+	}
 
-	strncpy(str, Mp + 1, len);
-	str[len] = 0;
+	strncpy(str, Mp + 1, copy_len);
+	str[copy_len] = 0;
 
-	Mp += len + 2;
-	return (int)len;
+	// step over the closing quote only if there is one
+	Mp += len + 1;
+	if (*Mp == '"')
+		Mp++;
+	return (int)copy_len;
 }
 
 /**
  * Stuff a string (" chars ") into str.
  */
 void get_string(SCP_string &str)
 {
+	if (*Mp == '\0') {
+		str.clear();
+		return;
+	}
+
 	auto len = strcspn(Mp + 1, "\"");
 	str.assign(Mp + 1, len);
 
-	Mp += len + 2;
+	Mp += len + 1;
+	if (*Mp == '"')
+		Mp++;
 }
 
 //	Stuff a string into a string buffer.
@@ -2067,13 +2086,16 @@ bool matches_version_specific_tag(const char *line_start, bool &compatible_versi
 		s_num = 3;
 	}
 
+	// sscanf counts conversions, not the trailing literal, so the closing ";;" may be missing
 	const char *ch = line_start + 6;
 	while ((*ch) != ';') {
-		Assertion((*ch) != '\0', "String that was already guaranteed to end with semicolons did not end with semicolons; it's possible we have fallen into an alternate universe. Failing string: [%s]\n", line_start);
+		if ((*ch == '\0') || (*ch == '\n') || (*ch == '\r'))
+			return false;
 		ch++;
 	}
 	ch++;
-	Assertion((*ch) == ';', "String that was guaranteed to have double semicolons did not; it's possible we have fallen into an alternate universe. Failing string: [%s]\n", line_start);
+	if ((*ch) != ';')
+		return false;
 	ch++;
 
 	tag_len = (int)(ch - line_start);

code/parse/parselo.h

@@ -452,7 +452,7 @@ struct loadout_row
 
 //Karajorma/Goober5000 - Parses mission and campaign ship loadouts.
 void stuff_loadout_list(SCP_vector<loadout_row> &list, ParseLookupType lookup_type);
-int get_string_or_variable (char *str);
+int get_string_or_variable (char *str, int max);
 int get_string_or_variable (SCP_string &str);
 #define PARSING_FOUND_STRING		0
 #define PARSING_FOUND_VARIABLE		1

code/parse/sexp.cpp

@@ -5057,22 +5057,27 @@ int stuff_sexp_variable_list()
 	ignore_white_space();
 
 	while (*Mp != ')') {
+		if (*Mp == '\0') {
+			error_display(1, "Reading sexp variable list.  Reached the end of the file before the closing ')'.\n");
+			throw parse::ParseException("Syntax error");
+		}
 		Assert(count < MAX_SEXP_VARIABLES);
 
 		// get index - for debug
+		index = -1;
 		stuff_int(&index);
 		ignore_gray_space();
 
 		// get var_name
-		get_string(var_name);
+		get_string(var_name, TOKEN_LENGTH);
 		ignore_gray_space();
 
 		// get default_value;
-		get_string(default_value);
+		get_string(default_value, TOKEN_LENGTH);
 		ignore_gray_space();
 
 		// get type
-		get_string(str_type);
+		get_string(str_type, TOKEN_LENGTH);
 		ignore_white_space();
 
 		// determine type
@@ -5094,7 +5099,7 @@ int stuff_sexp_variable_list()
 		// possibly get network-variable
 		if (check_for_string("\"network-variable\"")) {
 			// eat it
-			get_string(network);
+			get_string(network, TOKEN_LENGTH);
 			ignore_white_space();
 
 			// set type
@@ -5104,7 +5109,7 @@ int stuff_sexp_variable_list()
 		// maybe this is an eternal persistent variable of some type
 		if (check_for_string("\"eternal\"")) {
 			// eat it
-			get_string(persistent);
+			get_string(persistent, TOKEN_LENGTH);
 			ignore_white_space();
 
 			// set type
@@ -5114,40 +5119,54 @@ int stuff_sexp_variable_list()
 		// maybe this is a persistent variable of some type
 		if (check_for_string("\"player-persistent\"") || check_for_string("\"save-on-mission-close\"")) {
 			// eat it
-			get_string(persistent);
+			get_string(persistent, TOKEN_LENGTH);
 			ignore_white_space();
 
 			// set type
 			type |= SEXP_VARIABLE_SAVE_ON_MISSION_CLOSE;
 		} else if (check_for_string("\"campaign-persistent\"") || check_for_string("\"save-on-mission-progress\"")) {
 			// eat it
-			get_string(persistent);
+			get_string(persistent, TOKEN_LENGTH);
 			ignore_white_space();
 
 			// set type
 			type |= SEXP_VARIABLE_SAVE_ON_MISSION_PROGRESS;
 		// trap error
 		} else if (check_for_string("\"")) {
 			// eat garbage
-			get_string(persistent);
+			get_string(persistent, TOKEN_LENGTH);
 			ignore_white_space();
 
 			// notify of error
 			error_display(0, "Error parsing sexp variables - unknown persistence type encountered.  You can continue from here without trouble.");
 		}
 
 		// finished getting info; now add the variable
+		bool index_valid = (index >= 0) && (index < MAX_SEXP_VARIABLES);
 		if ( type & SEXP_VARIABLE_BLOCK ) {
-			add_block_variable(default_value, var_name, type, index);
+			if (index_valid) {
+				add_block_variable(default_value, var_name, type, index);
+			} else {
+				error_display(0, "Block variable '%s' has an invalid index (%d)!  Skipping it.", var_name, index);
+			}
 		}
 		else {
 			// check if variable name already exists
 			// (block variables can be duplicated, but regular ones can't)
 			if (get_index_sexp_variable_name(var_name) >= 0) {
 				error_display(0, "Variable '%s' already exists!  Skipping duplicate.", var_name);
 			} else {
-				count++;
-				sexp_add_variable(default_value, var_name, type, index);
+				if (!index_valid && (index != -1)) {
+					error_display(0, "Variable '%s' has an invalid index (%d)!  Using the next free slot.", var_name, index);
+					index = -1;
+				}
+				// the caller scans slots 0 through count - 1, so count is one past the highest slot used
+				int slot = sexp_add_variable(default_value, var_name, type, index);
+				if (slot >= 0) {
+					count = MAX(count, slot + 1);
+				} else {
+					error_display(0, "Too many variables!  Skipping '%s'.", var_name);
+				}
 			}
 		}
 	}

code/weapon/weapons.cpp

@@ -925,6 +925,9 @@ int parse_weapon(int subtype, bool replace, const char *filename)
 
 		// Check if the text exceeds the limits
 		auto current_line = wip->desc.get();
+		if ((current_line != nullptr) && (*current_line == '\n')) {
+			++current_line;
+		}
 		size_t num_lines = 0;
 		while (current_line != nullptr) {
 			auto line_end = strchr(current_line, '\n');
@@ -933,15 +936,20 @@ int parse_weapon(int subtype, bool replace, const char *filename)
 				line_length = strlen(current_line);
 			}
 
+			if ((line_end == nullptr) && (line_length == 0) && (num_lines > 0)) {
+				break;
+			}
+
 			if (line_length >= WEAPON_DESC_MAX_LENGTH) {
 				error_display(0, "Weapon description line " SIZE_T_ARG " is too long. Maximum is %d.", num_lines + 1, WEAPON_DESC_MAX_LENGTH);
 			}
 
 			++num_lines;
 			current_line = line_end == nullptr ? nullptr : line_end + 1; // Skip the \n character if it was a complete line
 		}
-		if (num_lines >= WEAPON_DESC_MAX_LINES) {
-			error_display(0, "Weapon description has too many lines. Maximum is %d.", WEAPON_DESC_MAX_LINES);
+		// the first two lines of the loadout description box hold the title
+		if (num_lines > WEAPON_DESC_MAX_LINES - 2) {
+			error_display(0, "Weapon description has too many lines. Maximum is %d.", WEAPON_DESC_MAX_LINES - 2);
 		}
 	}