P4: Make cfread_string_len read no more than fits in the buffer (#7863)

* Make cfread_string_len read no more than fits in the buffer

The char* version only checked the stored length with an Assertion, so in
release builds a string longer than the buffer was read past its end, and a
negative length wrote before it. It now reads at most buffer_size - 1
characters, skips the rest so the next read stays aligned, always terminates
the buffer, and logs when characters are dropped.

The SCP_string version clamps a negative or past-the-end length to what the
file still holds, so a corrupt length can't cause a huge allocation.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* Warn when cfread_string_len cuts or rejects a string

The char* version now reads up to the full buffer, so a string that fills it
exactly without a terminator is caught rather than cut silently. Both versions
clamp the stored length to what the file still holds and check the read limit
before reading, since cfseek does not; inside a limited pilot section a corrupt
length, negative or too long, throws cfile::max_read_length so the loader can
drop the section. A cut string, a corrupt length or a failed read gets a
Warning, and a failed read no longer leaves the file position off by the bytes
it did read. A buffer size of 0 skips the string without writing to the buffer.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

* Write the cfread_string_len warnings inline with format arguments

* Share one core between the cfread_string_len overloads

Both overloads now read the length, work out what is stored, read the
body and warn through the same three steps. A length that does not fit
the file throws inside a limited section even when the section ends at
the end of the file, a zero-size buffer still warns about a corrupt
length, and a cut-off length field warns (or throws inside a limited
section) instead of reading as an empty string.

* Share cfread's read-size check with cfread_string_len

The string reader had its own copy of the read-limit check, without the
clamp to end-of-file that cfread does first, so a truncated last section
threw where it used to load. Both now call cf_check_read_size. A negative
length still throws inside a limited section, a cut-off length field only
warns, the cut warning counts bytes, and the helpers sit in an anonymous
namespace.

* Keep cfread_string_len quiet at a clean end of file

A string read at a clean end of file now returns an empty string without a
warning, as cfread_int does; a length field cut partway still warns, so a
truncated pilot list no longer raises one warning per remaining entry. A
zero-size buffer warns when it drops a string, and an invalid file reads as an
empty string again. Adds tests for an empty string inside a limited section,
the one-short boundary, a length still past the limit after the clamp, a
length field cut by the limit, the string returned after a failed read, and
cfread at end of file.

* Warn when a length field is missing before the end of a file

A failed length read now counts as a clean end of file only when cfeof says
so, so a file whose recorded size is larger than its data still warns. Tests
now pin the read-limit check after the end-of-file clamp, cfread at the end of
file past the limit, a length field cut short on disk, the zero-buffer
Assertion in Debug, and an empty string into a zero-size buffer.

* Close the test file before removing it in the cfile temp-file guards

Windows cannot delete an open file, and a failed ASSERT returns with the
file still open, so the guard now closes it first.

---------

Co-authored-by: Dustinspace217 <[email protected]>
Co-authored-by: Claude Opus 5.5 <[email protected]>

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,98 @@ void cfread_string(char *buf, int n, CFILE *file)
 	} while (c != 0 );
 }
 
