Support variable size strings in ESM3

apps/essimporter/importcellref.cpp

@@ -111,7 +111,7 @@ namespace ESSImport
         {
             // used power
             esm.getSubHeader();
-            std::string id = esm.getString(32);
+            std::string id = esm.getMaybeFixedStringSize(32);
             (void)id;
             // timestamp can't be used: this is the total hours passed, calculated by
             // timestamp = 24 * (365 * year + cumulativeDays[month] + day)

apps/openmw_test_suite/esm3/testesmwriter.cpp

@@ -37,7 +37,7 @@ namespace ESM
             ESMWriter writer;
             writer.setAuthor(author);
             writer.setDescription(description);
-            writer.setFormatVersion(CurrentSaveGameFormatVersion);
+            writer.setFormatVersion(MaxLimitedSizeStringsFormatVersion);
             EXPECT_THROW(writer.save(stream), std::runtime_error);
         }
 
@@ -46,9 +46,9 @@ namespace ESM
             std::stringstream stream;
 
             ESMWriter writer;
-            writer.setFormatVersion(CurrentSaveGameFormatVersion);
+            writer.setFormatVersion(MaxLimitedSizeStringsFormatVersion);
             writer.save(stream);
-            EXPECT_THROW(writer.writeFixedSizeString(generateRandomString(33), 32), std::runtime_error);
+            EXPECT_THROW(writer.writeMaybeFixedSizeString(generateRandomString(33), 32), std::runtime_error);
         }
     }
 }

apps/openmw_test_suite/esm3/testsaveload.cpp

@@ -17,6 +17,7 @@ namespace ESM
         using namespace ::testing;
 
         constexpr std::array formats = {
+            MaxLimitedSizeStringsFormatVersion,
             CurrentSaveGameFormatVersion,
         };
 
@@ -74,15 +75,37 @@ namespace ESM
             std::minstd_rand mRandom;
             std::uniform_int_distribution<short> mRefIdDistribution{ 'a', 'z' };
 
-            RefId generateRandomRefId(std::size_t size = 33)
+            std::string generateRandomString(std::size_t size)
             {
                 std::string value;
                 while (value.size() < size)
                     value.push_back(static_cast<char>(mRefIdDistribution(mRandom)));
-                return RefId::stringRefId(value);
+                return value;
             }
+
+            RefId generateRandomRefId(std::size_t size = 33) { return RefId::stringRefId(generateRandomString(size)); }
         };
 
+        TEST_F(Esm3SaveLoadRecordTest, headerShouldNotChange)
+        {
+            const std::string author = generateRandomString(33);
+            const std::string description = generateRandomString(257);
+
+            auto stream = std::make_unique<std::stringstream>();
+
+            ESMWriter writer;
+            writer.setAuthor(author);
+            writer.setDescription(description);
+            writer.setFormatVersion(CurrentSaveGameFormatVersion);
+            writer.save(*stream);
+            writer.close();
+
+            ESMReader reader;
+            reader.open(std::move(stream), "stream");
+            EXPECT_EQ(reader.getAuthor(), author);
+            EXPECT_EQ(reader.getDesc(), description);
+        }
+
         TEST_P(Esm3SaveLoadRecordTest, playerShouldNotChange)
         {
             std::minstd_rand random;

components/esm/esmcommon.hpp

@@ -28,6 +28,8 @@ namespace ESM
         FLAG_Blocked = 0x00002000
     };
 
+    using StringSizeType = std::uint32_t;
+
     template <std::size_t capacity>
     struct FixedString
     {

components/esm3/esmreader.cpp

@@ -145,26 +145,10 @@ namespace ESM
 
     std::string ESMReader::getHString()
     {
-        getSubHeader();
+        return std::string(getHStringView());
-
-        // Hack to make MultiMark.esp load. Zero-length strings do not
-        // occur in any of the official mods, but MultiMark makes use of
-        // them. For some reason, they break the rules, and contain a byte
-        // (value 0) even if the header says there is no data. If
-        // Morrowind accepts it, so should we.
-        if (mCtx.leftSub == 0 && hasMoreSubs() && !mEsm->peek())
-        {
-            // Skip the following zero byte
-            mCtx.leftRec--;
-            char c;
-            getT(c);
-            return std::string();
-        }
-
-        return getString(mCtx.leftSub);
     }
 
-    RefId ESMReader::getRefId()
+    std::string_view ESMReader::getHStringView()
     {
         getSubHeader();
 
@@ -179,10 +163,15 @@ namespace ESM
             mCtx.leftRec--;
             char c;
             getT(c);
-            return ESM::RefId::sEmpty;
+            return std::string_view();
         }
 
-        return getRefId(mCtx.leftSub);
+        return getStringView(mCtx.leftSub);
+    }
+
+    RefId ESMReader::getRefId()
+    {
+        return ESM::RefId::stringRefId(getHStringView());
     }
 
     void ESMReader::skipHString()
