From f1ba61c7ba5989373880f14f80d2b56f8593eb81 Mon Sep 17 00:00:00 2001 From: "Philipp A. Hartmann" Date: Sun, 9 Jul 2017 14:31:29 +0200 Subject: [PATCH 1/2] unittest.h: change RAPIDJSON_ASSERT to allow usage in expressions --- test/unittest/unittest.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unittest/unittest.h b/test/unittest/unittest.h index 5837345..aa091aa 100644 --- a/test/unittest/unittest.h +++ b/test/unittest/unittest.h @@ -117,7 +117,7 @@ public: #pragma GCC diagnostic pop #endif -#define RAPIDJSON_ASSERT(x) if (!(x)) throw AssertException(RAPIDJSON_STRINGIFY(x)) +#define RAPIDJSON_ASSERT(x) (!(x) ? throw AssertException(RAPIDJSON_STRINGIFY(x)) : (void)0u) class Random { public: From 47c3c1ec9f5c3724c5befb42f8323e760acc1f69 Mon Sep 17 00:00:00 2001 From: "Philipp A. Hartmann" Date: Sun, 9 Jul 2017 14:46:59 +0200 Subject: [PATCH 2/2] Improved handling of NULL strings * Safely assert upon passing NULL string without length (requires usage of RAPIDJSON_ASSERT within an expression) * Allow using a NULL string together with an explicit length 0 (GenericStringRef, GenericValue::SetString, ...), see #817 * Add GenericValue::SetString(StringRefType, Allocator&) overload * Add tests for the various cases --- include/rapidjson/document.h | 26 +++++++++++++++++----- test/unittest/valuetest.cpp | 43 +++++++++++++++++++++++++++++++++--- 2 files changed, 60 insertions(+), 9 deletions(-) diff --git a/include/rapidjson/document.h b/include/rapidjson/document.h index 57f0b3c..06451ad 100644 --- a/include/rapidjson/document.h +++ b/include/rapidjson/document.h @@ -308,7 +308,7 @@ struct GenericStringRef { */ #endif explicit GenericStringRef(const CharType* str) - : s(str), length(internal::StrLen(str)){ RAPIDJSON_ASSERT(s != 0); } + : s(str), length(((RAPIDJSON_ASSERT(str != 0)), internal::StrLen(str))) {} //! Create constant string reference from pointer and length #ifndef __clang__ // -Wdocumentation @@ -320,7 +320,7 @@ struct GenericStringRef { */ #endif GenericStringRef(const CharType* str, SizeType len) - : s(str), length(len) { RAPIDJSON_ASSERT(s != 0); } + : s(RAPIDJSON_LIKELY(str) ? str : emptyString), length(len) { RAPIDJSON_ASSERT(str != 0 || len == 0u); } GenericStringRef(const GenericStringRef& rhs) : s(rhs.s), length(rhs.length) {} @@ -331,6 +331,9 @@ struct GenericStringRef { const SizeType length; //!< length of the string (excluding the trailing NULL terminator) private: + /// Empty string - used when passing in a NULL pointer + static const Ch emptyString[]; + //! Disallow construction from non-const array template GenericStringRef(CharType (&str)[N]) /* = delete */; @@ -338,6 +341,9 @@ private: GenericStringRef& operator=(const GenericStringRef& rhs) /* = delete */; }; +template +const CharType GenericStringRef::emptyString[] = { CharType() }; + //! Mark a character pointer as constant string /*! Mark a plain character pointer as a "string literal". This function can be used to avoid copying a character string to be referenced as a @@ -352,7 +358,7 @@ private: */ template inline GenericStringRef StringRef(const CharType* str) { - return GenericStringRef(str, internal::StrLen(str)); + return GenericStringRef(str); } //! Mark a character pointer as constant string @@ -1762,7 +1768,7 @@ public: \return The value itself for fluent API. \post IsString() == true && GetString() != s && strcmp(GetString(),s) == 0 && GetStringLength() == length */ - GenericValue& SetString(const Ch* s, SizeType length, Allocator& allocator) { this->~GenericValue(); SetStringRaw(StringRef(s, length), allocator); return *this; } + GenericValue& SetString(const Ch* s, SizeType length, Allocator& allocator) { return SetString(StringRef(s, length), allocator); } //! Set this value as a string by copying from source string. /*! \param s source string. @@ -1770,7 +1776,15 @@ public: \return The value itself for fluent API. \post IsString() == true && GetString() != s && strcmp(GetString(),s) == 0 && GetStringLength() == length */ - GenericValue& SetString(const Ch* s, Allocator& allocator) { return SetString(s, internal::StrLen(s), allocator); } + GenericValue& SetString(const Ch* s, Allocator& allocator) { return SetString(StringRef(s), allocator); } + + //! Set this value as a string by copying from source string. + /*! \param s source string reference + \param allocator Allocator for allocating copied buffer. Commonly use GenericDocument::GetAllocator(). + \return The value itself for fluent API. + \post IsString() == true && GetString() != s.s && strcmp(GetString(),s) == 0 && GetStringLength() == length + */ + GenericValue& SetString(StringRefType s, Allocator& allocator) { this->~GenericValue(); SetStringRaw(s, allocator); return *this; } #if RAPIDJSON_HAS_STDSTRING //! Set this value as a string by copying from source string. @@ -1780,7 +1794,7 @@ public: \post IsString() == true && GetString() != s.data() && strcmp(GetString(),s.data() == 0 && GetStringLength() == s.size() \note Requires the definition of the preprocessor symbol \ref RAPIDJSON_HAS_STDSTRING. */ - GenericValue& SetString(const std::basic_string& s, Allocator& allocator) { return SetString(s.data(), SizeType(s.size()), allocator); } + GenericValue& SetString(const std::basic_string& s, Allocator& allocator) { return SetString(StringRef(s), allocator); } #endif //@} diff --git a/test/unittest/valuetest.cpp b/test/unittest/valuetest.cpp index fefc001..307e1b0 100644 --- a/test/unittest/valuetest.cpp +++ b/test/unittest/valuetest.cpp @@ -857,9 +857,46 @@ TEST(Value, String) { } // Issue 226: Value of string type should not point to NULL -TEST(Value, SetStringNullException) { - Value v; - EXPECT_THROW(v.SetString(0, 0), AssertException); +TEST(Value, SetStringNull) { + + MemoryPoolAllocator<> allocator; + const char* nullPtr = 0; + { + // Construction with string type creates empty string + Value v(kStringType); + EXPECT_NE(v.GetString(), nullPtr); // non-null string returned + EXPECT_EQ(v.GetStringLength(), 0u); + + // Construction from/setting to null without length not allowed + EXPECT_THROW(Value(StringRef(nullPtr)), AssertException); + EXPECT_THROW(Value(StringRef(nullPtr), allocator), AssertException); + EXPECT_THROW(v.SetString(nullPtr, allocator), AssertException); + + // Non-empty length with null string is not allowed + EXPECT_THROW(v.SetString(nullPtr, 17u), AssertException); + EXPECT_THROW(v.SetString(nullPtr, 42u, allocator), AssertException); + + // Setting to null string with empty length is allowed + v.SetString(nullPtr, 0u); + EXPECT_NE(v.GetString(), nullPtr); // non-null string returned + EXPECT_EQ(v.GetStringLength(), 0u); + + v.SetNull(); + v.SetString(nullPtr, 0u, allocator); + EXPECT_NE(v.GetString(), nullPtr); // non-null string returned + EXPECT_EQ(v.GetStringLength(), 0u); + } + // Construction with null string and empty length is allowed + { + Value v(nullPtr,0u); + EXPECT_NE(v.GetString(), nullPtr); // non-null string returned + EXPECT_EQ(v.GetStringLength(), 0u); + } + { + Value v(nullPtr, 0u, allocator); + EXPECT_NE(v.GetString(), nullPtr); // non-null string returned + EXPECT_EQ(v.GetStringLength(), 0u); + } } template