-void cfread_string_len(char *buf, int buffer_size, 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");
-	}
+namespace {
+struct string_len_header {
+	int len = 0;
+	int stored = 0;
+	int start = 0;
+	bool length_cut = false;
+};
 
-	buf[MIN(len, buffer_size - 1)] = 0;
+// a past-the-end length is clamped like any cfread, but a negative one has nothing to clamp to, so inside a
+// limited section it throws and the loader skips the rest of the section
+string_len_header read_string_len_header(CFILE* file)
+{
+	string_len_header h;
+	if (!cf_is_valid(file)) {
+		return h;
+	}
+	int before = cftell(file);
+	h.length_cut = cfread(&h.len, sizeof(h.len), 1, file) != 1;
+	h.start = cftell(file);
+	if (h.length_cut) {
+		// a clean end of file reads as an empty string without a warning, as cfread_int does
+		h.length_cut = (h.start != before) || (cfeof(file) == 0);
+		return h;
+	}
+	h.len = INTEL_INT(h.len);
+	if ((h.len < 0) && (file->max_read_len != 0)) {
+		throw cfile::max_read_length("Attempted to read a string of negative length " + std::to_string(h.len));
+	}
+	h.stored = static_cast<int>(cf_check_read_size(file, static_cast<size_t>(std::max(h.len, 0))));
+	return h;
+}
+
+// reads count of the stored characters and leaves the file just past the whole string; false if the read failed
+bool read_string_len_body(const string_len_header& h, char* dest, int count, CFILE* file)
+{
+	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
+void warn_string_len(const string_len_header& h, bool read_ok, bool cut, int buffer_size, 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));
+	} else if (cut) {
+		Warning(LOCATION,
+			"cfread_string_len: a string of %d bytes was cut to fit a buffer of %d in %s",
+			h.len,
+			buffer_size,
+			cf_get_filename(file));
+	}
+}
+} // namespace
+
+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);
+	if (buffer_size <= 0) {
+		bool read_ok = read_string_len_body(h, buf, 0, file);
+		warn_string_len(h, read_ok, h.stored > 0, buffer_size, file);
+		return;
+	}
+	int count = std::min(h.stored, buffer_size);
+	bool read_ok = read_string_len_body(h, buf, count, file);
+	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, cut, buffer_size, 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, false, 0, file);
 
 	return str;
 }

code/cfile/cfile.h

@@ -320,20 +320,26 @@ 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 leaves the file just past the whole string, unless it throws.
+ *
  * @param buf Pre-allocated array to store string
  * @param buffer_size Size of pre-allocated array
  * @param file File to read from
  *
  * @note Appends NULL character to string (buf)
+ * @note Bad data gets a warning; at a clean end of file, or with an invalid file, the string is empty and nothing is
+ * warned. Inside a limited section (cf_set_max_read_len), a negative length, or a read that still passes the limit
+ * once clamped to the end of the file, throws cfile::max_read_length.
  */
 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
+ *
  * @param file The file to read the string from
  * @return The string that was read
+ *
+ * @note Warns and throws like the char* overload, except that nothing is cut.
  */
 SCP_string cfread_string_len(CFILE *file);
 

code/cfile/cfilearchive.cpp

@@ -193,21 +193,8 @@ int cfseek( CFILE *cfile, int offset, int where )
 }
 
 
-// cfread() reads from a file
-//
-// returns:   returns the number of full elements read
-//            
-//
-int cfread(void *buf, int elsize, int nelem, CFILE *cfile)
+size_t cf_check_read_size(CFILE* cfile, size_t size)
 {
-	if(!cf_is_valid(cfile))
-		return 0;
-
-	size_t size = elsize*nelem;
-
-	if(buf == NULL || size <= 0)
-		return 0;
-
 	if ( (cfile->raw_position+size) > cfile->size ) {
 		Assertion(cfile->raw_position <= cfile->size, "Invalid raw_position value detected!");
 		size = cfile->size - cfile->raw_position;
@@ -226,6 +213,29 @@ int cfread(void *buf, int elsize, int nelem, CFILE *cfile)
 		}
 	}
 
+	return size;
+}
+
+// cfread() reads from a file
+//
+// returns:   returns the number of full elements read
+//            
+//
+int cfread(void *buf, int elsize, int nelem, CFILE *cfile)
+{
+	if(!cf_is_valid(cfile))
+		return 0;
+
+	size_t size = elsize*nelem;
+
+	if(buf == nullptr || size <= 0)
+		return 0;
+
+	size = cf_check_read_size(cfile, size);
+	if (size < 1) {
+		return 0;
+	}
+
 	size_t bytes_read;
 	if (cfile->data != nullptr) {
 		// This is a file from memory

code/cfile/cfilearchive.h

@@ -62,4 +62,9 @@ void cf_check_compression(CFILE* cfile);
 // Used to clear compression info data and free dynamic memory used by compression
 void cf_clear_compression_info(CFILE* cfile);
 
+// Clamps a read of size bytes to the end of the file and returns the clamped size, or 0 at the end of the file
+// without checking the limit; throws cfile::max_read_length if the clamped read would pass the file's read limit
+// (cf_set_max_read_len).
+size_t cf_check_read_size(CFILE* cfile, size_t size);
+
 #endif