Merge branch 'esm3_variable_string_size' into 'master'

Support variable size strings in ESM3

See merge request OpenMW/openmw!2728

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/CMakeLists.txt

@@ -91,6 +91,7 @@ file(GLOB UNITTEST_SRC_FILES
 
     esm3/readerscache.cpp
     esm3/testsaveload.cpp
+    esm3/testesmwriter.cpp
 
     nifosg/testnifloader.cpp
 )

apps/openmw_test_suite/esm3/testesmwriter.cpp

@@ -0,0 +1,54 @@
+#include <components/esm3/esmwriter.hpp>
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+#include <algorithm>
+#include <memory>
+#include <random>
+
+namespace ESM
+{
+    namespace
+    {
+        using namespace ::testing;
+
+        struct Esm3EsmWriterTest : public Test
+        {
+            std::minstd_rand mRandom;
+            std::uniform_int_distribution<short> mRefIdDistribution{ 'a', 'z' };
+
+            std::string generateRandomString(std::size_t size)
+            {
+                std::string result;
+                std::generate_n(
+                    std::back_inserter(result), size, [&] { return static_cast<char>(mRefIdDistribution(mRandom)); });
+                return result;
+            }
+        };
+
+        TEST_F(Esm3EsmWriterTest, saveShouldThrowExceptionOnWhenTruncatingHeaderStrings)
+        {
+            const std::string author = generateRandomString(33);
+            const std::string description = generateRandomString(257);
+
+            std::stringstream stream;
+
+            ESMWriter writer;
+            writer.setAuthor(author);
+            writer.setDescription(description);
+            writer.setFormatVersion(MaxLimitedSizeStringsFormatVersion);
+            EXPECT_THROW(writer.save(stream), std::runtime_error);
+        }
+
+        TEST_F(Esm3EsmWriterTest, writeFixedStringShouldThrowExceptionOnTruncate)
+        {
+            std::stringstream stream;
+
+            ESMWriter writer;
+            writer.setFormatVersion(MaxLimitedSizeStringsFormatVersion);
+            writer.save(stream);
+            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

@@ -144,6 +144,11 @@ namespace ESM
     }
 
     std::string ESMReader::getHString()
+    {
+        return std::string(getHStringView());
+    }
+
+    std::string_view ESMReader::getHStringView()
     {
         getSubHeader();
 
@@ -158,31 +163,15 @@ namespace ESM
             mCtx.leftRec--;
             char c;
             getT(c);
-            return std::string();
+            return std::string_view();
         }
 
-        return getString(mCtx.leftSub);
+        return getStringView(mCtx.leftSub);
     }
 
     RefId ESMReader::getRefId()
     {
-        getSubHeader();
-
-        // 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 ESM::RefId::sEmpty;
-        }
-
-        return getRefId(mCtx.leftSub);
+        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 (mBuffer.size() <= s)
-            // Add some extra padding to reduce the chance of having to resize
-            // again later.
-            mBuffer.resize(3 * s);
-
-        // And make sure the string is zero terminated
-        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)));
+        if (mHeader.mFormatVersion > MaxLimitedSizeStringsFormatVersion)
+        {
+            StringSizeType storedSize = 0;
+            getT(storedSize);
+            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);
+        }
 
-        return std::string(ptr, size);
+        return std::string(getStringView(size));
     }
 
-    ESM::RefId ESMReader::getRefId(int size)
+    std::string_view ESMReader::getStringView(std::size_t size)
     {
-        size_t s = size;
-        if (mBuffer.size() <= s)
+        if (mBuffer.size() <= size)
             // 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)
-            return ESM::RefId::stringRefId(mEncoder->getUtf8(std::string_view(ptr, size)));
+        if (mEncoder != nullptr)
+            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);
-        ESM::RefId getRefId(int size);
+        std::string_view getStringView(std::size_t size);
 
         void skip(std::size_t bytes)
         {

components/esm3/esmwriter.cpp

@@ -167,12 +167,26 @@ namespace ESM
         endRecord(name);
     }
 
-    void ESMWriter::writeFixedSizeString(const std::string& data, int size)
+    void ESMWriter::writeMaybeFixedSizeString(const std::string& data, std::size_t size)
     {
         std::string string;
         if (!data.empty())
             string = mEncoder ? mEncoder->getLegacyEnc(data) : data;
-        string.resize(size);
+        if (mHeader.mFormatVersion <= MaxLimitedSizeStringsFormatVersion)
+        {
+            if (string.size() > 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

@@ -44,7 +44,7 @@ namespace ESM
         // Counts how many records we have actually written.
         // It is a good idea to compare this with the value you wrote into the header (setRecordCount)
         // It should be the record count you set + 1 (1 additional record for the TES3 header)
-        int getRecordCount() { return mRecordCount; }
+        int getRecordCount() const { return mRecordCount; }
         void setFormatVersion(FormatVersion value);
 
         void clearMaster();
@@ -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, int 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.desc.assign(esm.getString(256));
+            mData.author.assign(esm.getMaybeFixedStringSize(32));
+            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.writeFixedSizeString(mData.desc, 256);
+        esm.writeMaybeFixedSizeString(mData.author, 32);
+        esm.writeMaybeFixedSizeString(mData.desc, 256);
         esm.writeT(mData.records);
         esm.endRecord("HEDR");