@@ -363,52 +352,42 @@ namespace ESM
      *
      *************************************************************************/
 
-    std::string ESMReader::getString(int size)
+    std::string ESMReader::getMaybeFixedStringSize(std::size_t size)
     {
-        size_t s = size;
+        if (mHeader.mFormatVersion > MaxLimitedSizeStringsFormatVersion)
-        if (mBuffer.size() <= s)
+        {
-            // Add some extra padding to reduce the chance of having to resize
+            StringSizeType storedSize = 0;
-            // again later.
+            getT(storedSize);
-            mBuffer.resize(3 * s);
+            if (storedSize > mCtx.leftSub)
+                fail("String does not fit subrecord (" + std::to_string(storedSize) + " > "
+                    + std::to_string(mCtx.leftSub) + ")");
+            size = static_cast<std::size_t>(storedSize);
+        }
 
-        // And make sure the string is zero terminated
+        return std::string(getStringView(size));
-        mBuffer[s] = 0;
-
-        // read ESM data
-        char* ptr = mBuffer.data();
-        getExact(ptr, size);
-
-        size = static_cast<int>(strnlen(ptr, size));
-
-        // Convert to UTF8 and return
-        if (mEncoder)
-            return std::string(mEncoder->getUtf8(std::string_view(ptr, size)));
-
-        return std::string(ptr, size);
     }
 
-    ESM::RefId ESMReader::getRefId(int size)
+    std::string_view ESMReader::getStringView(std::size_t size)
     {
-        size_t s = size;
+        if (mBuffer.size() <= size)
-        if (mBuffer.size() <= s)
             // Add some extra padding to reduce the chance of having to resize
             // again later.
-            mBuffer.resize(3 * s);
+            mBuffer.resize(3 * size);
 
         // And make sure the string is zero terminated
-        mBuffer[s] = 0;
+        mBuffer[size] = 0;
 
         // read ESM data
         char* ptr = mBuffer.data();
         getExact(ptr, size);
 
-        size = static_cast<int>(strnlen(ptr, size));
+        size = strnlen(ptr, size);
 
         // Convert to UTF8 and return
-        if (mEncoder)
+        if (mEncoder != nullptr)
-            return ESM::RefId::stringRefId(mEncoder->getUtf8(std::string_view(ptr, size)));
+            return mEncoder->getUtf8(std::string_view(ptr, size));
 
-        return ESM::RefId::stringRefId(std::string_view(ptr, size));
+        return std::string_view(ptr, size);
     }
 
     [[noreturn]] void ESMReader::fail(const std::string& msg)

components/esm3/esmreader.hpp

@@ -174,6 +174,7 @@ namespace ESM
 
         // Read a string, including the sub-record header (but not the name)
         std::string getHString();
+        std::string_view getHStringView();
         RefId getRefId();
 
         void skipHString();
@@ -269,10 +270,11 @@ namespace ESM
         void getName(NAME& name) { getT(name); }
         void getUint(uint32_t& u) { getT(u); }
 
+        std::string getMaybeFixedStringSize(std::size_t size);
+
         // Read the next 'size' bytes and return them as a string. Converts
         // them from native encoding to UTF8 in the process.
-        std::string getString(int size);
+        std::string_view getStringView(std::size_t size);
-        ESM::RefId getRefId(int size);
 
         void skip(std::size_t bytes)
         {

components/esm3/esmwriter.cpp

@@ -167,15 +167,26 @@ namespace ESM
         endRecord(name);
     }
 
-    void ESMWriter::writeFixedSizeString(const std::string& data, std::size_t size)
+    void ESMWriter::writeMaybeFixedSizeString(const std::string& data, std::size_t size)
     {
         std::string string;
         if (!data.empty())
             string = mEncoder ? mEncoder->getLegacyEnc(data) : data;
-        if (string.size() > size)
+        if (mHeader.mFormatVersion <= MaxLimitedSizeStringsFormatVersion)
-            throw std::runtime_error("Fixed string data is too long: \"" + string + "\" ("
+        {
-                + std::to_string(string.size()) + " > " + std::to_string(size) + ")");
+            if (string.size() > size)
-        string.resize(size);
+                throw std::runtime_error("Fixed string data is too long: \"" + string + "\" ("
+                    + std::to_string(string.size()) + " > " + std::to_string(size) + ")");
+            string.resize(size);
+        }
+        else
+        {
+            constexpr StringSizeType maxSize = std::numeric_limits<StringSizeType>::max();
+            if (string.size() > maxSize)
+                throw std::runtime_error("String size is too long: \"" + string.substr(0, 64) + "<...>\" ("
+                    + std::to_string(string.size()) + " > " + std::to_string(maxSize) + ")");
+            writeT(static_cast<StringSizeType>(string.size()));
+        }
         write(string.c_str(), string.size());
     }
 

