From 04852627b286b4d4c26cb773450cda69b818661a Mon Sep 17 00:00:00 2001
From: Martchus <martchus@gmx.net>
Date: Fri, 8 Mar 2019 17:40:08 +0100
Subject: [PATCH] Fix binary serializiation of shared_ptr too big for variable
 length encoding

* Retain backward compatibility by using either variable length int
  or regular 64-bit int extending the flags
* Add test for both cases
---
 lib/binary/reflector.h        | 21 ++++++++++++++------
 lib/tests/binaryreflector.cpp | 37 +++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/lib/binary/reflector.h b/lib/binary/reflector.h
index 79691ba..e57b862 100644
--- a/lib/binary/reflector.h
+++ b/lib/binary/reflector.h
@@ -107,15 +107,15 @@ template <typename Type, Traits::EnableIf<Traits::IsSpecializationOf<Type, std::
 
 template <typename Type, Traits::EnableIf<Traits::IsSpecializationOf<Type, std::shared_ptr>> *> void BinaryDeserializer::read(Type &pointer)
 {
-    const auto occurence = readByte();
-    if (!occurence) {
+    auto mode = readByte();
+    if (!mode) {
         // pointer not set
         pointer.reset();
         return;
     }
 
-    const auto id = readVariableLengthUIntBE();
-    if (occurence == 1) {
+    const auto id = (mode & 0x4) ? readUInt64BE() : readVariableLengthUIntBE(); // the 3rd bit being flagged indicates a big ID
+    if ((mode & 0x3) == 1) {
         // first occurence: make a new pointer
         m_pointer[id] = pointer = std::make_shared<typename Type::element_type>();
         read(*pointer);
@@ -200,9 +200,18 @@ template <typename Type, Traits::EnableIf<Traits::IsSpecializingAnyOf<Type, std:
         return;
     }
     const auto id = reinterpret_cast<uint64>(pointer.get());
+    const auto bigId = id >= 0x80000000000000;
     auto &alreadyWritten = m_pointer[id];
-    writeByte(alreadyWritten ? 2 : 1);
-    writeVariableLengthUIntBE(id);
+    byte mode = alreadyWritten ? 2 : 1;
+    if (bigId) {
+        mode = mode | 0x4; // "flag" 3rd bit to indicate big ID
+    }
+    writeByte(mode);
+    if (bigId) {
+        writeUInt64BE(id);
+    } else {
+        writeVariableLengthUIntBE(id);
+    }
     if (!alreadyWritten) {
         alreadyWritten = true;
         write(*pointer);
diff --git a/lib/tests/binaryreflector.cpp b/lib/tests/binaryreflector.cpp
index 58f3f03..56af157 100644
--- a/lib/tests/binaryreflector.cpp
+++ b/lib/tests/binaryreflector.cpp
@@ -12,6 +12,7 @@ using TestUtilities::operator<<; // must be visible prior to the call site
 #include <cppunit/extensions/HelperMacros.h>
 
 #include <iostream>
+#include <limits>
 #include <map>
 #include <sstream>
 #include <string>
@@ -137,6 +138,8 @@ class BinaryReflectorTests : public TestFixture {
     CPPUNIT_TEST(testDeserializeSimpleStruct);
     CPPUNIT_TEST(testSerializeNestedStruct);
     CPPUNIT_TEST(testDeserializeNestedStruct);
+    CPPUNIT_TEST(testSmallSharedPointer);
+    CPPUNIT_TEST(testBigSharedPointer);
     CPPUNIT_TEST_SUITE_END();
 
 public:
@@ -150,6 +153,9 @@ public:
     void testSerializeNestedStruct();
     void testDeserializeNestedStruct();
     void assertTestObject(const TestObjectBinary &deserialized);
+    void testSharedPointer(std::uintptr_t fakePointer);
+    void testSmallSharedPointer();
+    void testBigSharedPointer();
 
 private:
     vector<unsigned char> m_buffer;
@@ -292,3 +298,34 @@ void BinaryReflectorTests::assertTestObject(const TestObjectBinary &deserialized
     CPPUNIT_ASSERT_EQUAL(m_testObj.someUnorderedSet, deserialized.someUnorderedSet);
     CPPUNIT_ASSERT_EQUAL(m_testObj.someUnorderedMultiset, deserialized.someUnorderedMultiset);
 }
+
+void BinaryReflectorTests::testSharedPointer(uintptr_t fakePointer)
+{
+    // create a shared pointer for the fake pointer ensuring that it is not actually deleted
+    shared_ptr<int> sharedPointer(reinterpret_cast<int *>(fakePointer), [](int *) {});
+
+    // setup stream
+    stringstream stream(ios_base::in | ios_base::out | ios_base::binary);
+    stream.exceptions(ios_base::failbit | ios_base::badbit);
+
+    // serialize the shared pointer assuming its contents have been written before (to prevent actually dereferencing it)
+    BinaryReflector::BinarySerializer serializer(&stream);
+    serializer.m_pointer[fakePointer] = true;
+    serializer.write(sharedPointer);
+
+    // deserialize the shared pointer assuming it has already been read and the type matches
+    deserializer.m_pointer[fakePointer] = make_shared<int>(42);
+    deserializer.read(readPtr);
+    CPPUNIT_ASSERT(readPtr != nullptr);
+    CPPUNIT_ASSERT_EQUAL(42, *readPtr);
+}
+
+void BinaryReflectorTests::testSmallSharedPointer()
+{
+    testSharedPointer(std::numeric_limits<std::uintptr_t>::min() + 1);
+}
+
+void BinaryReflectorTests::testBigSharedPointer()
+{
+    testSharedPointer(std::numeric_limits<std::uintptr_t>::max());
+}