From 4f683d1ee9a20d8dc4591938af1c193851a9c29a Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 11 Feb 2023 14:52:31 +0100 Subject: [PATCH 1/4] Throw exception on failed write --- components/esm3/cellref.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/esm3/cellref.cpp b/components/esm3/cellref.cpp index 733274619e..1e0dffb92b 100644 --- a/components/esm3/cellref.cpp +++ b/components/esm3/cellref.cpp @@ -169,7 +169,7 @@ namespace ESM else { if (isSet() && !hasContentFile()) - Log(Debug::Error) << "Generated RefNum can not be saved in 32bit format"; + throw std::runtime_error("Generated RefNum can not be saved in 32bit format"); int refNum = (mIndex & 0xffffff) | ((hasContentFile() ? mContentFile : 0xff) << 24); esm.writeHNT(tag, refNum, 4); } From a5ec108cfb0ab727934da37be6aae1fd2f33d9a2 Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 11 Feb 2023 14:52:50 +0100 Subject: [PATCH 2/4] Add missing space --- components/esm3/esmreader.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/esm3/esmreader.hpp b/components/esm3/esmreader.hpp index 7fde566d9e..b398b4aee5 100644 --- a/components/esm3/esmreader.hpp +++ b/components/esm3/esmreader.hpp @@ -297,7 +297,7 @@ namespace ESM private: [[noreturn]] void reportSubSizeMismatch(size_t want, size_t got) { - fail("record size mismatch, requested " + std::to_string(want) + ", got" + std::to_string(got)); + fail("record size mismatch, requested " + std::to_string(want) + ", got " + std::to_string(got)); } void clearCtx(); From 2e64155c0fa7d6fc0c254f649fe29096bad20d67 Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 11 Feb 2023 15:13:15 +0100 Subject: [PATCH 3/4] Use signed type for left record and files size in ESM3 reader context Otherwise reading some of the records like ESM::CellRef without a subrecord after could lead to underflow of ESM_Context::leftRec which makes ESM::ESMReader::hasMoreSubs to return true and load hangs for a while trying to read the same subrecord many times. Fix ESM::Variant tests since it's now required to have a record for any ESM data. Add 16 (size of record header) to all expected data sizes. --- apps/openmw_test_suite/esm/variant.cpp | 59 ++++++++++++-------------- components/esm/esmcommon.hpp | 5 ++- components/esm3/esmreader.cpp | 16 ++++--- 3 files changed, 41 insertions(+), 39 deletions(-) diff --git a/apps/openmw_test_suite/esm/variant.cpp b/apps/openmw_test_suite/esm/variant.cpp index 3a97d2fe37..82da91c1c4 100644 --- a/apps/openmw_test_suite/esm/variant.cpp +++ b/apps/openmw_test_suite/esm/variant.cpp @@ -1,3 +1,4 @@ +#include #include #include #include @@ -12,6 +13,8 @@ namespace using namespace testing; using namespace ESM; + constexpr std::uint32_t fakeRecordId = fourCC("FAKE"); + Variant makeVariant(VarType type) { Variant v; @@ -430,25 +433,28 @@ namespace std::ostringstream out; ESMWriter writer; writer.save(out); + writer.startRecord(fakeRecordId); variant.write(writer, format); + writer.endRecord(fakeRecordId); writer.close(); return out.str(); } - Variant read(const Variant::Format format, const std::string& data) + void read(const Variant::Format format, const std::string& data, Variant& result) { - Variant result; ESMReader reader; - reader.open(std::make_unique(data), ""); + reader.open(std::make_unique(data), "stream"); + ASSERT_TRUE(reader.hasMoreRecs()); + ASSERT_EQ(reader.getRecName().toInt(), fakeRecordId); + reader.getRecHeader(); result.read(reader, format); - return result; } - Variant writeAndRead(const Variant& variant, const Variant::Format format, std::size_t dataSize) + void writeAndRead(const Variant& variant, const Variant::Format format, std::size_t dataSize, Variant& result) { const std::string data = write(variant, format); EXPECT_EQ(data.size(), dataSize); - return read(format, data); + read(format, data, result); } struct ESMVariantToESMTest : TestWithParam @@ -458,36 +464,27 @@ namespace TEST_P(ESMVariantToESMTest, deserialized_is_equal_to_serialized) { const auto param = GetParam(); - const auto result = writeAndRead(param.mVariant, param.mFormat, param.mDataSize); + ESM::Variant result; + writeAndRead(param.mVariant, param.mFormat, param.mDataSize, result); ASSERT_EQ(param.mVariant, result); } - INSTANTIATE_TEST_SUITE_P(VariantAndData, ESMVariantToESMTest, - Values(WriteToESMTestCase{ Variant(), Variant::Format_Gmst, 324 }, - WriteToESMTestCase{ Variant(int{ 42 }), Variant::Format_Global, 345 }, - WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Global, 345 }, - WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Info, 336 }, - WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Local, 336 }, - WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Global, 345 }, - WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Local, 334 }, - WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Info, 336 }, - WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Local, 336 })); - - struct ESMVariantToESMNoneTest : TestWithParam - { + const std::array deserializedParams = { + WriteToESMTestCase{ Variant(), Variant::Format_Gmst, 340 }, + WriteToESMTestCase{ Variant(int{ 42 }), Variant::Format_Global, 361 }, + WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Global, 361 }, + WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Info, 352 }, + WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Local, 352 }, + WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Global, 361 }, + WriteToESMTestCase{ makeVariant(VT_Short, 42), Variant::Format_Local, 350 }, + WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Info, 352 }, + WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Local, 352 }, + WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Gmst, 352 }, + WriteToESMTestCase{ Variant(std::string("foo")), Variant::Format_Gmst, 351 }, + WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Gmst, 352 }, }; - TEST_P(ESMVariantToESMNoneTest, deserialized_is_none) - { - const auto param = GetParam(); - const auto result = writeAndRead(param.mVariant, param.mFormat, param.mDataSize); - ASSERT_EQ(Variant(), result); - } - - INSTANTIATE_TEST_SUITE_P(VariantAndData, ESMVariantToESMNoneTest, - Values(WriteToESMTestCase{ Variant(float{ 2.7f }), Variant::Format_Gmst, 336 }, - WriteToESMTestCase{ Variant(std::string("foo")), Variant::Format_Gmst, 335 }, - WriteToESMTestCase{ makeVariant(VT_Int, 42), Variant::Format_Gmst, 336 })); + INSTANTIATE_TEST_SUITE_P(VariantAndData, ESMVariantToESMTest, ValuesIn(deserializedParams)); struct ESMVariantWriteToESMFailTest : TestWithParam { diff --git a/components/esm/esmcommon.hpp b/components/esm/esmcommon.hpp index e12a4703ce..b2da3a7f41 100644 --- a/components/esm/esmcommon.hpp +++ b/components/esm/esmcommon.hpp @@ -188,8 +188,9 @@ namespace ESM struct ESM_Context { std::filesystem::path filename; - uint32_t leftRec, leftSub; - size_t leftFile; + std::streamsize leftRec; + std::uint32_t leftSub; + std::streamsize leftFile; NAME recName, subName; // When working with multiple esX files, we will generate lists of all files that // actually contribute to a specific cell. Therefore, we need to store the index diff --git a/components/esm3/esmreader.cpp b/components/esm3/esmreader.cpp index a358e262c1..a6ed122ed5 100644 --- a/components/esm3/esmreader.cpp +++ b/components/esm3/esmreader.cpp @@ -297,16 +297,18 @@ namespace ESM void ESMReader::getSubHeader() { - if (mCtx.leftRec < sizeof(mCtx.leftSub)) - fail("End of record while reading sub-record header"); + if (mCtx.leftRec < static_cast(sizeof(mCtx.leftSub))) + fail("End of record while reading sub-record header: " + std::to_string(mCtx.leftRec) + " < " + + std::to_string(sizeof(mCtx.leftSub))); // Get subrecord size - getT(mCtx.leftSub); + getUint(mCtx.leftSub); mCtx.leftRec -= sizeof(mCtx.leftSub); // Adjust number of record bytes left if (mCtx.leftRec < mCtx.leftSub) - fail("Record size is larger than rest of file"); + fail("Record size is larger than rest of file: " + std::to_string(mCtx.leftRec) + " < " + + std::to_string(mCtx.leftSub)); mCtx.leftRec -= mCtx.leftSub; } @@ -335,12 +337,14 @@ namespace ESM void ESMReader::getRecHeader(uint32_t& flags) { // General error checking - if (mCtx.leftFile < 3 * sizeof(uint32_t)) + if (mCtx.leftFile < static_cast(3 * sizeof(uint32_t))) fail("End of file while reading record header"); if (mCtx.leftRec) fail("Previous record contains unread bytes"); - getUint(mCtx.leftRec); + std::uint32_t leftRec = 0; + getUint(leftRec); + mCtx.leftRec = static_cast(leftRec); getUint(flags); // This header entry is always zero getUint(flags); mCtx.leftFile -= 3 * sizeof(uint32_t); From 1e9e7b7607647d6b8a7c45221c0b1654d709b167 Mon Sep 17 00:00:00 2001 From: elsid Date: Sat, 11 Feb 2023 13:30:28 +0100 Subject: [PATCH 4/4] Add tests to save and load some ESM3 records --- apps/openmw_test_suite/CMakeLists.txt | 1 + apps/openmw_test_suite/esm3/testsaveload.cpp | 133 +++++++++++++++++++ 2 files changed, 134 insertions(+) create mode 100644 apps/openmw_test_suite/esm3/testsaveload.cpp diff --git a/apps/openmw_test_suite/CMakeLists.txt b/apps/openmw_test_suite/CMakeLists.txt index cf1be3a18a..0a487e1022 100644 --- a/apps/openmw_test_suite/CMakeLists.txt +++ b/apps/openmw_test_suite/CMakeLists.txt @@ -90,6 +90,7 @@ file(GLOB UNITTEST_SRC_FILES fx/technique.cpp esm3/readerscache.cpp + esm3/testsaveload.cpp nifosg/testnifloader.cpp ) diff --git a/apps/openmw_test_suite/esm3/testsaveload.cpp b/apps/openmw_test_suite/esm3/testsaveload.cpp new file mode 100644 index 0000000000..d14c108c00 --- /dev/null +++ b/apps/openmw_test_suite/esm3/testsaveload.cpp @@ -0,0 +1,133 @@ +#include +#include +#include +#include + +#include +#include + +#include +#include +#include + +namespace ESM +{ + namespace + { + using namespace ::testing; + + constexpr std::array formats = { + CurrentSaveGameFormatVersion, + }; + + constexpr std::uint32_t fakeRecordId = fourCC("FAKE"); + + template + void save(const T& record, ESMWriter& writer) + { + record.save(writer); + } + + void save(const CellRef& record, ESMWriter& writer) + { + record.save(writer, true); + } + + template + std::unique_ptr makeEsmStream(const T& record, FormatVersion formatVersion) + { + ESMWriter writer; + auto stream = std::make_unique(); + writer.setFormatVersion(formatVersion); + writer.save(*stream); + writer.startRecord(fakeRecordId); + save(record, writer); + writer.endRecord(fakeRecordId); + return stream; + } + + template + void load(ESMReader& reader, T& record) + { + record.load(reader); + } + + void load(ESMReader& reader, CellRef& record) + { + bool deleted = false; + record.load(reader, deleted, true); + } + + template + void saveAndLoadRecord(const T& record, FormatVersion formatVersion, T& result) + { + ESMReader reader; + reader.open(makeEsmStream(record, formatVersion), "stream"); + ASSERT_TRUE(reader.hasMoreRecs()); + ASSERT_EQ(reader.getRecName().toInt(), fakeRecordId); + reader.getRecHeader(); + load(reader, result); + } + + struct Esm3SaveLoadRecordTest : public TestWithParam + { + std::minstd_rand mRandom; + std::uniform_int_distribution mRefIdDistribution{ 'a', 'z' }; + + RefId generateRandomRefId(std::size_t size = 33) + { + std::string value; + while (value.size() < size) + value.push_back(static_cast(mRefIdDistribution(mRandom))); + return RefId::stringRefId(value); + } + }; + + TEST_P(Esm3SaveLoadRecordTest, playerShouldNotChange) + { + std::minstd_rand random; + Player record{}; + record.mObject.blank(); + record.mBirthsign = generateRandomRefId(); + record.mObject.mRef.mRefID = generateRandomRefId(); + std::generate_n(std::inserter(record.mPreviousItems, record.mPreviousItems.end()), 2, + [&] { return std::make_pair(generateRandomRefId(), generateRandomRefId()); }); + Player result; + saveAndLoadRecord(record, GetParam(), result); + EXPECT_EQ(record.mBirthsign, result.mBirthsign); + EXPECT_EQ(record.mPreviousItems, result.mPreviousItems); + } + + TEST_P(Esm3SaveLoadRecordTest, cellRefShouldNotChange) + { + CellRef record; + record.blank(); + record.mRefID = generateRandomRefId(); + record.mOwner = generateRandomRefId(); + record.mSoul = generateRandomRefId(); + record.mFaction = generateRandomRefId(); + record.mKey = generateRandomRefId(); + CellRef result; + saveAndLoadRecord(record, GetParam(), result); + EXPECT_EQ(record.mRefID, result.mRefID); + EXPECT_EQ(record.mOwner, result.mOwner); + EXPECT_EQ(record.mSoul, result.mSoul); + EXPECT_EQ(record.mFaction, result.mFaction); + EXPECT_EQ(record.mKey, result.mKey); + } + + TEST_P(Esm3SaveLoadRecordTest, creatureStatsShouldNotChange) + { + CreatureStats record; + record.blank(); + record.mLastHitAttemptObject = generateRandomRefId(); + record.mLastHitObject = generateRandomRefId(); + CreatureStats result; + saveAndLoadRecord(record, GetParam(), result); + EXPECT_EQ(record.mLastHitAttemptObject, result.mLastHitAttemptObject); + EXPECT_EQ(record.mLastHitObject, result.mLastHitObject); + } + + INSTANTIATE_TEST_SUITE_P(FormatVersions, Esm3SaveLoadRecordTest, ValuesIn(formats)); + } +}