From 238d178c65f1001f2239340afc2457561f1c55ae Mon Sep 17 00:00:00 2001 From: Dustinspace217 Date: Wed, 14 Oct 2026 01:05:43 -0700 Subject: [PATCH 1/7] 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 --- code/cfile/cfile.cpp | 30 +++++++--- code/cfile/cfile.h | 8 ++- test/src/cfile/cfile.cpp | 117 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 11 deletions(-) diff --git a/code/cfile/cfile.cpp b/code/cfile/cfile.cpp index 12f6df421a9..be283490ff2 100644 --- a/code/cfile/cfile.cpp +++ b/code/cfile/cfile.cpp @@ -34,6 +34,7 @@ #include "cfilesystem.h" +#include #include char Cfile_root_dir[CFILE_ROOT_DIRECTORY_LEN] = ""; @@ -1107,26 +1108,37 @@ void cfread_string(char *buf, int n, CFILE *file) 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"); + Assertion(buffer_size > 0, "cfread_string_len needs room for at least the terminator, buffer: %i", buffer_size); + + int len = cfread_int(file); + int to_read = std::clamp(len, 0, buffer_size - 1); + + if (to_read > 0 && cfread(buf, to_read, 1, file) != 1) { + to_read = 0; } + buf[to_read] = 0; - buf[MIN(len, buffer_size - 1)] = 0; + if (len > to_read) { + if (len > buffer_size) { + mprintf(("cfread_string_len: a string of %d characters was cut to fit a buffer of %d in %s\n", len, buffer_size, cf_get_filename(file))); + } + // skip what didn't fit so the next read starts after this string + cfseek(file, len - to_read, CF_SEEK_CUR); + } } SCP_string cfread_string_len(CFILE *file) { int len = cfread_int(file); + // a corrupt length must not make us allocate more than the file can still hold + len = std::clamp(len, 0, std::max(cfilelength(file) - cftell(file), 0)); SCP_string str; str.resize(len); - if (len) - cfread(&str[0], len, 1, file); + if (len > 0 && cfread(str.data(), len, 1, file) != 1) { + str.clear(); + } return str; } diff --git a/code/cfile/cfile.h b/code/cfile/cfile.h index 7c720ac2371..eddebcec64c 100644 --- a/code/cfile/cfile.h +++ b/code/cfile/cfile.h @@ -320,8 +320,9 @@ 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) - * + * Reads at most buffer_size - 1 characters and skips the rest of the string in the file, so the next read + * starts after it. A negative length reads nothing. + * * @param buf Pre-allocated array to store string * @param buffer_size Size of pre-allocated array * @param file File to read from @@ -332,6 +333,9 @@ 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. + * * @param file The file to read the string from * @return The string that was read */ diff --git a/test/src/cfile/cfile.cpp b/test/src/cfile/cfile.cpp index f9842a55081..86574838f70 100644 --- a/test/src/cfile/cfile.cpp +++ b/test/src/cfile/cfile.cpp @@ -1,4 +1,5 @@ +#include #include #include #include @@ -212,3 +213,119 @@ TEST_F(CFileTest, subfolder_list) ASSERT_EQ(2, cf_get_file_list(table_files, CF_TYPE_TABLES, "*\\*.tbl", CF_SORT_NAME)); ASSERT_TRUE(table_files.back().substr(0, 6) == "folder"); } + +namespace { +// Builds the bytes cfwrite_string_len writes (a little-endian length, then the characters) as an in-memory file. +// The file reads from this object's buffer, so the object has to outlive the CFILE. +class StringLenFile { + public: + StringLenFile& add_int(int value) + { + for (int i = 0; i < 4; ++i) { + _bytes.push_back(static_cast((value >> (8 * i)) & 0xFF)); + } + return *this; + } + + StringLenFile& add_chars(const char* chars, size_t count) + { + _bytes.insert(_bytes.end(), chars, chars + count); + return *this; + } + + CFILE* open() const + { + CFileLocation res(true); + res.name_ext = "string_len.bin"; + res.full_name = "string_len.bin"; + res.data_ptr = _bytes.data(); + res.size = _bytes.size(); + return cfopen_special(res, "rb"); + } + + private: + SCP_vector _bytes; +}; +} // namespace + +TEST_F(CFileTest, string_len_fits) +{ + StringLenFile file; + file.add_int(3).add_chars("abc", 3).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[8]; + cfread_string_len(buf, sizeof(buf), fp); + EXPECT_STREQ(buf, "abc"); + EXPECT_EQ(cfread_int(fp), 42); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_exact_fit_with_stored_terminator) +{ + StringLenFile file; + file.add_int(4).add_chars("abc\0", 4).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[4]; + cfread_string_len(buf, sizeof(buf), fp); + EXPECT_STREQ(buf, "abc"); + EXPECT_EQ(cfread_int(fp), 42); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_longer_than_buffer) +{ + StringLenFile file; + file.add_int(10).add_chars("0123456789", 10).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[8]; + memset(buf, 'X', sizeof(buf)); + cfread_string_len(buf, 4, fp); + EXPECT_STREQ(buf, "012"); + for (int i = 4; i < 8; ++i) { + EXPECT_EQ(buf[i], 'X') << "wrote past the buffer at " << i; + } + EXPECT_EQ(cfread_int(fp), 42); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_zero_and_negative) +{ + StringLenFile file; + file.add_int(0).add_int(-5).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[8] = "old"; + cfread_string_len(buf, sizeof(buf), fp); + EXPECT_STREQ(buf, ""); + + strcpy_s(buf, "old"); + cfread_string_len(buf, sizeof(buf), fp); + EXPECT_STREQ(buf, ""); + EXPECT_EQ(cfread_int(fp), 42); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_scp_string) +{ + StringLenFile file; + file.add_int(3).add_chars("abc", 3).add_int(-5).add_int(1000000).add_chars("de", 2); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + EXPECT_EQ(cfread_string_len(fp), "abc"); + EXPECT_EQ(cfread_string_len(fp), ""); + EXPECT_EQ(cfread_string_len(fp), "de"); + + cfclose(fp); +} From b75515dd935e5ea5cde24f00573b05b518552e46 Mon Sep 17 00:00:00 2001 From: Dustinspace217 Date: Thu, 15 Oct 2026 04:42:23 -0700 Subject: [PATCH 2/7] 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 --- code/cfile/cfile.cpp | 65 ++++++++-- code/cfile/cfile.h | 7 +- test/src/cfile/cfile.cpp | 274 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 320 insertions(+), 26 deletions(-) diff --git a/code/cfile/cfile.cpp b/code/cfile/cfile.cpp index be283490ff2..dd0b7de3a5f 100644 --- a/code/cfile/cfile.cpp +++ b/code/cfile/cfile.cpp @@ -1106,38 +1106,75 @@ void cfread_string(char *buf, int n, CFILE *file) } while (c != 0 ); } +static void string_len_warning(const SCP_string& what, CFILE* file) +{ + Warning(LOCATION, "cfread_string_len: %s in %s", what.c_str(), cf_get_filename(file)); +} + +// cfseek ignores the read limit, so it is checked here as cfread would have; a negative length counts as reaching the +// end of the file, so a corrupt length inside a limited section throws +static int string_len_to_read(int len, CFILE* file) +{ + int remaining = std::max(cfilelength(file) - cftell(file), 0); + if (file->max_read_len != 0 && + file->raw_position + (len < 0 ? remaining : std::min(len, remaining)) > file->max_read_len) { + throw cfile::max_read_length( + "Attempted to read a string of length " + std::to_string(len) + " beyond length limit"); + } + return std::clamp(len, 0, remaining); +} + 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); - int len = cfread_int(file); - int to_read = std::clamp(len, 0, buffer_size - 1); + int stored = string_len_to_read(len, file); + int start = cftell(file); + if (buffer_size <= 0) { + cfseek(file, start + stored, CF_SEEK_SET); + return; + } - if (to_read > 0 && cfread(buf, to_read, 1, file) != 1) { + int to_read = std::min(stored, buffer_size); + bool read_failed = to_read > 0 && cfread(buf, to_read, 1, file) != 1; + if (read_failed) { to_read = 0; } - buf[to_read] = 0; + bool cut = to_read == buffer_size && memchr(buf, 0, buffer_size) == nullptr; + buf[std::min(to_read, buffer_size - 1)] = 0; - if (len > to_read) { - if (len > buffer_size) { - mprintf(("cfread_string_len: a string of %d characters was cut to fit a buffer of %d in %s\n", len, buffer_size, cf_get_filename(file))); - } - // skip what didn't fit so the next read starts after this string - cfseek(file, len - to_read, CF_SEEK_CUR); + if (read_failed || stored > to_read) { + cfseek(file, start + stored, CF_SEEK_SET); + } + + // warned last, so the file position is right even if a debug build stops here + if (read_failed) { + string_len_warning("could not read a string of " + std::to_string(stored) + " characters", file); + } else if (len != stored) { + string_len_warning("corrupt string length " + std::to_string(len), file); + } else if (cut) { + string_len_warning("a string of " + std::to_string(len) + " characters was cut to fit a buffer of " + + std::to_string(buffer_size), + file); } } SCP_string cfread_string_len(CFILE *file) { int len = cfread_int(file); - // a corrupt length must not make us allocate more than the file can still hold - len = std::clamp(len, 0, std::max(cfilelength(file) - cftell(file), 0)); + int stored = string_len_to_read(len, file); + int start = cftell(file); SCP_string str; - str.resize(len); + str.resize(stored); - if (len > 0 && cfread(str.data(), len, 1, file) != 1) { + bool read_failed = stored > 0 && cfread(str.data(), stored, 1, file) != 1; + if (read_failed) { str.clear(); + cfseek(file, start + stored, CF_SEEK_SET); + string_len_warning("could not read a string of " + std::to_string(stored) + " characters", file); + } else if (len != stored) { + string_len_warning("corrupt string length " + std::to_string(len), file); } return str; diff --git a/code/cfile/cfile.h b/code/cfile/cfile.h index eddebcec64c..410a9cd18eb 100644 --- a/code/cfile/cfile.h +++ b/code/cfile/cfile.h @@ -320,8 +320,9 @@ 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. * - * Reads at most buffer_size - 1 characters and skips the rest of the string in the file, so the next read - * starts after it. A negative length reads nothing. + * 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, or a length past the end of the file gets a + * warning. * * @param buf Pre-allocated array to store string * @param buffer_size Size of pre-allocated array @@ -334,7 +335,7 @@ 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. + * A negative length reads nothing, and a length past the end of the file reads only what is left; both get a warning. * * @param file The file to read the string from * @return The string that was read diff --git a/test/src/cfile/cfile.cpp b/test/src/cfile/cfile.cpp index 86574838f70..c7ee6616477 100644 --- a/test/src/cfile/cfile.cpp +++ b/test/src/cfile/cfile.cpp @@ -1,8 +1,10 @@ #include #include +#include #include #include +#include #include "util/FSTestFixture.h" @@ -246,6 +248,32 @@ class StringLenFile { private: SCP_vector _bytes; }; + +// Debug test builds turn Warning() into an exception, Release builds only count it +template +void expect_warning(const char* text, Read read) +{ +#ifndef NDEBUG + try { + read(); + ADD_FAILURE() << "expected a warning containing '" << text << "'"; + } catch (const os::dialogs::WarningException& e) { + EXPECT_NE(SCP_string(e.what()).find(text), SCP_string::npos) << e.what(); + } +#else + int warnings = Global_warning_count; + read(); + EXPECT_EQ(Global_warning_count, warnings + 1) << "expected one warning containing '" << text << "'"; +#endif +} + +template +void expect_no_warning(Read read) +{ + int warnings = Global_warning_count; + EXPECT_NO_THROW(read()); + EXPECT_EQ(Global_warning_count, warnings); +} } // namespace TEST_F(CFileTest, string_len_fits) @@ -256,7 +284,7 @@ TEST_F(CFileTest, string_len_fits) ASSERT_NE(fp, nullptr); char buf[8]; - cfread_string_len(buf, sizeof(buf), fp); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, "abc"); EXPECT_EQ(cfread_int(fp), 42); @@ -271,7 +299,7 @@ TEST_F(CFileTest, string_len_exact_fit_with_stored_terminator) ASSERT_NE(fp, nullptr); char buf[4]; - cfread_string_len(buf, sizeof(buf), fp); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, "abc"); EXPECT_EQ(cfread_int(fp), 42); @@ -287,7 +315,7 @@ TEST_F(CFileTest, string_len_longer_than_buffer) char buf[8]; memset(buf, 'X', sizeof(buf)); - cfread_string_len(buf, 4, fp); + expect_warning("was cut", [&] { cfread_string_len(buf, 4, fp); }); EXPECT_STREQ(buf, "012"); for (int i = 4; i < 8; ++i) { EXPECT_EQ(buf[i], 'X') << "wrote past the buffer at " << i; @@ -305,11 +333,11 @@ TEST_F(CFileTest, string_len_zero_and_negative) ASSERT_NE(fp, nullptr); char buf[8] = "old"; - cfread_string_len(buf, sizeof(buf), fp); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, ""); strcpy_s(buf, "old"); - cfread_string_len(buf, sizeof(buf), fp); + expect_warning("corrupt string length -5", [&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, ""); EXPECT_EQ(cfread_int(fp), 42); @@ -319,13 +347,241 @@ TEST_F(CFileTest, string_len_zero_and_negative) TEST_F(CFileTest, string_len_scp_string) { StringLenFile file; - file.add_int(3).add_chars("abc", 3).add_int(-5).add_int(1000000).add_chars("de", 2); + file.add_int(3).add_chars("abc", 3).add_int(-5).add_int(42).add_int(1000000).add_chars("de", 2); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + SCP_string fits; + expect_no_warning([&] { fits = cfread_string_len(fp); }); + EXPECT_EQ(fits, "abc"); + // Debug test builds throw at the warning, before the string is returned, so only the position is checked there + SCP_string negative = "old"; + expect_warning("corrupt string length -5", [&] { negative = cfread_string_len(fp); }); + EXPECT_EQ(cfread_int(fp), 42); + SCP_string past_end; + expect_warning("corrupt string length 1000000", [&] { past_end = cfread_string_len(fp); }); + EXPECT_EQ(cftell(fp), cfilelength(fp)); +#ifdef NDEBUG + EXPECT_EQ(negative, ""); + EXPECT_EQ(past_end, "de"); +#endif + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_exact_fit_without_terminator) +{ + StringLenFile file; + file.add_int(4).add_chars("abcd", 4).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[4]; + expect_warning("was cut", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, "abc"); + EXPECT_EQ(cfread_int(fp), 42); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_past_end_of_file) +{ + StringLenFile file; + file.add_int(20).add_chars("hello", 5); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[32]; + expect_warning("corrupt string length 20", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, "hello"); + EXPECT_EQ(cftell(fp), cfilelength(fp)); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_respects_max_read_len) +{ + StringLenFile file; + file.add_int(12).add_chars("0123456789ab", 12).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, 10); + char buf[4]; + EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_inside_max_read_len) +{ + StringLenFile file; + file.add_int(6).add_chars("abcdef", 6).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + cf_set_max_read_len(fp, 10); + + char buf[8]; + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, "abcdef"); + EXPECT_EQ(cftell(fp), 10); + EXPECT_THROW(cfread_int(fp), cfile::max_read_length); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_cut_at_max_read_len) +{ + StringLenFile file; + file.add_int(6).add_chars("abcdef", 6).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + cf_set_max_read_len(fp, 10); + + char buf[4]; + expect_warning("was cut", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, "abc"); + EXPECT_EQ(cftell(fp), 10); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_negative_inside_max_read_len) +{ + StringLenFile file; + file.add_int(-5).add_chars("abcdefgh", 8).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + cf_set_max_read_len(fp, 8); + + char buf[8]; + EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); + cfclose(fp); + + auto fp2 = file.open(); + ASSERT_NE(fp2, nullptr); + cf_set_max_read_len(fp2, 8); + EXPECT_THROW(cfread_string_len(fp2), cfile::max_read_length); + cfclose(fp2); +} + +TEST_F(CFileTest, string_len_scp_string_respects_max_read_len) +{ + StringLenFile file; + file.add_int(12).add_chars("0123456789ab", 12).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, 10); + EXPECT_THROW(cfread_string_len(fp), cfile::max_read_length); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_nul_inside_buffer_is_not_cut) +{ + StringLenFile file; + file.add_int(10).add_chars("ab\0defghij", 10).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_no_warning([&] { cfread_string_len(buf, 4, fp); }); + EXPECT_STREQ(buf, "ab"); + EXPECT_EQ(buf[3], 0); + for (int i = 4; i < 8; ++i) { + EXPECT_EQ(buf[i], 'X') << "wrote past the buffer at " << i; + } + EXPECT_EQ(cfread_int(fp), 42); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_corrupt_and_cut_warns_once) +{ + StringLenFile file; + file.add_int(20).add_chars("hello", 5); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[4]; + expect_warning("corrupt string length 20", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, "hel"); + EXPECT_EQ(cftell(fp), cfilelength(fp)); + + cfclose(fp); +} + +namespace { +// A file on disk that holds fewer bytes than its recorded size, as when it shrinks after it was opened, so reads come +// up short. The one-byte offset opens it the way a VP entry is opened, which takes the size from the location. +CFILE* open_short_file(const SCP_string& path, const SCP_vector& bytes, size_t recorded_size) +{ + FILE* fp = fopen(path.c_str(), "wb"); + if (fp == nullptr) { + return nullptr; + } + fputc(0, fp); + fwrite(bytes.data(), 1, bytes.size(), fp); + fclose(fp); + + CFileLocation res(true); + res.name_ext = "short.bin"; + res.full_name = path; + res.offset = 1; + res.size = recorded_size; + return cfopen_special(res, "rb"); +} + +SCP_vector length_then_chars(int len, const char* chars) +{ + SCP_vector bytes; + for (int i = 0; i < 4; ++i) { + bytes.push_back(static_cast((len >> (8 * i)) & 0xFF)); + } + bytes.insert(bytes.end(), chars, chars + strlen(chars)); + return bytes; +} +} // namespace + +TEST_F(CFileTest, string_len_failed_read_skips_the_string) +{ + SCP_string path = testing::TempDir() + "cfile_short_read.bin"; + auto fp = open_short_file(path, length_then_chars(10, "hel"), 18); + ASSERT_NE(fp, nullptr); + + char buf[16] = "old"; + expect_warning("could not read", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + EXPECT_EQ(cftell(fp), 14); + cfclose(fp); + + fp = open_short_file(path, length_then_chars(10, "hel"), 18); + ASSERT_NE(fp, nullptr); + expect_warning("could not read", [&] { cfread_string_len(fp); }); + EXPECT_EQ(cftell(fp), 14); + cfclose(fp); + + remove(path.c_str()); +} + +#ifdef NDEBUG +TEST_F(CFileTest, string_len_zero_buffer_writes_nothing) +{ + StringLenFile file; + file.add_int(3).add_chars("abc", 3); auto fp = file.open(); ASSERT_NE(fp, nullptr); - EXPECT_EQ(cfread_string_len(fp), "abc"); - EXPECT_EQ(cfread_string_len(fp), ""); - EXPECT_EQ(cfread_string_len(fp), "de"); + char buf[2] = {'X', 'X'}; + int warnings = Global_warning_count; + cfread_string_len(buf + 1, 0, fp); + EXPECT_EQ(buf[0], 'X'); + EXPECT_EQ(buf[1], 'X'); + EXPECT_EQ(Global_warning_count, warnings); + EXPECT_EQ(cftell(fp), cfilelength(fp)); cfclose(fp); } +#endif From bc6f9c1e410fe8e7dc50551487fe035176b38c8b Mon Sep 17 00:00:00 2001 From: Dustinspace217 Date: Fri, 16 Oct 2026 08:28:32 -0700 Subject: [PATCH 3/7] Write the cfread_string_len warnings inline with format arguments --- code/cfile/cfile.cpp | 26 ++++++++++---------------- 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/code/cfile/cfile.cpp b/code/cfile/cfile.cpp index dd0b7de3a5f..56cd4874700 100644 --- a/code/cfile/cfile.cpp +++ b/code/cfile/cfile.cpp @@ -1106,18 +1106,13 @@ void cfread_string(char *buf, int n, CFILE *file) } while (c != 0 ); } -static void string_len_warning(const SCP_string& what, CFILE* file) -{ - Warning(LOCATION, "cfread_string_len: %s in %s", what.c_str(), cf_get_filename(file)); -} - // cfseek ignores the read limit, so it is checked here as cfread would have; a negative length counts as reaching the // end of the file, so a corrupt length inside a limited section throws static int string_len_to_read(int len, CFILE* file) { int remaining = std::max(cfilelength(file) - cftell(file), 0); if (file->max_read_len != 0 && - file->raw_position + (len < 0 ? remaining : std::min(len, remaining)) > file->max_read_len) { + (file->raw_position + (len < 0 ? remaining : std::min(len, remaining))) > file->max_read_len) { throw cfile::max_read_length( "Attempted to read a string of length " + std::to_string(len) + " beyond length limit"); } @@ -1136,11 +1131,11 @@ void cfread_string_len(char *buf, int buffer_size, CFILE *file) } int to_read = std::min(stored, buffer_size); - bool read_failed = to_read > 0 && cfread(buf, to_read, 1, file) != 1; + bool read_failed = (to_read > 0) && (cfread(buf, to_read, 1, file) != 1); if (read_failed) { to_read = 0; } - bool cut = to_read == buffer_size && memchr(buf, 0, buffer_size) == nullptr; + bool cut = (to_read == buffer_size) && (memchr(buf, 0, buffer_size) == nullptr); buf[std::min(to_read, buffer_size - 1)] = 0; if (read_failed || stored > to_read) { @@ -1149,13 +1144,12 @@ void cfread_string_len(char *buf, int buffer_size, CFILE *file) // warned last, so the file position is right even if a debug build stops here if (read_failed) { - string_len_warning("could not read a string of " + std::to_string(stored) + " characters", file); + Warning(LOCATION, "cfread_string_len: could not read a string of %d characters in %s", stored, cf_get_filename(file)); } else if (len != stored) { - string_len_warning("corrupt string length " + std::to_string(len), file); + Warning(LOCATION, "cfread_string_len: corrupt string length %d in %s", len, cf_get_filename(file)); } else if (cut) { - string_len_warning("a string of " + std::to_string(len) + " characters was cut to fit a buffer of " + - std::to_string(buffer_size), - file); + Warning(LOCATION, "cfread_string_len: a string of %d characters was cut to fit a buffer of %d in %s", len, + buffer_size, cf_get_filename(file)); } } @@ -1168,13 +1162,13 @@ SCP_string cfread_string_len(CFILE *file) SCP_string str; str.resize(stored); - bool read_failed = stored > 0 && cfread(str.data(), stored, 1, file) != 1; + bool read_failed = (stored > 0) && (cfread(str.data(), stored, 1, file) != 1); if (read_failed) { str.clear(); cfseek(file, start + stored, CF_SEEK_SET); - string_len_warning("could not read a string of " + std::to_string(stored) + " characters", file); + Warning(LOCATION, "cfread_string_len: could not read a string of %d characters in %s", stored, cf_get_filename(file)); } else if (len != stored) { - string_len_warning("corrupt string length " + std::to_string(len), file); + Warning(LOCATION, "cfread_string_len: corrupt string length %d in %s", len, cf_get_filename(file)); } return str; From 030d66738478d7aacf7a388fd6fc05b5faf06f0c Mon Sep 17 00:00:00 2001 From: Dustinspace217 Date: Fri, 16 Oct 2026 09:22:42 -0700 Subject: [PATCH 4/7] 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. --- code/cfile/cfile.cpp | 113 +++++++++++++++++++++++---------------- code/cfile/cfile.h | 9 ++-- test/src/cfile/cfile.cpp | 73 +++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 50 deletions(-) diff --git a/code/cfile/cfile.cpp b/code/cfile/cfile.cpp index 56cd4874700..80d6f123a55 100644 --- a/code/cfile/cfile.cpp +++ b/code/cfile/cfile.cpp @@ -33,7 +33,6 @@ #include "parse/encrypt.h" #include "cfilesystem.h" - #include #include @@ -1106,70 +1105,90 @@ void cfread_string(char *buf, int n, CFILE *file) } while (c != 0 ); } -// cfseek ignores the read limit, so it is checked here as cfread would have; a negative length counts as reaching the -// end of the file, so a corrupt length inside a limited section throws -static int string_len_to_read(int len, CFILE* file) -{ - int remaining = std::max(cfilelength(file) - cftell(file), 0); - if (file->max_read_len != 0 && - (file->raw_position + (len < 0 ? remaining : std::min(len, remaining))) > file->max_read_len) { +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(len) + " beyond length limit"); + "Attempted to read a string of length " + std::to_string(h.len) + " beyond length limit"); } - return std::clamp(len, 0, remaining); + h.stored = h.length_cut ? 0 : std::clamp(h.len, 0, remaining); + return h; } -void cfread_string_len(char *buf, int buffer_size, CFILE *file) +// 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) { - Assertion(buffer_size > 0, "cfread_string_len needs room for at least the terminator, buffer: %i", buffer_size); - int len = cfread_int(file); - int stored = string_len_to_read(len, file); - int start = cftell(file); - if (buffer_size <= 0) { - cfseek(file, start + stored, CF_SEEK_SET); - return; - } - - int to_read = std::min(stored, buffer_size); - bool read_failed = (to_read > 0) && (cfread(buf, to_read, 1, file) != 1); - if (read_failed) { - to_read = 0; + bool ok = (count <= 0) || (cfread(dest, count, 1, file) == 1); + if (!ok || (h.stored > count)) { + cfseek(file, h.start + h.stored, CF_SEEK_SET); } - bool cut = (to_read == buffer_size) && (memchr(buf, 0, buffer_size) == nullptr); - buf[std::min(to_read, buffer_size - 1)] = 0; + return ok; +} - if (read_failed || stored > to_read) { - cfseek(file, start + stored, CF_SEEK_SET); +// 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)); } +} - // warned last, so the file position is right even if a debug build stops here - if (read_failed) { - Warning(LOCATION, "cfread_string_len: could not read a string of %d characters in %s", stored, cf_get_filename(file)); - } else if (len != stored) { - Warning(LOCATION, "cfread_string_len: corrupt string length %d in %s", len, cf_get_filename(file)); - } else if (cut) { - Warning(LOCATION, "cfread_string_len: a string of %d characters was cut to fit a buffer of %d in %s", len, - buffer_size, cf_get_filename(file)); +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); - int stored = string_len_to_read(len, file); - int start = cftell(file); + string_len_header h = read_string_len_header(file); SCP_string str; - str.resize(stored); - - bool read_failed = (stored > 0) && (cfread(str.data(), stored, 1, file) != 1); - if (read_failed) { + str.resize(h.stored); + bool read_ok = read_string_len_body(h, str.data(), h.stored, file); + if (!read_ok) { str.clear(); - cfseek(file, start + stored, CF_SEEK_SET); - Warning(LOCATION, "cfread_string_len: could not read a string of %d characters in %s", stored, cf_get_filename(file)); - } else if (len != stored) { - Warning(LOCATION, "cfread_string_len: corrupt string length %d in %s", len, cf_get_filename(file)); } + warn_string_len(h, read_ok, file); return str; } diff --git a/code/cfile/cfile.h b/code/cfile/cfile.h index 410a9cd18eb..8d81ffa1d2f 100644 --- a/code/cfile/cfile.h +++ b/code/cfile/cfile.h @@ -321,8 +321,9 @@ 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. * * 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, or a length past the end of the file gets a - * warning. + * 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 @@ -335,7 +336,9 @@ 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. + * 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 diff --git a/test/src/cfile/cfile.cpp b/test/src/cfile/cfile.cpp index c7ee6616477..89a28d4fb1c 100644 --- a/test/src/cfile/cfile.cpp +++ b/test/src/cfile/cfile.cpp @@ -585,3 +585,76 @@ TEST_F(CFileTest, string_len_zero_buffer_writes_nothing) cfclose(fp); } #endif + +TEST_F(CFileTest, string_len_corrupt_length_in_last_section_throws) +{ + StringLenFile file; + file.add_int(-5); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, cfilelength(fp)); + char buf[8]; + EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); + cfclose(fp); + + StringLenFile past_end; + past_end.add_int(1000000).add_chars("hi", 2); + fp = past_end.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, cfilelength(fp)); + EXPECT_THROW(cfread_string_len(fp), cfile::max_read_length); + cfclose(fp); +} + +TEST_F(CFileTest, string_len_cut_off_length_field_warns) +{ + StringLenFile file; + file.add_chars("\x05\x00", 2); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[8] = "old"; + expect_warning("length field", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + EXPECT_EQ(cftell(fp), cfilelength(fp)); + cfclose(fp); + + fp = file.open(); + ASSERT_NE(fp, nullptr); + SCP_string str; + expect_warning("length field", [&] { str = cfread_string_len(fp); }); + EXPECT_EQ(str, ""); + EXPECT_EQ(cftell(fp), cfilelength(fp)); + cfclose(fp); +} + +#ifdef NDEBUG +TEST_F(CFileTest, string_len_zero_buffer_still_warns_on_corrupt_length) +{ + StringLenFile file; + file.add_int(-5); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[2] = {'X', 'X'}; + expect_warning("corrupt string length -5", [&] { cfread_string_len(buf + 1, 0, fp); }); + EXPECT_EQ(buf[0], 'X'); + EXPECT_EQ(buf[1], 'X'); + cfclose(fp); +} +#endif + +TEST_F(CFileTest, string_len_cut_off_length_field_inside_max_read_len_throws) +{ + StringLenFile file; + file.add_chars("\x00\x00", 2); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, cfilelength(fp)); + char buf[8]; + EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); + cfclose(fp); +} From 764d0fe8442d04aeeb966a2e38b02619d6904e25 Mon Sep 17 00:00:00 2001 From: Dustinspace217 Date: Sat, 17 Oct 2026 20:03:33 -0700 Subject: [PATCH 5/7] 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. --- code/cfile/cfile.cpp | 58 +++++++++++++++++++------------------ code/cfile/cfile.h | 13 ++++----- code/cfile/cfilearchive.cpp | 38 +++++++++++++++--------- code/cfile/cfilearchive.h | 4 +++ test/src/cfile/cfile.cpp | 54 +++++++++++++++++++++++++++------- 5 files changed, 106 insertions(+), 61 deletions(-) diff --git a/code/cfile/cfile.cpp b/code/cfile/cfile.cpp index 80d6f123a55..0ba64abd9d8 100644 --- a/code/cfile/cfile.cpp +++ b/code/cfile/cfile.cpp @@ -1105,6 +1105,7 @@ void cfread_string(char *buf, int n, CFILE *file) } while (c != 0 ); } +namespace { struct string_len_header { int len = 0; int stored = 0; @@ -1112,26 +1113,26 @@ struct string_len_header { 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) +// 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; 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"); + if (h.length_cut) { + return h; } - h.stored = h.length_cut ? 0 : std::clamp(h.len, 0, remaining); + 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(cf_check_read_size(file, static_cast(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 -static bool read_string_len_body(const string_len_header& h, char* dest, int count, CFILE* file) +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)) { @@ -1141,7 +1142,7 @@ static bool read_string_len_body(const string_len_header& h, char* dest, int cou } // 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) +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)); @@ -1152,30 +1153,31 @@ static void warn_string_len(const string_len_header& h, bool read_ok, CFILE* fil 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); - 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)); - } + if (buffer_size <= 0) { + bool read_ok = read_string_len_body(h, buf, 0, file); + warn_string_len(h, read_ok, false, buffer_size, file); return; } - warn_string_len(h, read_ok, file); + 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) @@ -1188,7 +1190,7 @@ SCP_string cfread_string_len(CFILE *file) if (!read_ok) { str.clear(); } - warn_string_len(h, read_ok, file); + warn_string_len(h, read_ok, false, 0, file); return str; } diff --git a/code/cfile/cfile.h b/code/cfile/cfile.h index 8d81ffa1d2f..6498f589ca7 100644 --- a/code/cfile/cfile.h +++ b/code/cfile/cfile.h @@ -320,28 +320,25 @@ 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. * - * 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. + * Keeps at most buffer_size - 1 characters and leaves the file just past the whole string. * * @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. Inside a limited section (cf_set_max_read_len), a negative length or a string that + * runs past the limit 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 * - * 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 + * + * @note Warns and throws in the same cases as the char* overload. */ SCP_string cfread_string_len(CFILE *file); diff --git a/code/cfile/cfilearchive.cpp b/code/cfile/cfilearchive.cpp index eb34205b59b..409814b9c58 100644 --- a/code/cfile/cfilearchive.cpp +++ b/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 diff --git a/code/cfile/cfilearchive.h b/code/cfile/cfilearchive.h index 275a95282f5..26a796c8512 100644 --- a/code/cfile/cfilearchive.h +++ b/code/cfile/cfilearchive.h @@ -62,4 +62,8 @@ 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; throws +// cfile::max_read_length if the 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 diff --git a/test/src/cfile/cfile.cpp b/test/src/cfile/cfile.cpp index 89a28d4fb1c..04c663b04e7 100644 --- a/test/src/cfile/cfile.cpp +++ b/test/src/cfile/cfile.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include "util/FSTestFixture.h" @@ -548,6 +549,7 @@ SCP_vector length_then_chars(int len, const char* chars) TEST_F(CFileTest, string_len_failed_read_skips_the_string) { SCP_string path = testing::TempDir() + "cfile_short_read.bin"; + auto remove_file = util::finally([&] { remove(path.c_str()); }); auto fp = open_short_file(path, length_then_chars(10, "hel"), 18); ASSERT_NE(fp, nullptr); @@ -562,8 +564,6 @@ TEST_F(CFileTest, string_len_failed_read_skips_the_string) expect_warning("could not read", [&] { cfread_string_len(fp); }); EXPECT_EQ(cftell(fp), 14); cfclose(fp); - - remove(path.c_str()); } #ifdef NDEBUG @@ -586,7 +586,7 @@ TEST_F(CFileTest, string_len_zero_buffer_writes_nothing) } #endif -TEST_F(CFileTest, string_len_corrupt_length_in_last_section_throws) +TEST_F(CFileTest, string_len_negative_in_last_section_throws) { StringLenFile file; file.add_int(-5); @@ -598,16 +598,46 @@ TEST_F(CFileTest, string_len_corrupt_length_in_last_section_throws) EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); cfclose(fp); - StringLenFile past_end; - past_end.add_int(1000000).add_chars("hi", 2); - fp = past_end.open(); + fp = file.open(); ASSERT_NE(fp, nullptr); - cf_set_max_read_len(fp, cfilelength(fp)); EXPECT_THROW(cfread_string_len(fp), cfile::max_read_length); cfclose(fp); } +TEST_F(CFileTest, string_len_past_end_in_last_section_warns) +{ + StringLenFile file; + file.add_int(1000000).add_chars("hi", 2); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, cfilelength(fp)); + SCP_string str; + expect_warning("corrupt string length 1000000", [&] { str = cfread_string_len(fp); }); +#ifdef NDEBUG + EXPECT_EQ(str, "hi"); +#endif + EXPECT_EQ(cftell(fp), cfilelength(fp)); + cfclose(fp); +} + +TEST_F(CFileTest, string_len_truncated_section_reads_what_is_left) +{ + StringLenFile file; + file.add_int(20).add_chars("abc", 3); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + // the section claims more bytes than the file still has, as in a truncated pilot file + cf_set_max_read_len(fp, 10); + char buf[8] = "old"; + expect_warning("corrupt string length 20", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, "abc"); + EXPECT_EQ(cftell(fp), cfilelength(fp)); + cfclose(fp); +} + TEST_F(CFileTest, string_len_cut_off_length_field_warns) { StringLenFile file; @@ -646,15 +676,17 @@ TEST_F(CFileTest, string_len_zero_buffer_still_warns_on_corrupt_length) } #endif -TEST_F(CFileTest, string_len_cut_off_length_field_inside_max_read_len_throws) +TEST_F(CFileTest, string_len_cut_off_length_field_inside_max_read_len_warns) { StringLenFile file; - file.add_chars("\x00\x00", 2); + file.add_chars("\x05\x00", 2); auto fp = file.open(); ASSERT_NE(fp, nullptr); cf_set_max_read_len(fp, cfilelength(fp)); - char buf[8]; - EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); + char buf[8] = "old"; + expect_warning("length field", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + EXPECT_EQ(cftell(fp), cfilelength(fp)); cfclose(fp); } From 78eaab20fe46ce9e8194e042fbc06fc9881dfd84 Mon Sep 17 00:00:00 2001 From: Dustinspace217 Date: Sat, 17 Oct 2026 20:23:49 -0700 Subject: [PATCH 6/7] 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. --- code/cfile/cfile.cpp | 8 +- code/cfile/cfile.h | 9 ++- code/cfile/cfilearchive.h | 5 +- test/src/cfile/cfile.cpp | 155 ++++++++++++++++++++++++++++++++++++-- 4 files changed, 162 insertions(+), 15 deletions(-) diff --git a/code/cfile/cfile.cpp b/code/cfile/cfile.cpp index 0ba64abd9d8..b516283c35d 100644 --- a/code/cfile/cfile.cpp +++ b/code/cfile/cfile.cpp @@ -1118,9 +1118,15 @@ struct string_len_header { 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); return h; } h.len = INTEL_INT(h.len); @@ -1169,7 +1175,7 @@ void cfread_string_len(char* buf, int buffer_size, CFILE* file) 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, false, buffer_size, file); + warn_string_len(h, read_ok, h.stored > 0, buffer_size, file); return; } int count = std::min(h.stored, buffer_size); diff --git a/code/cfile/cfile.h b/code/cfile/cfile.h index 6498f589ca7..8272e4b9239 100644 --- a/code/cfile/cfile.h +++ b/code/cfile/cfile.h @@ -320,15 +320,16 @@ 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. * - * Keeps at most buffer_size - 1 characters and leaves the file just past the whole string. + * 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. Inside a limited section (cf_set_max_read_len), a negative length or a string that - * runs past the limit throws cfile::max_read_length. + * @note Bad data gets a warning; at a clean end of 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); @@ -338,7 +339,7 @@ void cfread_string_len(char *buf,int buffer_size, CFILE *file); * @param file The file to read the string from * @return The string that was read * - * @note Warns and throws in the same cases as the char* overload. + * @note Warns and throws like the char* overload, except that nothing is cut. */ SCP_string cfread_string_len(CFILE *file); diff --git a/code/cfile/cfilearchive.h b/code/cfile/cfilearchive.h index 26a796c8512..b32d9654a5b 100644 --- a/code/cfile/cfilearchive.h +++ b/code/cfile/cfilearchive.h @@ -62,8 +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; throws -// cfile::max_read_length if the read would pass the file's read limit (cf_set_max_read_len). +// 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 diff --git a/test/src/cfile/cfile.cpp b/test/src/cfile/cfile.cpp index 04c663b04e7..0de897234ee 100644 --- a/test/src/cfile/cfile.cpp +++ b/test/src/cfile/cfile.cpp @@ -285,6 +285,23 @@ TEST_F(CFileTest, string_len_fits) ASSERT_NE(fp, nullptr); char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, "abc"); + EXPECT_EQ(cfread_int(fp), 42); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_one_short_of_the_buffer) +{ + StringLenFile file; + file.add_int(3).add_chars("abc", 3).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[4]; + memset(buf, 'X', sizeof(buf)); expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, "abc"); EXPECT_EQ(cfread_int(fp), 42); @@ -292,6 +309,65 @@ TEST_F(CFileTest, string_len_fits) cfclose(fp); } +TEST_F(CFileTest, string_len_empty_inside_max_read_len) +{ + StringLenFile file; + file.add_int(0).add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + cf_set_max_read_len(fp, 8); + + char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + EXPECT_EQ(cfread_int(fp), 42); + cfclose(fp); + + fp = file.open(); + ASSERT_NE(fp, nullptr); + cf_set_max_read_len(fp, 4); + SCP_string str = "old"; + expect_no_warning([&] { str = cfread_string_len(fp); }); + EXPECT_EQ(str, ""); + EXPECT_EQ(cftell(fp), 4); + cfclose(fp); +} + +TEST_F(CFileTest, string_len_clean_end_of_file_is_silent) +{ + StringLenFile file; + file.add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + ASSERT_EQ(cfread_int(fp), 42); + + char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + EXPECT_EQ(cftell(fp), 4); + + SCP_string str = "old"; + expect_no_warning([&] { str = cfread_string_len(fp); }); + EXPECT_EQ(str, ""); + EXPECT_EQ(cftell(fp), 4); + + cfclose(fp); +} + +TEST_F(CFileTest, string_len_without_a_file_reads_empty) +{ + char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), nullptr); }); + EXPECT_STREQ(buf, ""); + + SCP_string str = "old"; + expect_no_warning([&] { str = cfread_string_len(nullptr); }); + EXPECT_EQ(str, ""); +} + TEST_F(CFileTest, string_len_exact_fit_with_stored_terminator) { StringLenFile file; @@ -393,6 +469,7 @@ TEST_F(CFileTest, string_len_past_end_of_file) ASSERT_NE(fp, nullptr); char buf[32]; + memset(buf, 'X', sizeof(buf)); expect_warning("corrupt string length 20", [&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, "hello"); EXPECT_EQ(cftell(fp), cfilelength(fp)); @@ -507,6 +584,7 @@ TEST_F(CFileTest, string_len_corrupt_and_cut_warns_once) ASSERT_NE(fp, nullptr); char buf[4]; + memset(buf, 'X', sizeof(buf)); expect_warning("corrupt string length 20", [&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, "hel"); EXPECT_EQ(cftell(fp), cfilelength(fp)); @@ -523,9 +601,10 @@ CFILE* open_short_file(const SCP_string& path, const SCP_vector& bytes, si if (fp == nullptr) { return nullptr; } - fputc(0, fp); - fwrite(bytes.data(), 1, bytes.size(), fp); - fclose(fp); + bool written = (fputc(0, fp) == 0) && (fwrite(bytes.data(), 1, bytes.size(), fp) == bytes.size()); + if ((fclose(fp) != 0) || !written) { + return nullptr; + } CFileLocation res(true); res.name_ext = "short.bin"; @@ -561,13 +640,17 @@ TEST_F(CFileTest, string_len_failed_read_skips_the_string) fp = open_short_file(path, length_then_chars(10, "hel"), 18); ASSERT_NE(fp, nullptr); - expect_warning("could not read", [&] { cfread_string_len(fp); }); + SCP_string str = "old"; + expect_warning("could not read", [&] { str = cfread_string_len(fp); }); +#ifdef NDEBUG + EXPECT_EQ(str, ""); +#endif EXPECT_EQ(cftell(fp), 14); cfclose(fp); } #ifdef NDEBUG -TEST_F(CFileTest, string_len_zero_buffer_writes_nothing) +TEST_F(CFileTest, string_len_zero_buffer_warns_and_writes_nothing) { StringLenFile file; file.add_int(3).add_chars("abc", 3); @@ -579,7 +662,7 @@ TEST_F(CFileTest, string_len_zero_buffer_writes_nothing) cfread_string_len(buf + 1, 0, fp); EXPECT_EQ(buf[0], 'X'); EXPECT_EQ(buf[1], 'X'); - EXPECT_EQ(Global_warning_count, warnings); + EXPECT_EQ(Global_warning_count, warnings + 1); EXPECT_EQ(cftell(fp), cfilelength(fp)); cfclose(fp); @@ -631,7 +714,8 @@ TEST_F(CFileTest, string_len_truncated_section_reads_what_is_left) // the section claims more bytes than the file still has, as in a truncated pilot file cf_set_max_read_len(fp, 10); - char buf[8] = "old"; + char buf[8]; + memset(buf, 'X', sizeof(buf)); expect_warning("corrupt string length 20", [&] { cfread_string_len(buf, sizeof(buf), fp); }); EXPECT_STREQ(buf, "abc"); EXPECT_EQ(cftell(fp), cfilelength(fp)); @@ -653,9 +737,11 @@ TEST_F(CFileTest, string_len_cut_off_length_field_warns) fp = file.open(); ASSERT_NE(fp, nullptr); - SCP_string str; + SCP_string str = "old"; expect_warning("length field", [&] { str = cfread_string_len(fp); }); +#ifdef NDEBUG EXPECT_EQ(str, ""); +#endif EXPECT_EQ(cftell(fp), cfilelength(fp)); cfclose(fp); } @@ -690,3 +776,56 @@ TEST_F(CFileTest, string_len_cut_off_length_field_inside_max_read_len_warns) EXPECT_EQ(cftell(fp), cfilelength(fp)); cfclose(fp); } + +TEST_F(CFileTest, string_len_clamped_but_still_past_max_read_len_throws) +{ + StringLenFile file; + file.add_int(20).add_chars("0123456789", 10); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, 6); + char buf[8]; + EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); + cfclose(fp); +} + +TEST_F(CFileTest, string_len_length_field_cut_by_max_read_len_throws) +{ + StringLenFile file; + file.add_int(5).add_chars("abcde", 5); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, 2); + char buf[8]; + EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); + cfclose(fp); +} + +TEST_F(CFileTest, cfread_keeps_whole_elements_at_end_of_file) +{ + StringLenFile file; + file.add_chars("abcdef", 6); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[12]; + EXPECT_EQ(cfread(buf, 4, 3, fp), 1); + EXPECT_EQ(cftell(fp), 6); + cfclose(fp); +} + +TEST_F(CFileTest, cfread_at_end_of_file_does_not_check_max_read_len) +{ + StringLenFile file; + file.add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + cf_set_max_read_len(fp, 4); + EXPECT_EQ(cfread_int(fp), 42); + int value = 0; + EXPECT_NO_THROW(EXPECT_EQ(cfread(&value, sizeof(value), 1, fp), 0)); + cfclose(fp); +} From 002ce29316ff29ca7a725f8dc122e7d91e0b50e7 Mon Sep 17 00:00:00 2001 From: Dustinspace217 Date: Sat, 17 Oct 2026 20:37:21 -0700 Subject: [PATCH 7/7] 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. --- code/cfile/cfile.cpp | 2 +- code/cfile/cfile.h | 6 +-- test/src/cfile/cfile.cpp | 83 ++++++++++++++++++++++++++++++++++++++-- 3 files changed, 84 insertions(+), 7 deletions(-) diff --git a/code/cfile/cfile.cpp b/code/cfile/cfile.cpp index b516283c35d..f4868f104fa 100644 --- a/code/cfile/cfile.cpp +++ b/code/cfile/cfile.cpp @@ -1126,7 +1126,7 @@ string_len_header read_string_len_header(CFILE* file) 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); + h.length_cut = (h.start != before) || (cfeof(file) == 0); return h; } h.len = INTEL_INT(h.len); diff --git a/code/cfile/cfile.h b/code/cfile/cfile.h index 8272e4b9239..f5e99d8960b 100644 --- a/code/cfile/cfile.h +++ b/code/cfile/cfile.h @@ -327,9 +327,9 @@ void cfread_string(char *buf,int n, CFILE *file); * @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 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. + * @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); diff --git a/test/src/cfile/cfile.cpp b/test/src/cfile/cfile.cpp index 0de897234ee..1a7f390e0da 100644 --- a/test/src/cfile/cfile.cpp +++ b/test/src/cfile/cfile.cpp @@ -784,8 +784,9 @@ TEST_F(CFileTest, string_len_clamped_but_still_past_max_read_len_throws) auto fp = file.open(); ASSERT_NE(fp, nullptr); + // a 2-byte buffer keeps the body read inside the limit, so only the header's check can throw cf_set_max_read_len(fp, 6); - char buf[8]; + char buf[2]; EXPECT_THROW(cfread_string_len(buf, sizeof(buf), fp), cfile::max_read_length); cfclose(fp); } @@ -823,9 +824,85 @@ TEST_F(CFileTest, cfread_at_end_of_file_does_not_check_max_read_len) auto fp = file.open(); ASSERT_NE(fp, nullptr); - cf_set_max_read_len(fp, 4); - EXPECT_EQ(cfread_int(fp), 42); + cf_set_max_read_len(fp, 2); + cfseek(fp, 0, CF_SEEK_END); int value = 0; EXPECT_NO_THROW(EXPECT_EQ(cfread(&value, sizeof(value), 1, fp), 0)); cfclose(fp); } + +TEST_F(CFileTest, string_len_clean_end_of_file_inside_max_read_len_is_silent) +{ + StringLenFile file; + file.add_int(42); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + cf_set_max_read_len(fp, 4); + ASSERT_EQ(cfread_int(fp), 42); + + char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_no_warning([&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + cfclose(fp); +} + +TEST_F(CFileTest, string_len_missing_length_field_before_the_end_warns) +{ + SCP_string path = testing::TempDir() + "cfile_missing_length.bin"; + auto remove_file = util::finally([&] { remove(path.c_str()); }); + auto fp = open_short_file(path, {}, 8); + ASSERT_NE(fp, nullptr); + + char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_warning("length field", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + cfclose(fp); +} + +#ifdef NDEBUG +TEST_F(CFileTest, string_len_zero_buffer_and_empty_string_is_silent) +{ + StringLenFile file; + file.add_int(0); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[2] = {'X', 'X'}; + expect_no_warning([&] { cfread_string_len(buf + 1, 0, fp); }); + EXPECT_EQ(buf[1], 'X'); + cfclose(fp); +} +#endif + +TEST_F(CFileTest, string_len_length_field_cut_short_on_disk_warns) +{ + SCP_string path = testing::TempDir() + "cfile_short_length.bin"; + auto remove_file = util::finally([&] { remove(path.c_str()); }); + auto fp = open_short_file(path, {'\x05', '\x00'}, 18); + ASSERT_NE(fp, nullptr); + + char buf[8]; + memset(buf, 'X', sizeof(buf)); + expect_warning("length field", [&] { cfread_string_len(buf, sizeof(buf), fp); }); + EXPECT_STREQ(buf, ""); + EXPECT_EQ(cftell(fp), 2); + cfclose(fp); +} + +#ifndef NDEBUG +TEST_F(CFileTest, string_len_zero_buffer_asserts) +{ + StringLenFile file; + file.add_int(3).add_chars("abc", 3); + auto fp = file.open(); + ASSERT_NE(fp, nullptr); + + char buf[2] = {'X', 'X'}; + EXPECT_THROW(cfread_string_len(buf + 1, 0, fp), os::dialogs::AssertException); + EXPECT_EQ(buf[0], 'X'); + EXPECT_EQ(buf[1], 'X'); + cfclose(fp); +} +#endif