components/esm3/esmwriter.hpp

@@ -136,7 +136,7 @@ namespace ESM
         void startSubRecord(NAME name);
         void endRecord(NAME name);
         void endRecord(uint32_t name);
-        void writeFixedSizeString(const std::string& data, std::size_t size);
+        void writeMaybeFixedSizeString(const std::string& data, std::size_t size);
         void writeHString(const std::string& data);
         void writeHCString(const std::string& data);
         void writeName(NAME data);

components/esm3/formatversion.hpp

@@ -19,7 +19,8 @@ namespace ESM
     inline constexpr FormatVersion MaxOldAiPackageFormatVersion = 17;
     inline constexpr FormatVersion MaxOldSkillsAndAttributesFormatVersion = 18;
     inline constexpr FormatVersion MaxOldCreatureStatsFormatVersion = 19;
-    inline constexpr FormatVersion CurrentSaveGameFormatVersion = 22;
+    inline constexpr FormatVersion MaxLimitedSizeStringsFormatVersion = 22;
+    inline constexpr FormatVersion CurrentSaveGameFormatVersion = 23;
 }
 
 #endif

components/esm3/loadcont.cpp

@@ -12,7 +12,7 @@ namespace ESM
         esm.getSubHeader();
         ContItem ci;
         esm.getT(ci.mCount);
-        ci.mItem = ESM::RefId::stringRefId(esm.getString(32));
+        ci.mItem = ESM::RefId::stringRefId(esm.getMaybeFixedStringSize(32));
         mList.push_back(ci);
     }
 
@@ -22,7 +22,7 @@ namespace ESM
         {
             esm.startSubRecord("NPCO");
             esm.writeT(it->mCount);
-            esm.writeFixedSizeString(it->mItem.getRefIdString(), 32);
+            esm.writeMaybeFixedSizeString(it->mItem.getRefIdString(), 32);
             esm.endRecord("NPCO");
         }
     }

components/esm3/loadregn.cpp

@@ -53,7 +53,7 @@ namespace ESM
                 {
                     esm.getSubHeader();
                     SoundRef sr;
-                    sr.mSound = ESM::RefId::stringRefId(esm.getString(32));
+                    sr.mSound = ESM::RefId::stringRefId(esm.getMaybeFixedStringSize(32));
                     esm.getT(sr.mChance);
                     mSoundList.push_back(sr);
                     break;
@@ -95,7 +95,7 @@ namespace ESM
         for (std::vector<SoundRef>::const_iterator it = mSoundList.begin(); it != mSoundList.end(); ++it)
         {
             esm.startSubRecord("SNAM");
-            esm.writeFixedSizeString(it->mSound.getRefIdString(), 32);
+            esm.writeMaybeFixedSizeString(it->mSound.getRefIdString(), 32);
             esm.writeT(it->mChance);
             esm.endRecord("SNAM");
         }

components/esm3/loadscpt.cpp

@@ -98,7 +98,7 @@ namespace ESM
                 case fourCC("SCHD"):
                 {
                     esm.getSubHeader();
-                    mId = ESM::RefId::stringRefId(esm.getString(32));
+                    mId = ESM::RefId::stringRefId(esm.getMaybeFixedStringSize(32));
                     esm.getT(mData);
 
                     hasHeader = true;
@@ -152,7 +152,7 @@ namespace ESM
                 varNameString.append(*it);
 
         esm.startSubRecord("SCHD");
-        esm.writeFixedSizeString(mId.getRefIdString(), 32);
+        esm.writeMaybeFixedSizeString(mId.getRefIdString(), 32);
         esm.writeT(mData, 20);
         esm.endRecord("SCHD");
 

components/esm3/loadtes3.cpp

@@ -30,8 +30,8 @@ namespace ESM
             esm.getSubHeader();
             esm.getT(mData.version);
             esm.getT(mData.type);
-            mData.author.assign(esm.getString(32));
+            mData.author.assign(esm.getMaybeFixedStringSize(32));
-            mData.desc.assign(esm.getString(256));
+            mData.desc.assign(esm.getMaybeFixedStringSize(256));
             esm.getT(mData.records);
         }
 
@@ -71,8 +71,8 @@ namespace ESM
         esm.startSubRecord("HEDR");
         esm.writeT(mData.version);
         esm.writeT(mData.type);
-        esm.writeFixedSizeString(mData.author, 32);
+        esm.writeMaybeFixedSizeString(mData.author, 32);
-        esm.writeFixedSizeString(mData.desc, 256);
+        esm.writeMaybeFixedSizeString(mData.desc, 256);
         esm.writeT(mData.records);
         esm.endRecord("HEDR");