From c69148c9468b02835803480fd204121f06c7530a Mon Sep 17 00:00:00 2001 From: Andrey Okoshkin Date: Fri, 12 Jan 2018 11:26:34 +0300 Subject: [PATCH 1/3] Fix Value::copyPayload() and Value::copy() (#704) Value copy constructor shares the same code with Value::copy() and Value::copyPayload(). New Value::releasePayload() is used to free payload memory. Fixes: #704 --- include/json/value.h | 1 + src/lib_json/json_value.cpp | 131 +++++++++++++++++++----------------- 2 files changed, 69 insertions(+), 63 deletions(-) diff --git a/include/json/value.h b/include/json/value.h index d1bf8ff..1643210 100644 --- a/include/json/value.h +++ b/include/json/value.h @@ -606,6 +606,7 @@ Json::Value obj_value(Json::objectValue); // {} private: void initBasic(ValueType type, bool allocated = false); + void releasePayload(); Value& resolveReference(const char* key); Value& resolveReference(const char* key, const char* end); diff --git a/src/lib_json/json_value.cpp b/src/lib_json/json_value.cpp index 91d4802..eb7b6b0 100644 --- a/src/lib_json/json_value.cpp +++ b/src/lib_json/json_value.cpp @@ -441,48 +441,11 @@ Value::Value(bool value) { value_.bool_ = value; } -Value::Value(Value const& other) - : type_(other.type_), allocated_(false) - , - comments_(0), start_(other.start_), limit_(other.limit_) -{ - switch (type_) { - case nullValue: - case intValue: - case uintValue: - case realValue: - case booleanValue: - value_ = other.value_; - break; - case stringValue: - if (other.value_.string_ && other.allocated_) { - unsigned len; - char const* str; - decodePrefixedString(other.allocated_, other.value_.string_, - &len, &str); - value_.string_ = duplicateAndPrefixStringValue(str, len); - allocated_ = true; - } else { - value_.string_ = other.value_.string_; - allocated_ = false; - } - break; - case arrayValue: - case objectValue: - value_.map_ = new ObjectValues(*other.value_.map_); - break; - default: - JSON_ASSERT_UNREACHABLE; - } - if (other.comments_) { - comments_ = new CommentInfo[numberOfCommentPlacement]; - for (int comment = 0; comment < numberOfCommentPlacement; ++comment) { - const CommentInfo& otherComment = other.comments_[comment]; - if (otherComment.comment_) - comments_[comment].setComment( - otherComment.comment_, strlen(otherComment.comment_)); - } - } +Value::Value(const Value& other) { + type_ = nullValue; + allocated_ = false; + comments_ = 0; + copy(other); } #if JSON_HAS_RVALUE_REFERENCES @@ -494,24 +457,7 @@ Value::Value(Value&& other) { #endif Value::~Value() { - switch (type_) { - case nullValue: - case intValue: - case uintValue: - case realValue: - case booleanValue: - break; - case stringValue: - if (allocated_) - releasePrefixedStringValue(value_.string_); - break; - case arrayValue: - case objectValue: - delete value_.map_; - break; - default: - JSON_ASSERT_UNREACHABLE; - } + releasePayload(); delete[] comments_; @@ -534,9 +480,36 @@ void Value::swapPayload(Value& other) { } void Value::copyPayload(const Value& other) { + releasePayload(); type_ = other.type_; - value_ = other.value_; - allocated_ = other.allocated_; + allocated_ = false; + switch (type_) { + case nullValue: + case intValue: + case uintValue: + case realValue: + case booleanValue: + value_ = other.value_; + break; + case stringValue: + if (other.value_.string_ && other.allocated_) { + unsigned len; + char const* str; + decodePrefixedString(other.allocated_, other.value_.string_, + &len, &str); + value_.string_ = duplicateAndPrefixStringValue(str, len); + allocated_ = true; + } else { + value_.string_ = other.value_.string_; + } + break; + case arrayValue: + case objectValue: + value_.map_ = new ObjectValues(*other.value_.map_); + break; + default: + JSON_ASSERT_UNREACHABLE; + } } void Value::swap(Value& other) { @@ -548,7 +521,18 @@ void Value::swap(Value& other) { void Value::copy(const Value& other) { copyPayload(other); - comments_ = other.comments_; + delete[] comments_; + if (other.comments_) { + comments_ = new CommentInfo[numberOfCommentPlacement]; + for (int comment = 0; comment < numberOfCommentPlacement; ++comment) { + const CommentInfo& otherComment = other.comments_[comment]; + if (otherComment.comment_) + comments_[comment].setComment( + otherComment.comment_, strlen(otherComment.comment_)); + } + } else { + comments_ = 0; + } start_ = other.start_; limit_ = other.limit_; } @@ -1049,6 +1033,27 @@ void Value::initBasic(ValueType vtype, bool allocated) { limit_ = 0; } +void Value::releasePayload() { + switch (type_) { + case nullValue: + case intValue: + case uintValue: + case realValue: + case booleanValue: + break; + case stringValue: + if (allocated_) + releasePrefixedStringValue(value_.string_); + break; + case arrayValue: + case objectValue: + delete value_.map_; + break; + default: + JSON_ASSERT_UNREACHABLE; + } +} + // Access an object value by name, create a null member if it does not exist. // @pre Type of '*this' is object or null. // @param key is null-terminated. From 392e3a5b49cd63554694d57f3f4bd24b49dbb108 Mon Sep 17 00:00:00 2001 From: Andrey Okoshkin Date: Fri, 12 Jan 2018 11:27:11 +0300 Subject: [PATCH 2/3] Add basic test for Value::copy() (#704) --- src/test_lib_json/main.cpp | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/test_lib_json/main.cpp b/src/test_lib_json/main.cpp index d00e107..ee81790 100644 --- a/src/test_lib_json/main.cpp +++ b/src/test_lib_json/main.cpp @@ -1433,6 +1433,40 @@ JSONTEST_FIXTURE(ValueTest, compareType) { Json::Value(Json::objectValue))); } +JSONTEST_FIXTURE(ValueTest, CopyObject) { + Json::Value arrayVal; + arrayVal.append("val1"); + arrayVal.append("val2"); + arrayVal.append("val3"); + Json::Value stringVal("string value"); + Json::Value copy1, copy2; + { + Json::Value arrayCopy, stringCopy; + arrayCopy.copy(arrayVal); + stringCopy.copy(stringVal); + JSONTEST_ASSERT_PRED(checkIsEqual(arrayCopy, arrayVal)); + JSONTEST_ASSERT_PRED(checkIsEqual(stringCopy, stringVal)); + arrayCopy.append("val4"); + JSONTEST_ASSERT(arrayCopy.size() == 4); + arrayVal.append("new4"); + arrayVal.append("new5"); + JSONTEST_ASSERT(arrayVal.size() == 5); + JSONTEST_ASSERT(!(arrayCopy == arrayVal)); + stringCopy = "another string"; + JSONTEST_ASSERT(!(stringCopy == stringVal)); + copy1.copy(arrayCopy); + copy2.copy(stringCopy); + } + JSONTEST_ASSERT(arrayVal.size() == 5); + JSONTEST_ASSERT(stringVal == "string value"); + JSONTEST_ASSERT(copy1.size() == 4); + JSONTEST_ASSERT(copy2 == "another string"); + copy1.copy(stringVal); + JSONTEST_ASSERT(copy1 == "string value"); + copy2.copy(arrayVal); + JSONTEST_ASSERT(copy2.size() == 5); +} + void ValueTest::checkIsLess(const Json::Value& x, const Json::Value& y) { JSONTEST_ASSERT(x < y); JSONTEST_ASSERT(y > x); @@ -2544,6 +2578,7 @@ int main(int argc, const char* argv[]) { JSONTEST_REGISTER_FIXTURE(runner, ValueTest, compareArray); JSONTEST_REGISTER_FIXTURE(runner, ValueTest, compareObject); JSONTEST_REGISTER_FIXTURE(runner, ValueTest, compareType); + JSONTEST_REGISTER_FIXTURE(runner, ValueTest, CopyObject); JSONTEST_REGISTER_FIXTURE(runner, ValueTest, offsetAccessors); JSONTEST_REGISTER_FIXTURE(runner, ValueTest, typeChecksThrowExceptions); JSONTEST_REGISTER_FIXTURE(runner, ValueTest, StaticString); From 9b569c8ce38419fa8ad98de4db27a349ecdaf3d0 Mon Sep 17 00:00:00 2001 From: Andrey Okoshkin Date: Fri, 12 Jan 2018 15:59:20 +0300 Subject: [PATCH 3/3] Make Value copy constructor simplier Helper private methods Value::dupPayload() and Value::dupMeta() are added. Value copy constructor doesn't attempt to delete its data first. * Value::dupPayload() duplicates a payload. * Value::dupMeta() duplicates comments and an offset position with a limit. --- include/json/value.h | 2 + src/lib_json/json_value.cpp | 98 ++++++++++++++++++++----------------- 2 files changed, 54 insertions(+), 46 deletions(-) diff --git a/include/json/value.h b/include/json/value.h index 1643210..bcf3675 100644 --- a/include/json/value.h +++ b/include/json/value.h @@ -606,7 +606,9 @@ Json::Value obj_value(Json::objectValue); // {} private: void initBasic(ValueType type, bool allocated = false); + void dupPayload(const Value& other); void releasePayload(); + void dupMeta(const Value& other); Value& resolveReference(const char* key); Value& resolveReference(const char* key, const char* end); diff --git a/src/lib_json/json_value.cpp b/src/lib_json/json_value.cpp index eb7b6b0..30449fd 100644 --- a/src/lib_json/json_value.cpp +++ b/src/lib_json/json_value.cpp @@ -442,10 +442,8 @@ Value::Value(bool value) { } Value::Value(const Value& other) { - type_ = nullValue; - allocated_ = false; - comments_ = 0; - copy(other); + dupPayload(other); + dupMeta(other); } #if JSON_HAS_RVALUE_REFERENCES @@ -481,35 +479,7 @@ void Value::swapPayload(Value& other) { void Value::copyPayload(const Value& other) { releasePayload(); - type_ = other.type_; - allocated_ = false; - switch (type_) { - case nullValue: - case intValue: - case uintValue: - case realValue: - case booleanValue: - value_ = other.value_; - break; - case stringValue: - if (other.value_.string_ && other.allocated_) { - unsigned len; - char const* str; - decodePrefixedString(other.allocated_, other.value_.string_, - &len, &str); - value_.string_ = duplicateAndPrefixStringValue(str, len); - allocated_ = true; - } else { - value_.string_ = other.value_.string_; - } - break; - case arrayValue: - case objectValue: - value_.map_ = new ObjectValues(*other.value_.map_); - break; - default: - JSON_ASSERT_UNREACHABLE; - } + dupPayload(other); } void Value::swap(Value& other) { @@ -522,19 +492,7 @@ void Value::swap(Value& other) { void Value::copy(const Value& other) { copyPayload(other); delete[] comments_; - if (other.comments_) { - comments_ = new CommentInfo[numberOfCommentPlacement]; - for (int comment = 0; comment < numberOfCommentPlacement; ++comment) { - const CommentInfo& otherComment = other.comments_[comment]; - if (otherComment.comment_) - comments_[comment].setComment( - otherComment.comment_, strlen(otherComment.comment_)); - } - } else { - comments_ = 0; - } - start_ = other.start_; - limit_ = other.limit_; + dupMeta(other); } ValueType Value::type() const { return type_; } @@ -1033,6 +991,38 @@ void Value::initBasic(ValueType vtype, bool allocated) { limit_ = 0; } +void Value::dupPayload(const Value& other) { + type_ = other.type_; + allocated_ = false; + switch (type_) { + case nullValue: + case intValue: + case uintValue: + case realValue: + case booleanValue: + value_ = other.value_; + break; + case stringValue: + if (other.value_.string_ && other.allocated_) { + unsigned len; + char const* str; + decodePrefixedString(other.allocated_, other.value_.string_, + &len, &str); + value_.string_ = duplicateAndPrefixStringValue(str, len); + allocated_ = true; + } else { + value_.string_ = other.value_.string_; + } + break; + case arrayValue: + case objectValue: + value_.map_ = new ObjectValues(*other.value_.map_); + break; + default: + JSON_ASSERT_UNREACHABLE; + } +} + void Value::releasePayload() { switch (type_) { case nullValue: @@ -1054,6 +1044,22 @@ void Value::releasePayload() { } } +void Value::dupMeta(const Value& other) { + if (other.comments_) { + comments_ = new CommentInfo[numberOfCommentPlacement]; + for (int comment = 0; comment < numberOfCommentPlacement; ++comment) { + const CommentInfo& otherComment = other.comments_[comment]; + if (otherComment.comment_) + comments_[comment].setComment( + otherComment.comment_, strlen(otherComment.comment_)); + } + } else { + comments_ = 0; + } + start_ = other.start_; + limit_ = other.limit_; +} + // Access an object value by name, create a null member if it does not exist. // @pre Type of '*this' is object or null. // @param key is null-terminated.