Make cfread_string_len read no more than fits in the buffer

code/cfile/cfile.cpp

@@ -33,7 +33,7 @@
 #include "parse/encrypt.h"
 #include "cfilesystem.h"
 
-
+#include <algorithm>
 #include <limits>
 
 char Cfile_root_dir[CFILE_ROOT_DIRECTORY_LEN] = "";
@@ -1105,28 +1105,90 @@ void cfread_string(char *buf, int n, CFILE *file)
 	} while (c != 0 );
 }
 
-void cfread_string_len(char *buf, int buffer_size, CFILE *file)
+struct string_len_header {
+	int len = 0;
+	int stored = 0;
+	int start = 0;
+	bool length_cut = false;
+};
+
+// cfseek ignores the read limit, so it is checked here as cfread would have; inside a limited section any length
+// that does not fit the file is corrupt and throws, so the loader drops the section
+static string_len_header read_string_len_header(CFILE* file)
+{
+	string_len_header h;
+	h.length_cut = cfread(&h.len, sizeof(h.len), 1, file) != 1;
+	h.len = INTEL_INT(h.len);
+	h.start = cftell(file);
+	int remaining = std::max(cfilelength(file) - h.start, 0);
+	if ((file->max_read_len != 0) &&
+		(h.length_cut || (h.len < 0) || (file->raw_position + h.len > file->max_read_len))) {
+		throw cfile::max_read_length(
+			"Attempted to read a string of length " + std::to_string(h.len) + " beyond length limit");
+	}
+	h.stored = h.length_cut ? 0 : std::clamp(h.len, 0, remaining);
+	return h;
+}
+
+// reads count of the stored characters and leaves the file just past the whole string; false if the read failed
+static bool read_string_len_body(const string_len_header& h, char* dest, int count, CFILE* file)
 {
-	int len;
-	len = cfread_int(file);
-	Assertion( (len <= buffer_size), "String cannot fit in buffer, len: %i, buffer: %i", len, buffer_size);
-	if (len) {
-		cfread(buf, len, 1, file);
-		Assertion(len != buffer_size || buf[len - 1] == 0, "Unterminated string does not fit in buffer");
+	bool ok = (count <= 0) || (cfread(dest, count, 1, file) == 1);
+	if (!ok || (h.stored > count)) {
+		cfseek(file, h.start + h.stored, CF_SEEK_SET);
+	}
+	return ok;
+}
+
+// warned last, so the file position is right even if a debug build stops here
+static void warn_string_len(const string_len_header& h, bool read_ok, CFILE* file)
+{
+	if (h.length_cut) {
+		Warning(LOCATION, "cfread_string_len: cut-off string length field in %s", cf_get_filename(file));
+	} else if (!read_ok) {
+		Warning(LOCATION,
+			"cfread_string_len: could not read a string of %d characters in %s",
+			h.stored,
+			cf_get_filename(file));
+	} else if (h.len != h.stored) {
+		Warning(LOCATION, "cfread_string_len: corrupt string length %d in %s", h.len, cf_get_filename(file));
 	}
+}
 
-	buf[MIN(len, buffer_size - 1)] = 0;
+void cfread_string_len(char* buf, int buffer_size, CFILE* file)
+{
+	Assertion(buffer_size > 0, "cfread_string_len needs room for at least the terminator, buffer: %i", buffer_size);
+	string_len_header h = read_string_len_header(file);
+	int count = std::min(h.stored, std::max(buffer_size, 0));
+	bool read_ok = read_string_len_body(h, buf, count, file);
+	if (buffer_size > 0) {
+		int kept = read_ok ? count : 0;
+		bool cut = (kept == buffer_size) && (memchr(buf, 0, buffer_size) == nullptr);
+		buf[std::min(kept, buffer_size - 1)] = 0;
+		warn_string_len(h, read_ok, file);
+		if (read_ok && (h.len == h.stored) && cut) {
+			Warning(LOCATION,
+				"cfread_string_len: a string of %d characters was cut to fit a buffer of %d in %s",
+				h.len,
+				buffer_size,
+				cf_get_filename(file));
+		}
+		return;
+	}
+	warn_string_len(h, read_ok, file);
 }
 
 SCP_string cfread_string_len(CFILE *file)
 {
-	int len = cfread_int(file);
+	string_len_header h = read_string_len_header(file);
 
 	SCP_string str;
-	str.resize(len);
-
-	if (len)
-		cfread(&str[0], len, 1, file);
+	str.resize(h.stored);
+	bool read_ok = read_string_len_body(h, str.data(), h.stored, file);
+	if (!read_ok) {
+		str.clear();
+	}
+	warn_string_len(h, read_ok, file);
 
 	return str;
 }

code/cfile/cfile.h

@@ -320,8 +320,11 @@ void cfread_string(char *buf,int n, CFILE *file);
 /**
  * @brief Read a fixed length string that is not null-terminated, with the length stored in file.
  *
- * If it is not null-terminated, the length of the string *must* be less than the size of the buffer (to make room for one)
- * 
+ * Keeps at most buffer_size - 1 characters and skips the rest of the string in the file, so the next read
+ * starts after it. A string whose text does not fit, a negative length, a length past the end of the file, a cut-off
+ * length field, or a failed read gets a warning. Inside a limited section (cf_set_max_read_len) a negative length or
+ * one past the limit throws cfile::max_read_length instead.
+ *
  * @param buf Pre-allocated array to store string
  * @param buffer_size Size of pre-allocated array
  * @param file File to read from
@@ -332,6 +335,11 @@ void cfread_string_len(char *buf,int buffer_size, CFILE *file);
 
 /**
  * @brief Read a string from the file where the length is stored in the file
+ *
+ * A negative length reads nothing, and a length past the end of the file reads only what is left; both get a warning,
+ * as do a cut-off length field and a failed read. Inside a limited section (cf_set_max_read_len) a negative length or
+ * one past the limit throws cfile::max_read_length instead.
+ *
  * @param file The file to read the string from
  * @return The string that was read
  */