Commit 02673bec authored by Milo Yip's avatar Milo Yip

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
parent 0a56e649
...@@ -144,7 +144,7 @@ public: ...@@ -144,7 +144,7 @@ public:
break; break;
case kObjectFlag: 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->name.~GenericValue();
m->value.~GenericValue(); m->value.~GenericValue();
} }
...@@ -234,7 +234,7 @@ public: ...@@ -234,7 +234,7 @@ public:
A better approach is to use the now public FindMember(). A better approach is to use the now public FindMember().
*/ */
GenericValue& operator[](const Ch* name) { GenericValue& operator[](const Ch* name) {
if (Member* member = FindMember(name)) if (MemberIterator member = FindMember(name))
return member->value; return member->value;
else { else {
RAPIDJSON_ASSERT(false); // see above note RAPIDJSON_ASSERT(false); // see above note
...@@ -244,6 +244,19 @@ public: ...@@ -244,6 +244,19 @@ public:
} }
const GenericValue& operator[](const Ch* name) const { return const_cast<GenericValue&>(*this)[name]; } const GenericValue& operator[](const Ch* name) const { return const_cast<GenericValue&>(*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<GenericValue&>(*this)[name]; }
//! Member iterators. //! Member iterators.
ConstMemberIterator MemberBegin() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members; } ConstMemberIterator MemberBegin() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members; }
ConstMemberIterator MemberEnd() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members + data_.o.size; } ConstMemberIterator MemberEnd() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members + data_.o.size; }
...@@ -256,22 +269,40 @@ public: ...@@ -256,22 +269,40 @@ public:
*/ */
bool HasMember(const Ch* name) const { return FindMember(name) != 0; } 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. //! Find member by name.
/*! /*!
\return Return the member if exists. Otherwise returns null pointer. \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(name);
RAPIDJSON_ASSERT(IsObject()); RAPIDJSON_ASSERT(IsObject());
Object& o = data_.o; SizeType len = internal::StrLen(name);
for (Member* member = o.members; member != data_.o.members + data_.o.size; ++member) for (MemberIterator member = MemberBegin(); member != MemberEnd(); ++member)
if (name[member->name.data_.s.length] == '\0' && memcmp(member->name.data_.s.str, name, member->name.data_.s.length * sizeof(Ch)) == 0) if (member->name.data_.s.length == len && memcmp(member->name.data_.s.str, name, len * sizeof(Ch)) == 0)
return member;
return 0;
}
ConstMemberIterator FindMember(const Ch* name) const { return const_cast<GenericValue&>(*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 member;
return 0; return 0;
} }
const Member* FindMember(const Ch* name) const { return const_cast<GenericValue&>(*this).FindMember(name); } ConstMemberIterator FindMember(const GenericValue& name) const { return const_cast<GenericValue&>(*this).FindMember(name); }
//! Add a member (name-value pair) to the object. //! Add a member (name-value pair) to the object.
/*! \param name A string value as name of member. /*! \param name A string value as name of member.
...@@ -283,6 +314,7 @@ public: ...@@ -283,6 +314,7 @@ public:
GenericValue& AddMember(GenericValue& name, GenericValue& value, Allocator& allocator) { GenericValue& AddMember(GenericValue& name, GenericValue& value, Allocator& allocator) {
RAPIDJSON_ASSERT(IsObject()); RAPIDJSON_ASSERT(IsObject());
RAPIDJSON_ASSERT(name.IsString()); RAPIDJSON_ASSERT(name.IsString());
Object& o = data_.o; Object& o = data_.o;
if (o.size >= o.capacity) { if (o.size >= o.capacity) {
if (o.capacity == 0) { if (o.capacity == 0) {
...@@ -324,26 +356,49 @@ public: ...@@ -324,26 +356,49 @@ public:
\note Removing member is implemented by moving the last member. So the ordering of members is changed. \note Removing member is implemented by moving the last member. So the ordering of members is changed.
*/ */
bool RemoveMember(const Ch* name) { bool RemoveMember(const Ch* name) {
RAPIDJSON_ASSERT(IsObject()); MemberIterator m = FindMember(name);
if (Member* m = FindMember(name)) { if (m) {
RAPIDJSON_ASSERT(data_.o.size > 0); RemoveMember(m);
RAPIDJSON_ASSERT(data_.o.members != 0); return true;
}
Member* last = data_.o.members + (data_.o.size - 1); else
if (data_.o.size > 1 && m != last) { return false;
// Move the last one to this place }
m->name = last->name;
m->value = last->value; bool RemoveMember(const GenericValue& name) {
} MemberIterator m = FindMember(name);
else { if (m) {
// Only one left, just destroy RemoveMember(m);
m->name.~GenericValue();
m->value.~GenericValue();
}
--data_.o.size;
return true; return true;
} }
return false; 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. ...@@ -524,7 +579,7 @@ int z = a[0u].GetInt(); // This works too.
case kObjectType: case kObjectType:
handler.StartObject(); 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); handler.String(m->name.data_.s.str, m->name.data_.s.length, false);
m->value.Accept(handler); m->value.Accept(handler);
} }
......
...@@ -470,19 +470,31 @@ TEST(Value, Object) { ...@@ -470,19 +470,31 @@ TEST(Value, Object) {
value.SetString("Banana", 6); value.SetString("Banana", 6);
x.AddMember(name, value, allocator); 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() // HasMember()
EXPECT_TRUE(x.HasMember("A")); EXPECT_TRUE(x.HasMember("A"));
EXPECT_TRUE(x.HasMember("B")); EXPECT_TRUE(x.HasMember("B"));
EXPECT_TRUE(y.HasMember("A")); EXPECT_TRUE(y.HasMember("A"));
EXPECT_TRUE(y.HasMember("B")); EXPECT_TRUE(y.HasMember("B"));
name.SetString("C\0D", 3);
EXPECT_TRUE(x.HasMember(name));
EXPECT_TRUE(y.HasMember(name));
// operator[] // operator[]
EXPECT_STREQ("Apple", x["A"].GetString()); EXPECT_STREQ("Apple", x["A"].GetString());
EXPECT_STREQ("Banana", x["B"].GetString()); EXPECT_STREQ("Banana", x["B"].GetString());
EXPECT_STREQ("CherryD", x[C0D].GetString());
// const operator[] // const operator[]
EXPECT_STREQ("Apple", y["A"].GetString()); EXPECT_STREQ("Apple", y["A"].GetString());
EXPECT_STREQ("Banana", y["B"].GetString()); EXPECT_STREQ("Banana", y["B"].GetString());
EXPECT_STREQ("CherryD", y[C0D].GetString());
// member iterator // member iterator
Value::MemberIterator itr = x.MemberBegin(); Value::MemberIterator itr = x.MemberBegin();
...@@ -494,6 +506,10 @@ TEST(Value, Object) { ...@@ -494,6 +506,10 @@ TEST(Value, Object) {
EXPECT_STREQ("B", itr->name.GetString()); EXPECT_STREQ("B", itr->name.GetString());
EXPECT_STREQ("Banana", itr->value.GetString()); EXPECT_STREQ("Banana", itr->value.GetString());
++itr; ++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()); EXPECT_FALSE(itr != x.MemberEnd());
// const member iterator // const member iterator
...@@ -506,6 +522,10 @@ TEST(Value, Object) { ...@@ -506,6 +522,10 @@ TEST(Value, Object) {
EXPECT_STREQ("B", citr->name.GetString()); EXPECT_STREQ("B", citr->name.GetString());
EXPECT_STREQ("Banana", citr->value.GetString()); EXPECT_STREQ("Banana", citr->value.GetString());
++citr; ++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()); EXPECT_FALSE(citr != y.MemberEnd());
// RemoveMember() // RemoveMember()
...@@ -515,6 +535,9 @@ TEST(Value, Object) { ...@@ -515,6 +535,9 @@ TEST(Value, Object) {
x.RemoveMember("B"); x.RemoveMember("B");
EXPECT_FALSE(x.HasMember("B")); EXPECT_FALSE(x.HasMember("B"));
x.RemoveMember(name);
EXPECT_FALSE(x.HasMember(name));
EXPECT_TRUE(x.MemberBegin() == x.MemberEnd()); EXPECT_TRUE(x.MemberBegin() == x.MemberEnd());
// SetObject() // SetObject()
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment