From a0e5e68fdbf123d378f45a709315311f04a51594 Mon Sep 17 00:00:00 2001 From: "Philipp A. Hartmann" Date: Fri, 28 Feb 2014 12:09:24 +0100 Subject: [PATCH 1/3] GenericDocument::Accept: deep-copy strings, if needed Instead of always just shallowly referencing the potentially allocated strings when calling the Handler::String function, request a copy in case the string has been allocated from an Allocator before. This is necessary to avoid double free()s of the string memory, especially when using the Handler to create a deep copy of a Value. The explicit comparison against '0' is done to suppress the warning C4800 on MSVC, see pah/rapidjson#5. --- include/rapidjson/document.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/rapidjson/document.h b/include/rapidjson/document.h index 8f2417b..9102915 100644 --- a/include/rapidjson/document.h +++ b/include/rapidjson/document.h @@ -579,7 +579,7 @@ int z = a[0u].GetInt(); // This works too. case kObjectType: handler.StartObject(); for (ConstMemberIterator m = MemberBegin(); m != MemberEnd(); ++m) { - handler.String(m->name.data_.s.str, m->name.data_.s.length, false); + handler.String(m->name.data_.s.str, m->name.data_.s.length, (m->name.flags_ & kCopyFlag) != 0); m->value.Accept(handler); } handler.EndObject(data_.o.size); @@ -593,7 +593,7 @@ int z = a[0u].GetInt(); // This works too. break; case kStringType: - handler.String(data_.s.str, data_.s.length, false); + handler.String(data_.s.str, data_.s.length, (flags_ & kCopyFlag) != 0); break; case kNumberType: From 8bde3be116e587f384b03720198ad287d70e8ecb Mon Sep 17 00:00:00 2001 From: "Philipp A. Hartmann" Date: Fri, 28 Feb 2014 13:01:57 +0100 Subject: [PATCH 2/3] GenericValue: add copy constructor and CopyFrom To allow deep copying from an existing GenericValue, an explicit "copy constructor" (with required Allocator param) and an "CopyFrom" assignment function are added. Document d; Document::AllocatorType& a = d.GetAllocator(); Value v1("foo"); // Value v2(v1); // not allowed Value v2(v1,a); // make a copy RAPIDJSON_ASSERT(v1.IsString()); // v1 untouched d.SetArray().PushBack(v1,a).PushBack(v2,a); RAPIDJSON_ASSERT(v1.Empty() && v2.Empty()); v2.CopyFrom(d,a); // copy whole document RAPIDJSON_ASSERT(d.IsArray() && d.Size()); // d untouched v1.SetObject().AddMember( "array", v2, a ); d.PushBack(v1,a); Additionally, the Handler implementation in GenericDocument is made private again, restricting access to GenericReader and GenericValue. --- include/rapidjson/document.h | 42 ++++++++++++++++++++++++++++++++++-- 1 file changed, 40 insertions(+), 2 deletions(-) diff --git a/include/rapidjson/document.h b/include/rapidjson/document.h index 9102915..40a0442 100644 --- a/include/rapidjson/document.h +++ b/include/rapidjson/document.h @@ -69,6 +69,16 @@ public: flags_ = defaultFlags[type]; } + //! Explicit copy constructor (with allocator) + /*! Creates a copy of a Value by using the given Allocator + \tparam SourceAllocator allocator of \c rhs + \param rhs Value to copy from (read-only) + \param allocator Allocator to use for copying + \see CopyFrom() + */ + template< typename SourceAllocator > + GenericValue(const GenericValue& rhs, Allocator & allocator); + //! Constructor for boolean value. explicit GenericValue(bool b) : flags_(b ? kTrueFlag : kFalseFlag) {} @@ -183,6 +193,21 @@ public: new (this) GenericValue(value); return *this; } + + //! Deep-copy assignment from Value + /*! Assigns a \b copy of the Value to the current Value object + \tparam SourceAllocator Allocator type of \c rhs + \param rhs Value to copy from (read-only) + \param allocator Allocator to use for copying + */ + template + GenericValue& CopyFrom(const GenericValue& rhs, Allocator& allocator) { + RAPIDJSON_ASSERT((void*)this != (void*)&rhs); + this->~GenericValue(); + new (this) GenericValue(rhs,allocator); + return *this; + } + //@} //!@name Type @@ -840,8 +865,10 @@ public: //! Get the capacity of stack in bytes. size_t GetStackCapacity() const { return stack_.GetCapacity(); } -//private: - //friend class GenericReader; // for Reader to call the following private handler functions +private: + // callers of the following private Handler functions + template friend class GenericReader; // for parsing + friend class GenericValue; // for deep copying // Implementation of Handler void Null() { new (stack_.template Push()) ValueType(); } @@ -893,6 +920,17 @@ private: typedef GenericDocument > Document; +// defined here due to the dependency on GenericDocument +template +template +inline +GenericValue::GenericValue(const GenericValue& rhs, Allocator& allocator) +{ + GenericDocument d(&allocator); + rhs.Accept(d); + RawAssign(*d.stack_.template Pop(1)); +} + } // namespace rapidjson #ifdef _MSC_VER From 65b4316da95f759cb9aa0477293d5611149e1667 Mon Sep 17 00:00:00 2001 From: "Philipp A. Hartmann" Date: Fri, 28 Feb 2014 13:11:21 +0100 Subject: [PATCH 3/3] valuetest: add deep copy unit test This commit adds some simple tests for the deep-copying of values, either based on the explicit constructor, or the CopyFrom function. It uses the CrtAllocator to test for possible double-free errors due to insufficient copying. --- test/unittest/valuetest.cpp | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/test/unittest/valuetest.cpp b/test/unittest/valuetest.cpp index 6800b7b..d1bf63c 100644 --- a/test/unittest/valuetest.cpp +++ b/test/unittest/valuetest.cpp @@ -25,6 +25,36 @@ TEST(Value, assignment_operator) { EXPECT_EQ(1234, y.GetInt()); } +TEST(Value, CopyFrom) +{ + // use CrtAllocator to explicitly malloc/free any memory + // comment this line to use the default Allocator instead + typedef GenericValue,CrtAllocator> Value; + + Value::AllocatorType a; + Value v1(1234); + Value v2(v1,a); // deep copy constructor + EXPECT_TRUE(v1.GetType() == v2.GetType()); + EXPECT_EQ(v1.GetInt(), v2.GetInt()); + + v1.SetString("foo"); + v2.CopyFrom(v1,a); + EXPECT_TRUE(v1.GetType() == v2.GetType()); + EXPECT_STREQ(v1.GetString(), v2.GetString()); + EXPECT_EQ(v1.GetString(), v2.GetString()); // string NOT copied + + v1.SetArray().PushBack(1234,a); + v2.CopyFrom(v1,a); + EXPECT_TRUE(v2.IsArray()); + EXPECT_EQ(v1.Size(), v2.Size()); + + v1.PushBack(Value().SetString("foo",a),a); // push string copy + EXPECT_TRUE(v1.Size() != v2.Size()); + v2.CopyFrom(v1,a); + EXPECT_TRUE(v1.Size() == v2.Size()); + EXPECT_STREQ(v1[1].GetString(), v2[1].GetString()); + EXPECT_NE(v1[1].GetString(), v2[1].GetString()); // string got copied +} TEST(Value, Null) { // Default constructor