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/code/model/model.h b/code/model/model.h index 0c74294797c..3abb24777f0 100644 --- a/code/model/model.h +++ b/code/model/model.h @@ -904,6 +904,7 @@ class polymodel // texture maps for model int n_textures; texture_map maps[MAX_MODEL_TEXTURES]; + SCP_vector texture_properties; std::shared_ptr submodel; // an array of size n_models of submodel info. diff --git a/code/model/modelread.cpp b/code/model/modelread.cpp index 84f16dcc872..7ffe67c5601 100644 --- a/code/model/modelread.cpp +++ b/code/model/modelread.cpp @@ -2937,6 +2937,24 @@ modelread_status read_model_file_no_subsys(polymodel * pm, const char* filename, } break; + case ID_TXPR: { + int n = pof_read_count(fp, filename, "texture properties", MAX_MODEL_TEXTURES); + pm->texture_properties.clear(); + for (i = 0; i < n && cftell(fp) < next_chunk; i++) { + auto tex_props = cfread_string_len(fp); + if (cftell(fp) > next_chunk) { + Warning(LOCATION, + "Model %s has a texture properties string that runs past the end of its chunk!", + filename); + break; + } + // POF writers pad strings with zero bytes for alignment + tex_props.resize(strlen(tex_props.c_str())); + pm->texture_properties.push_back(std::move(tex_props)); + } + break; + } + // autocentering info case ID_ACEN: cfread_vector(&pm->autocenter, fp); @@ -2965,6 +2983,14 @@ modelread_status read_model_file_no_subsys(polymodel * pm, const char* filename, Warning(LOCATION, "Model %s has %d textures, but only %d are supported!", filename, n_textures_in_file, MAX_MODEL_TEXTURES); return modelread_status::FAIL; } + if (!pm->texture_properties.empty() && static_cast(pm->texture_properties.size()) != pm->n_textures) { + Warning(LOCATION, + "Model %s has texture properties for %d textures, but it has %d textures!", + filename, + static_cast(pm->texture_properties.size()), + pm->n_textures); + } + pm->texture_properties.resize(pm->n_textures); for (i = 0; i < pm->n_detail_levels; i++) { if (pm->detail[i] < 0 || pm->detail[i] >= pm->n_models) { Warning(LOCATION, "Model %s detail %d is %d which is not a valid submodel!", pm->filename, i, pm->detail[i]); diff --git a/code/model/modelreplace.cpp b/code/model/modelreplace.cpp index 47fb0d653aa..e8d5ae96105 100644 --- a/code/model/modelreplace.cpp +++ b/code/model/modelreplace.cpp @@ -414,6 +414,7 @@ void VirtualPOFOperationAddSubmodel::process(polymodel* pm, model_read_deferred_ return; } pm->n_textures = new_n_textures; + pm->texture_properties.resize(new_n_textures); int old_n_submodel = reallocate_and_copy_array(pm->submodel, pm->n_models, to_copy_submodels.size()); @@ -477,6 +478,9 @@ void VirtualPOFOperationAddSubmodel::process(polymodel* pm, model_read_deferred_ //Actually copy textures for (const auto& usedTexture : textureIDReplace) { pm->maps[usedTexture.second] = appendingPM->maps[usedTexture.first]; + // polygons can name a texture index the source POF never listed + if (usedTexture.first < static_cast(appendingPM->texture_properties.size())) + pm->texture_properties[usedTexture.second] = appendingPM->texture_properties[usedTexture.first]; appendingPMholder->keepTexture(usedTexture.first); } diff --git a/code/model/modelsinc.h b/code/model/modelsinc.h index 80bc4a4ee63..1b8e8b85a7b 100644 --- a/code/model/modelsinc.h +++ b/code/model/modelsinc.h @@ -41,6 +41,7 @@ class polymodel; #define ID_OBJ2 0x324a424f // 2JBO (OBJ2): Subobject header #define ID_TXTR 0x52545854 // RTXT (TXTR): Texture filename list +#define ID_TXPR 0x52505854 // RPXT (TXPR): Per-texture properties, one string per TXTR entry #define ID_INFO 0x464e4950 // FNIP (PINF): POF file information, like command line, etc #define ID_GRID 0x44495247 // DIRG (GRID): Grid information #define ID_SPCL 0x4c435053 // LCPS (SPCL): Special object -- like a gun, missile, docking point, etc. 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); +} diff --git a/test/src/model/test_texture_properties.cpp b/test/src/model/test_texture_properties.cpp new file mode 100644 index 00000000000..9211c821e02 --- /dev/null +++ b/test/src/model/test_texture_properties.cpp @@ -0,0 +1,117 @@ +#include +#include +#include + +#include "util/FSTestFixture.h" + +// The fixtures are a three-texture cube saved by POF-Tools; txpr_count_*.pof and txpr_overrun.pof are byte-patched +// copies of txpr_cube.pof (count or last string length changed, an unknown chunk appended) that POF-Tools can't write. + +class ModelTexturePropertiesTest : public test::FSTestFixture { +public: + ModelTexturePropertiesTest() : test::FSTestFixture(INIT_CFILE | INIT_GRAPHICS) { + pushModDir("model"); + } + +protected: + void SetUp() override { + test::FSTestFixture::SetUp(); + // model_init() may only run once per process; this initializes on first use and otherwise frees + model_free_all(); + } + + void TearDown() override { + model_free_all(); + test::FSTestFixture::TearDown(); + } + + static SCP_string long_properties() { + SCP_string props = "$curve="; + for (int i = 0; i < 1199; i++) { + props += "0.5, "; + } + return props + "x"; + } + + static const SCP_vector& load_properties(const char* filename) { + int model_num = model_load(filename, nullptr, ErrorType::NONE); + EXPECT_GE(model_num, 0); + return model_get(model_num)->texture_properties; + } +}; + +TEST_F(ModelTexturePropertiesTest, read_from_txpr) { + auto& props = load_properties("txpr_cube.pof"); + + ASSERT_EQ(props.size(), 3u); + EXPECT_EQ(props[0], "$hull=1"); + EXPECT_EQ(props[1], ""); + EXPECT_EQ(props[2].size(), 6003u); + EXPECT_EQ(props[2], long_properties()); +} + +TEST_F(ModelTexturePropertiesTest, empty_without_txpr) { + auto& props = load_properties("no_txpr_cube.pof"); + + ASSERT_EQ(props.size(), 3u); + EXPECT_TRUE(props[0].empty()); + EXPECT_TRUE(props[1].empty()); + EXPECT_TRUE(props[2].empty()); +} + +TEST_F(ModelTexturePropertiesTest, count_below_textures_warns_and_pads) { +#ifndef NDEBUG + GTEST_SKIP() << "in debug test builds Warning() throws mid-load and leaves parsing paused for later tests"; +#else + int warnings = Global_warning_count; + auto& props = load_properties("txpr_count_low.pof"); + + EXPECT_EQ(Global_warning_count, warnings + 1); + ASSERT_EQ(props.size(), 3u); + EXPECT_EQ(props[0], "$hull=1"); + EXPECT_TRUE(props[1].empty()); + EXPECT_TRUE(props[2].empty()); +#endif +} + +TEST_F(ModelTexturePropertiesTest, count_above_strings_stops_at_chunk_end) { + int warnings = Global_warning_count; + auto& props = load_properties("txpr_count_high.pof"); + + EXPECT_EQ(Global_warning_count, warnings); + ASSERT_EQ(props.size(), 3u); + EXPECT_EQ(props[0], "$hull=1"); + EXPECT_EQ(props[1], ""); + EXPECT_EQ(props[2], long_properties()); +} + +TEST_F(ModelTexturePropertiesTest, string_past_chunk_end_is_dropped) { +#ifndef NDEBUG + GTEST_SKIP() << "in debug test builds Warning() throws mid-load and leaves parsing paused for later tests"; +#else + int warnings = Global_warning_count; + auto& props = load_properties("txpr_overrun.pof"); + + EXPECT_EQ(Global_warning_count, warnings + 2); + ASSERT_EQ(props.size(), 3u); + EXPECT_EQ(props[0], "$hull=1"); + EXPECT_EQ(props[1], ""); + EXPECT_TRUE(props[2].empty()); +#endif +} + +TEST_F(ModelTexturePropertiesTest, virtual_pof_keeps_added_textures_properties) { + virtual_pof_init(); + int model_num = model_load("vp_texprops.pof", nullptr, ErrorType::NONE); + ASSERT_GE(model_num, 0); + auto pm = model_get(model_num); + + ASSERT_EQ(pm->n_textures, 6); + ASSERT_EQ(pm->texture_properties.size(), 6u); + EXPECT_TRUE(pm->texture_properties[0].empty()); + EXPECT_TRUE(pm->texture_properties[1].empty()); + EXPECT_TRUE(pm->texture_properties[2].empty()); + EXPECT_EQ(pm->texture_properties[3], "$hull=1"); + EXPECT_EQ(pm->texture_properties[4], ""); + EXPECT_EQ(pm->texture_properties[5], long_properties()); +} diff --git a/test/src/source_groups.cmake b/test/src/source_groups.cmake index 1c268dda86e..ee8d516853d 100644 --- a/test/src/source_groups.cmake +++ b/test/src/source_groups.cmake @@ -47,6 +47,7 @@ add_file_folder("mod" add_file_folder("model" model/test_modelread.cpp + model/test_texture_properties.cpp ) add_file_folder("Options" diff --git a/test/test_data/model/data/models/no_txpr_cube.pof b/test/test_data/model/data/models/no_txpr_cube.pof new file mode 100644 index 00000000000..3537eba395e Binary files /dev/null and b/test/test_data/model/data/models/no_txpr_cube.pof differ diff --git a/test/test_data/model/data/models/txpr_count_high.pof b/test/test_data/model/data/models/txpr_count_high.pof new file mode 100644 index 00000000000..5c425e0e3fe Binary files /dev/null and b/test/test_data/model/data/models/txpr_count_high.pof differ diff --git a/test/test_data/model/data/models/txpr_count_low.pof b/test/test_data/model/data/models/txpr_count_low.pof new file mode 100644 index 00000000000..86b9429f4a5 Binary files /dev/null and b/test/test_data/model/data/models/txpr_count_low.pof differ diff --git a/test/test_data/model/data/models/txpr_cube.pof b/test/test_data/model/data/models/txpr_cube.pof new file mode 100644 index 00000000000..c03e238052b Binary files /dev/null and b/test/test_data/model/data/models/txpr_cube.pof differ diff --git a/test/test_data/model/data/models/txpr_overrun.pof b/test/test_data/model/data/models/txpr_overrun.pof new file mode 100644 index 00000000000..bbd1ababa00 Binary files /dev/null and b/test/test_data/model/data/models/txpr_overrun.pof differ diff --git a/test/test_data/model/data/tables/texprops-pof.tbm b/test/test_data/model/data/tables/texprops-pof.tbm new file mode 100644 index 00000000000..af9936de5b1 --- /dev/null +++ b/test/test_data/model/data/tables/texprops-pof.tbm @@ -0,0 +1,13 @@ +#Virtual POFs + +$POF: vp_texprops.pof + +Base POF: no_txpr_cube.pof + $Add Subobject: + +POF to Add: txpr_cube.pof + +Source Subobject: detail0 + $Rename Subobjects: + +Replace: detail0 + +With: grafted + +Destination Subobject: detail0 + +#End