Merge pull request #1503 from ylavic/sub_value_assignment

Fix (Sub-)Value assignment

3 files changed, 20 insertions, 3 deletions

diff --git a/include/rapidjson/document.h b/include/rapidjson/document.h
index 09101229..61d031bd 100644
--- a/include/rapidjson/document.h
+++ b/include/rapidjson/document.h
@@ -916,8 +916,13 @@ public:
     */
     GenericValue& operator=(GenericValue& rhs) RAPIDJSON_NOEXCEPT {
         if (RAPIDJSON_LIKELY(this != &rhs)) {
+            // Can't destroy "this" before assigning "rhs", otherwise "rhs"
+            // could be used after free if it's an sub-Value of "this",
+            // hence the temporary danse.
+            GenericValue temp;
+            temp.RawAssign(rhs);
             this->~GenericValue();
-            RawAssign(rhs);
+            RawAssign(temp);
         }
         return *this;
     }
diff --git a/test/unittest/documenttest.cpp b/test/unittest/documenttest.cpp
index 472165fd..c3d1e484 100644
--- a/test/unittest/documenttest.cpp
+++ b/test/unittest/documenttest.cpp
@@ -325,6 +325,8 @@ TEST(Document, Swap) {
     EXPECT_TRUE(d1.IsNull());
 
     // reset document, including allocator
+    // so clear o before so that it doesnt contain dangling elements
+    o.Clear();
     Document().Swap(d2);
     EXPECT_TRUE(d2.IsNull());
     EXPECT_NE(&d2.GetAllocator(), &a);
diff --git a/test/unittest/valuetest.cpp b/test/unittest/valuetest.cpp
index 00f06528..0a6b325f 100644
--- a/test/unittest/valuetest.cpp
+++ b/test/unittest/valuetest.cpp
@@ -1078,9 +1078,9 @@ static void TestArray(T& x, Allocator& allocator) {
 }
 
 TEST(Value, Array) {
+    Value::AllocatorType allocator;
     Value x(kArrayType);
     const Value& y = x;
-    Value::AllocatorType allocator;
 
     EXPECT_EQ(kArrayType, x.GetType());
     EXPECT_TRUE(x.IsArray());
@@ -1119,6 +1119,16 @@ TEST(Value, Array) {
     z.SetArray();
     EXPECT_TRUE(z.IsArray());
     EXPECT_TRUE(z.Empty());
+
+    // PR #1503: assign from inner Value
+    {
+        CrtAllocator a; // Free() is not a noop
+        GenericValue<UTF8<>, CrtAllocator> nullValue;
+        GenericValue<UTF8<>, CrtAllocator> arrayValue(kArrayType);
+        arrayValue.PushBack(nullValue, a);
+        arrayValue = arrayValue[0]; // shouldn't crash (use after free)
+        EXPECT_TRUE(arrayValue.IsNull());
+    }
 }
 
 TEST(Value, ArrayHelper) {
@@ -1481,9 +1491,9 @@ static void TestObject(T& x, Allocator& allocator) {
 }
 
 TEST(Value, Object) {
+    Value::AllocatorType allocator;
     Value x(kObjectType);
     const Value& y = x; // const version
-    Value::AllocatorType allocator;
 
     EXPECT_EQ(kObjectType, x.GetType());
     EXPECT_TRUE(x.IsObject());