From c033292aeaf014accfb5b4054d8fdac36358266c Mon Sep 17 00:00:00 2001 From: ylavic Date: Sat, 27 Apr 2019 03:41:19 +0200 Subject: [PATCH 1/3] Safer GenericValue& operator=(GenericValue& rhs). When rhs is a sub-Value of *this, destroying *this also destroys/frees rhs, thus the following RawAssign(rhs) crashes. Address this by saving/moving rhs to a temporary first, which clears rhs and avoids its destruction with *this. The crash can be reproduced in test Value.MergeDuplicateKey by using the CrtAllocator instead of the default Document's MemoryPoolAllocator. --- include/rapidjson/document.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/include/rapidjson/document.h b/include/rapidjson/document.h index 0910122..61d031b 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; } From 50cb424c348fe82814196e9ccc04e7efb69d82ca Mon Sep 17 00:00:00 2001 From: ylavic Date: Mon, 15 Mar 2021 23:56:55 +0100 Subject: [PATCH 2/3] Test assignment from inner Value. --- test/unittest/valuetest.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/unittest/valuetest.cpp b/test/unittest/valuetest.cpp index 00f0652..f34d4b0 100644 --- a/test/unittest/valuetest.cpp +++ b/test/unittest/valuetest.cpp @@ -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, CrtAllocator> nullValue; + GenericValue, CrtAllocator> arrayValue(kArrayType); + arrayValue.PushBack(nullValue, a); + arrayValue = arrayValue[0]; // shouldn't crash (use after free) + EXPECT_TRUE(arrayValue.IsNull()); + } } TEST(Value, ArrayHelper) { From 117276c4135b0a3fbc0641a5c9f4d1ad9b44d785 Mon Sep 17 00:00:00 2001 From: ylavic Date: Sat, 27 Apr 2019 03:55:23 +0200 Subject: [PATCH 3/3] Fix would-crash tests if the default allocator used were kNeedFree. The allocator cannot be destroyed before the Document, otherwise the Value destructor double frees. --- test/unittest/documenttest.cpp | 2 ++ test/unittest/valuetest.cpp | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/test/unittest/documenttest.cpp b/test/unittest/documenttest.cpp index 472165f..c3d1e48 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 f34d4b0..0a6b325 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()); @@ -1491,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());