From 5c3ae3d94cfbcc7e01dc4e62bbdc798ffc51c402 Mon Sep 17 00:00:00 2001 From: elsid Date: Mon, 17 Jun 2024 22:46:13 +0200 Subject: [PATCH] Make loading and saving script record more robust * Check the header presence before using it. * Write the header based on the actual content. --- apps/components_tests/esm3/testsaveload.cpp | 27 ++- apps/esmtool/record.cpp | 11 +- apps/essimporter/importscpt.cpp | 4 +- apps/essimporter/importscpt.hpp | 6 +- components/esm3/loadscpt.cpp | 198 +++++++++++--------- components/esm3/loadscpt.hpp | 20 +- 6 files changed, 139 insertions(+), 127 deletions(-) diff --git a/apps/components_tests/esm3/testsaveload.cpp b/apps/components_tests/esm3/testsaveload.cpp index afbe410014..41a79313cc 100644 --- a/apps/components_tests/esm3/testsaveload.cpp +++ b/apps/components_tests/esm3/testsaveload.cpp @@ -279,11 +279,11 @@ namespace ESM Script record; record.blank(); record.mId = generateRandomRefId(33); - record.mData.mNumShorts = 42; + record.mNumShorts = 42; Script result; saveAndLoadRecord(record, CurrentSaveGameFormatVersion, result); EXPECT_EQ(result.mId, record.mId); - EXPECT_EQ(result.mData.mNumShorts, record.mData.mNumShorts); + EXPECT_EQ(result.mNumShorts, record.mNumShorts); } TEST_P(Esm3SaveLoadRecordTest, playerShouldNotChange) @@ -417,26 +417,21 @@ namespace ESM Script record; record.blank(); record.mId = generateRandomRefId(32); - record.mData.mNumShorts = 3; - record.mData.mNumFloats = 4; - record.mData.mNumLongs = 5; - record.mData.mScriptDataSize = 13; - generateStrings(std::back_inserter(record.mVarNames), - record.mData.mNumShorts + record.mData.mNumFloats + record.mData.mNumLongs); - record.mData.mStringTableSize = std::accumulate(record.mVarNames.begin(), record.mVarNames.end(), 0, - [](std::size_t r, const std::string& v) { return r + v.size() + 1; }); - generateBytes(std::back_inserter(record.mScriptData), record.mData.mScriptDataSize); + record.mNumShorts = 3; + record.mNumFloats = 4; + record.mNumLongs = 5; + generateStrings( + std::back_inserter(record.mVarNames), record.mNumShorts + record.mNumFloats + record.mNumLongs); + generateBytes(std::back_inserter(record.mScriptData), 13); record.mScriptText = generateRandomString(17); Script result; saveAndLoadRecord(record, GetParam(), result); EXPECT_EQ(result.mId, record.mId); - EXPECT_EQ(result.mData.mNumShorts, record.mData.mNumShorts); - EXPECT_EQ(result.mData.mNumFloats, record.mData.mNumFloats); - EXPECT_EQ(result.mData.mNumShorts, record.mData.mNumShorts); - EXPECT_EQ(result.mData.mScriptDataSize, record.mData.mScriptDataSize); - EXPECT_EQ(result.mData.mStringTableSize, record.mData.mStringTableSize); + EXPECT_EQ(result.mNumShorts, record.mNumShorts); + EXPECT_EQ(result.mNumFloats, record.mNumFloats); + EXPECT_EQ(result.mNumShorts, record.mNumShorts); EXPECT_EQ(result.mVarNames, record.mVarNames); EXPECT_EQ(result.mScriptData, record.mScriptData); EXPECT_EQ(result.mScriptText, record.mScriptText); diff --git a/apps/esmtool/record.cpp b/apps/esmtool/record.cpp index cba389568b..b9c64d964a 100644 --- a/apps/esmtool/record.cpp +++ b/apps/esmtool/record.cpp @@ -2,6 +2,7 @@ #include "labels.hpp" #include +#include #include #include @@ -1181,11 +1182,11 @@ namespace EsmTool { std::cout << " Name: " << mData.mId << std::endl; - std::cout << " Num Shorts: " << mData.mData.mNumShorts << std::endl; - std::cout << " Num Longs: " << mData.mData.mNumLongs << std::endl; - std::cout << " Num Floats: " << mData.mData.mNumFloats << std::endl; - std::cout << " Script Data Size: " << mData.mData.mScriptDataSize << std::endl; - std::cout << " Table Size: " << mData.mData.mStringTableSize << std::endl; + std::cout << " Num Shorts: " << mData.mNumShorts << std::endl; + std::cout << " Num Longs: " << mData.mNumLongs << std::endl; + std::cout << " Num Floats: " << mData.mNumFloats << std::endl; + std::cout << " Script Data Size: " << mData.mScriptData.size() << std::endl; + std::cout << " Table Size: " << ESM::computeScriptStringTableSize(mData.mVarNames) << std::endl; for (const std::string& variable : mData.mVarNames) std::cout << " Variable: " << variable << std::endl; diff --git a/apps/essimporter/importscpt.cpp b/apps/essimporter/importscpt.cpp index 8fe4afd336..bb62c61103 100644 --- a/apps/essimporter/importscpt.cpp +++ b/apps/essimporter/importscpt.cpp @@ -7,8 +7,8 @@ namespace ESSImport void SCPT::load(ESM::ESMReader& esm) { - esm.getHNT("SCHD", mSCHD.mName.mData, mSCHD.mData.mNumShorts, mSCHD.mData.mNumLongs, mSCHD.mData.mNumFloats, - mSCHD.mData.mScriptDataSize, mSCHD.mData.mStringTableSize); + esm.getHNT("SCHD", mSCHD.mName.mData, mSCHD.mNumShorts, mSCHD.mNumLongs, mSCHD.mNumFloats, + mSCHD.mScriptDataSize, mSCHD.mStringTableSize); mSCRI.load(esm); diff --git a/apps/essimporter/importscpt.hpp b/apps/essimporter/importscpt.hpp index fe936af439..7c728ee97e 100644 --- a/apps/essimporter/importscpt.hpp +++ b/apps/essimporter/importscpt.hpp @@ -19,7 +19,11 @@ namespace ESSImport struct SCHD { ESM::NAME32 mName; - ESM::Script::SCHD mData; + std::uint32_t mNumShorts; + std::uint32_t mNumLongs; + std::uint32_t mNumFloats; + std::uint32_t mScriptDataSize; + std::uint32_t mStringTableSize; }; // A running global script diff --git a/components/esm3/loadscpt.cpp b/components/esm3/loadscpt.cpp index 9339233116..91f952f3f0 100644 --- a/components/esm3/loadscpt.cpp +++ b/components/esm3/loadscpt.cpp @@ -1,6 +1,8 @@ #include "loadscpt.hpp" #include +#include +#include #include #include @@ -11,80 +13,93 @@ namespace ESM { - template T> - void decompose(T&& v, const auto& f) + namespace { - f(v.mNumShorts, v.mNumLongs, v.mNumFloats, v.mScriptDataSize, v.mStringTableSize); - } - - void Script::loadSCVR(ESMReader& esm) - { - uint32_t s = mData.mStringTableSize; - - std::vector tmp(s); - // not using getHExact, vanilla doesn't seem to mind unused bytes at the end - esm.getSubHeader(); - uint32_t left = esm.getSubSize(); - if (left < s) - esm.fail("SCVR string list is smaller than specified"); - esm.getExact(tmp.data(), s); - if (left > s) - esm.skip(left - s); // skip the leftover junk - - // Set up the list of variable names - mVarNames.resize(mData.mNumShorts + mData.mNumLongs + mData.mNumFloats); - - // The tmp buffer is a null-byte separated string list, we - // just have to pick out one string at a time. - char* str = tmp.data(); - if (tmp.empty()) + struct SCHD { - if (mVarNames.size() > 0) - Log(Debug::Warning) << "SCVR with no variable names"; + /// Data from script-precompling in the editor. + /// \warning Do not use them. OpenCS currently does not precompile scripts. + std::uint32_t mNumShorts = 0; + std::uint32_t mNumLongs = 0; + std::uint32_t mNumFloats = 0; + std::uint32_t mScriptDataSize = 0; + std::uint32_t mStringTableSize = 0; + }; - return; + template T> + void decompose(T&& v, const auto& f) + { + f(v.mNumShorts, v.mNumLongs, v.mNumFloats, v.mScriptDataSize, v.mStringTableSize); } - // Support '\r' terminated strings like vanilla. See Bug #1324. - std::replace(tmp.begin(), tmp.end(), '\r', '\0'); - // Avoid heap corruption - if (tmp.back() != '\0') + void loadVarNames(const SCHD& header, std::vector& varNames, ESMReader& esm) { - tmp.emplace_back('\0'); - std::stringstream ss; - ss << "Malformed string table"; - ss << "\n File: " << esm.getName(); - ss << "\n Record: " << esm.getContext().recName.toStringView(); - ss << "\n Subrecord: " - << "SCVR"; - ss << "\n Offset: 0x" << std::hex << esm.getFileOffset(); - Log(Debug::Verbose) << ss.str(); - str = tmp.data(); - } + uint32_t s = header.mStringTableSize; - const auto tmpEnd = tmp.data() + tmp.size(); - for (size_t i = 0; i < mVarNames.size(); i++) - { - mVarNames[i] = std::string(str); - str += mVarNames[i].size() + 1; - if (str >= tmpEnd) + std::vector tmp(s); + // not using getHExact, vanilla doesn't seem to mind unused bytes at the end + esm.getSubHeader(); + uint32_t left = esm.getSubSize(); + if (left < s) + esm.fail("SCVR string list is smaller than specified"); + esm.getExact(tmp.data(), s); + if (left > s) + esm.skip(left - s); // skip the leftover junk + + // Set up the list of variable names + varNames.resize(header.mNumShorts + header.mNumLongs + header.mNumFloats); + + // The tmp buffer is a null-byte separated string list, we + // just have to pick out one string at a time. + if (tmp.empty()) { - if (str > tmpEnd) + if (!varNames.empty()) + Log(Debug::Warning) << "SCVR with no variable names"; + + return; + } + + // Support '\r' terminated strings like vanilla. See Bug #1324. + std::replace(tmp.begin(), tmp.end(), '\r', '\0'); + // Avoid heap corruption + if (tmp.back() != '\0') + { + tmp.push_back('\0'); + std::stringstream ss; + ss << "Malformed string table"; + ss << "\n File: " << esm.getName(); + ss << "\n Record: " << esm.getContext().recName.toStringView(); + ss << "\n Subrecord: " + << "SCVR"; + ss << "\n Offset: 0x" << std::hex << esm.getFileOffset(); + Log(Debug::Verbose) << ss.str(); + } + + const char* str = tmp.data(); + const char* const tmpEnd = tmp.data() + tmp.size(); + for (size_t i = 0; i < varNames.size(); i++) + { + varNames[i] = std::string(str); + str += varNames[i].size() + 1; + if (str >= tmpEnd) { - // SCVR subrecord is unused and variable names are determined - // from the script source, so an overflow is not fatal. - std::stringstream ss; - ss << "String table overflow"; - ss << "\n File: " << esm.getName(); - ss << "\n Record: " << esm.getContext().recName.toStringView(); - ss << "\n Subrecord: " - << "SCVR"; - ss << "\n Offset: 0x" << std::hex << esm.getFileOffset(); - Log(Debug::Verbose) << ss.str(); + if (str > tmpEnd) + { + // SCVR subrecord is unused and variable names are determined + // from the script source, so an overflow is not fatal. + std::stringstream ss; + ss << "String table overflow"; + ss << "\n File: " << esm.getName(); + ss << "\n Record: " << esm.getContext().recName.toStringView(); + ss << "\n Subrecord: " + << "SCVR"; + ss << "\n Offset: 0x" << std::hex << esm.getFileOffset(); + Log(Debug::Verbose) << ss.str(); + } + // Get rid of empty strings in the list. + varNames.resize(i + 1); + break; } - // Get rid of empty strings in the list. - mVarNames.resize(i + 1); - break; } } } @@ -96,7 +111,7 @@ namespace ESM mVarNames.clear(); - bool hasHeader = false; + std::optional header; while (esm.hasMoreSubs()) { esm.getSubName(); @@ -106,22 +121,27 @@ namespace ESM { esm.getSubHeader(); mId = esm.getMaybeFixedRefIdSize(32); - esm.getComposite(mData); - - hasHeader = true; + esm.getComposite(header.emplace()); + mNumShorts = header->mNumShorts; + mNumLongs = header->mNumLongs; + mNumFloats = header->mNumFloats; break; } case fourCC("SCVR"): - // list of local variables - loadSCVR(esm); + if (!header.has_value()) + esm.fail("SCVR is placed before SCHD record"); + loadVarNames(*header, mVarNames, esm); break; case fourCC("SCDT"): { + if (!header.has_value()) + esm.fail("SCDT is placed before SCHD record"); + // compiled script esm.getSubHeader(); uint32_t subSize = esm.getSubSize(); - if (subSize != mData.mScriptDataSize) + if (subSize != header->mScriptDataSize) { std::stringstream ss; ss << "Script data size defined in SCHD subrecord does not match size of SCDT subrecord"; @@ -147,22 +167,21 @@ namespace ESM } } - if (!hasHeader) + if (!header.has_value()) esm.fail("Missing SCHD subrecord"); - // Reported script data size is not always trustworthy, so override it with actual data size - mData.mScriptDataSize = static_cast(mScriptData.size()); } void Script::save(ESMWriter& esm, bool isDeleted) const { - std::string varNameString; - if (!mVarNames.empty()) - for (std::vector::const_iterator it = mVarNames.begin(); it != mVarNames.end(); ++it) - varNameString.append(*it); - esm.startSubRecord("SCHD"); esm.writeMaybeFixedSizeRefId(mId, 32); - esm.writeComposite(mData); + esm.writeComposite(SCHD{ + .mNumShorts = mNumShorts, + .mNumLongs = mNumLongs, + .mNumFloats = mNumFloats, + .mScriptDataSize = static_cast(mScriptData.size()), + .mStringTableSize = computeScriptStringTableSize(mVarNames), + }); esm.endRecord("SCHD"); if (isDeleted) @@ -174,15 +193,13 @@ namespace ESM if (!mVarNames.empty()) { esm.startSubRecord("SCVR"); - for (std::vector::const_iterator it = mVarNames.begin(); it != mVarNames.end(); ++it) - { - esm.writeHCString(*it); - } + for (const std::string& v : mVarNames) + esm.writeHCString(v); esm.endRecord("SCVR"); } esm.startSubRecord("SCDT"); - esm.write(reinterpret_cast(mScriptData.data()), mData.mScriptDataSize); + esm.write(reinterpret_cast(mScriptData.data()), mScriptData.size()); esm.endRecord("SCDT"); esm.writeHNOString("SCTX", mScriptText); @@ -191,9 +208,9 @@ namespace ESM void Script::blank() { mRecordFlags = 0; - mData.mNumShorts = mData.mNumLongs = mData.mNumFloats = 0; - mData.mScriptDataSize = 0; - mData.mStringTableSize = 0; + mNumShorts = 0; + mNumLongs = 0; + mNumFloats = 0; mVarNames.clear(); mScriptData.clear(); @@ -204,4 +221,9 @@ namespace ESM mScriptText = "Begin " + stringId + "\n\nEnd " + stringId + "\n"; } + std::uint32_t computeScriptStringTableSize(const std::vector& varNames) + { + return std::accumulate(varNames.begin(), varNames.end(), static_cast(0), + [](std::uint32_t r, const std::string& v) { return r + 1 + static_cast(v.size()); }); + } } diff --git a/components/esm3/loadscpt.hpp b/components/esm3/loadscpt.hpp index 50f2463ce2..2b98807e7b 100644 --- a/components/esm3/loadscpt.hpp +++ b/components/esm3/loadscpt.hpp @@ -26,21 +26,12 @@ namespace ESM /// Return a string descriptor for this record type. Currently used for debugging / error logs only. static std::string_view getRecordType() { return "Script"; } - struct SCHD - { - /// Data from script-precompling in the editor. - /// \warning Do not use them. OpenCS currently does not precompile scripts. - std::uint32_t mNumShorts; - std::uint32_t mNumLongs; - std::uint32_t mNumFloats; - std::uint32_t mScriptDataSize; - std::uint32_t mStringTableSize; - }; - uint32_t mRecordFlags; RefId mId; - SCHD mData; + std::uint32_t mNumShorts; + std::uint32_t mNumLongs; + std::uint32_t mNumFloats; /// Variable names generated by script-precompiling in the editor. /// \warning Do not use this field. OpenCS currently does not precompile scripts. @@ -58,9 +49,8 @@ namespace ESM void blank(); ///< Set record to default state (does not touch the ID/index). - - private: - void loadSCVR(ESMReader& esm); }; + + std::uint32_t computeScriptStringTableSize(const std::vector& varNames); } #endif