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

@@ -1,3 +1,4 @@
+#include <components/esm/fourcc.hpp>
 #include <components/esm3/esmreader.hpp>
 #include <components/esm3/esmwriter.hpp>
 #include <components/esm3/variant.hpp>
@@ -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<std::istringstream>(data), "");
+        reader.open(std::make_unique<std::istringstream>(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<WriteToESMTestCase>
@@ -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<WriteToESMTestCase>
-    {
+    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<WriteToESMTestCase>
     {

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

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<std::streamsize>(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<std::streamsize>(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<std::streamsize>(leftRec);
         getUint(flags); // This header entry is always zero
         getUint(flags);
         mCtx.leftFile -= 3 * sizeof(uint32_t);