From 02673bec74967dcd108be1a77d9e8841b38d1aa0 Mon Sep 17 00:00:00 2001 From: Milo Yip Date: Fri, 20 Jun 2014 19:14:45 +0800 Subject: [PATCH] Fixed out of bound read in FindMember() and added related new APIs The original FindMember() may access out-of-bound of the 'const char* name' parameter. This commit firstly follows https://github.com/pah/rapidjson/commit/f86af8c232dc280ae510b119018d66ca08b82312 However, this must incur an StrLen() for name. A better API is by using Value as the name, which provides the length of string internally. So a set of new API are added: operator[](const GenericValue& name) FindMember(const GenericValue& name) RemoveMember(const GenericValue& name) During refactoring, it also adds an API: RemoveMember(MemberIterator m) which can be used for other purpose, such as removing a member while iterating an object. Fixes #7 --- include/rapidjson/document.h | 107 ++++++++++++++++++++++++++--------- test/unittest/valuetest.cpp | 23 ++++++++ 2 files changed, 104 insertions(+), 26 deletions(-) diff --git a/include/rapidjson/document.h b/include/rapidjson/document.h index 5524241..b7f18bd 100644 --- a/include/rapidjson/document.h +++ b/include/rapidjson/document.h @@ -144,7 +144,7 @@ public: break; case kObjectFlag: - for (Member* m = data_.o.members; m != data_.o.members + data_.o.size; ++m) { + for (MemberIterator m = MemberBegin(); m != MemberEnd(); ++m) { m->name.~GenericValue(); m->value.~GenericValue(); } @@ -234,7 +234,7 @@ public: A better approach is to use the now public FindMember(). */ GenericValue& operator[](const Ch* name) { - if (Member* member = FindMember(name)) + if (MemberIterator member = FindMember(name)) return member->value; else { RAPIDJSON_ASSERT(false); // see above note @@ -244,6 +244,19 @@ public: } const GenericValue& operator[](const Ch* name) const { return const_cast(*this)[name]; } + // This version is faster because it does not need a StrLen(). + // It can also handle string with null character. + GenericValue& operator[](const GenericValue& name) { + if (Member* member = FindMember(name)) + return member->value; + else { + RAPIDJSON_ASSERT(false); // see above note + static GenericValue NullValue; + return NullValue; + } + } + const GenericValue& operator[](const GenericValue& name) const { return const_cast(*this)[name]; } + //! Member iterators. ConstMemberIterator MemberBegin() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members; } ConstMemberIterator MemberEnd() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members + data_.o.size; } @@ -256,22 +269,40 @@ public: */ bool HasMember(const Ch* name) const { return FindMember(name) != 0; } + // This version is faster because it does not need a StrLen(). + // It can also handle string with null character. + bool HasMember(const GenericValue& name) const { return FindMember(name) != 0; } + //! Find member by name. /*! \return Return the member if exists. Otherwise returns null pointer. */ - Member* FindMember(const Ch* name) { + MemberIterator FindMember(const Ch* name) { RAPIDJSON_ASSERT(name); RAPIDJSON_ASSERT(IsObject()); - Object& o = data_.o; - for (Member* member = o.members; member != data_.o.members + data_.o.size; ++member) - if (name[member->name.data_.s.length] == '\0' && memcmp(member->name.data_.s.str, name, member->name.data_.s.length * sizeof(Ch)) == 0) + SizeType len = internal::StrLen(name); + for (MemberIterator member = MemberBegin(); member != MemberEnd(); ++member) + if (member->name.data_.s.length == len && memcmp(member->name.data_.s.str, name, len * sizeof(Ch)) == 0) return member; return 0; } - const Member* FindMember(const Ch* name) const { return const_cast(*this).FindMember(name); } + ConstMemberIterator FindMember(const Ch* name) const { return const_cast(*this).FindMember(name); } + + // This version is faster because it does not need a StrLen(). + // It can also handle string with null character. + MemberIterator FindMember(const GenericValue& name) { + RAPIDJSON_ASSERT(IsObject()); + RAPIDJSON_ASSERT(name.IsString()); + SizeType len = name.data_.s.length; + for (MemberIterator member = MemberBegin(); member != MemberEnd(); ++member) + if (member->name.data_.s.length == len && memcmp(member->name.data_.s.str, name.data_.s.str, len * sizeof(Ch)) == 0) + return member; + + return 0; + } + ConstMemberIterator FindMember(const GenericValue& name) const { return const_cast(*this).FindMember(name); } //! Add a member (name-value pair) to the object. /*! \param name A string value as name of member. @@ -283,6 +314,7 @@ public: GenericValue& AddMember(GenericValue& name, GenericValue& value, Allocator& allocator) { RAPIDJSON_ASSERT(IsObject()); RAPIDJSON_ASSERT(name.IsString()); + Object& o = data_.o; if (o.size >= o.capacity) { if (o.capacity == 0) { @@ -324,26 +356,49 @@ public: \note Removing member is implemented by moving the last member. So the ordering of members is changed. */ bool RemoveMember(const Ch* name) { - RAPIDJSON_ASSERT(IsObject()); - if (Member* m = FindMember(name)) { - RAPIDJSON_ASSERT(data_.o.size > 0); - RAPIDJSON_ASSERT(data_.o.members != 0); - - Member* last = data_.o.members + (data_.o.size - 1); - if (data_.o.size > 1 && m != last) { - // Move the last one to this place - m->name = last->name; - m->value = last->value; - } - else { - // Only one left, just destroy - m->name.~GenericValue(); - m->value.~GenericValue(); - } - --data_.o.size; + MemberIterator m = FindMember(name); + if (m) { + RemoveMember(m); return true; } - return false; + else + return false; + } + + bool RemoveMember(const GenericValue& name) { + MemberIterator m = FindMember(name); + if (m) { + RemoveMember(m); + return true; + } + else + return false; + } + + //! Remove a member in object by iterator. + /*! \param m member iterator (obtained by FindMember() or MemberBegin()). + \return the new iterator after removal. + \note Removing member is implemented by moving the last member. So the ordering of members is changed. + */ + MemberIterator RemoveMember(MemberIterator m) { + RAPIDJSON_ASSERT(IsObject()); + RAPIDJSON_ASSERT(data_.o.size > 0); + RAPIDJSON_ASSERT(data_.o.members != 0); + RAPIDJSON_ASSERT(m >= MemberBegin() && m < MemberEnd()); + + MemberIterator last = data_.o.members + (data_.o.size - 1); + if (data_.o.size > 1 && m != last) { + // Move the last one to this place + m->name = last->name; + m->value = last->value; + } + else { + // Only one left, just destroy + m->name.~GenericValue(); + m->value.~GenericValue(); + } + --data_.o.size; + return m; } //@} @@ -524,7 +579,7 @@ int z = a[0u].GetInt(); // This works too. case kObjectType: handler.StartObject(); - for (Member* m = data_.o.members; m != data_.o.members + data_.o.size; ++m) { + for (ConstMemberIterator m = MemberBegin(); m != MemberEnd(); ++m) { handler.String(m->name.data_.s.str, m->name.data_.s.length, false); m->value.Accept(handler); } diff --git a/test/unittest/valuetest.cpp b/test/unittest/valuetest.cpp index 0507ca1..6800b7b 100644 --- a/test/unittest/valuetest.cpp +++ b/test/unittest/valuetest.cpp @@ -470,19 +470,31 @@ TEST(Value, Object) { value.SetString("Banana", 6); x.AddMember(name, value, allocator); + // Tests a member with null character + const Value C0D("C\0D", 3); + name.SetString(C0D.GetString(), 3); + value.SetString("CherryD", 7); + x.AddMember(name, value, allocator); + // HasMember() EXPECT_TRUE(x.HasMember("A")); EXPECT_TRUE(x.HasMember("B")); EXPECT_TRUE(y.HasMember("A")); EXPECT_TRUE(y.HasMember("B")); + name.SetString("C\0D", 3); + EXPECT_TRUE(x.HasMember(name)); + EXPECT_TRUE(y.HasMember(name)); + // operator[] EXPECT_STREQ("Apple", x["A"].GetString()); EXPECT_STREQ("Banana", x["B"].GetString()); + EXPECT_STREQ("CherryD", x[C0D].GetString()); // const operator[] EXPECT_STREQ("Apple", y["A"].GetString()); EXPECT_STREQ("Banana", y["B"].GetString()); + EXPECT_STREQ("CherryD", y[C0D].GetString()); // member iterator Value::MemberIterator itr = x.MemberBegin(); @@ -494,6 +506,10 @@ TEST(Value, Object) { EXPECT_STREQ("B", itr->name.GetString()); EXPECT_STREQ("Banana", itr->value.GetString()); ++itr; + EXPECT_TRUE(itr != x.MemberEnd()); + EXPECT_TRUE(memcmp(itr->name.GetString(), "C\0D", 4) == 0); + EXPECT_STREQ("CherryD", itr->value.GetString()); + ++itr; EXPECT_FALSE(itr != x.MemberEnd()); // const member iterator @@ -506,6 +522,10 @@ TEST(Value, Object) { EXPECT_STREQ("B", citr->name.GetString()); EXPECT_STREQ("Banana", citr->value.GetString()); ++citr; + EXPECT_TRUE(citr != y.MemberEnd()); + EXPECT_TRUE(memcmp(citr->name.GetString(), "C\0D", 4) == 0); + EXPECT_STREQ("CherryD", citr->value.GetString()); + ++citr; EXPECT_FALSE(citr != y.MemberEnd()); // RemoveMember() @@ -515,6 +535,9 @@ TEST(Value, Object) { x.RemoveMember("B"); EXPECT_FALSE(x.HasMember("B")); + x.RemoveMember(name); + EXPECT_FALSE(x.HasMember(name)); + EXPECT_TRUE(x.MemberBegin() == x.MemberEnd()); // SetObject()