Skip to content

Commit

Permalink
Fixed out of bound read in FindMember() and added related new APIs
Browse files Browse the repository at this point in the history 
The original FindMember() may access out-of-bound of the 'const char*
name' parameter.
This commit firstly follows
pah/rapidjson@f86af8c

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
  • Loading branch information
@miloyip
miloyip committed Jun 20, 2014
1 parent 0a56e64 commit 02673be
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 26 deletions.
107 changes: 81 additions & 26 deletions include/rapidjson/document.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class GenericValue {
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();
}
Expand Down Expand Up @@ -234,7 +234,7 @@ class GenericValue {
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
Expand All @@ -244,6 +244,19 @@ class GenericValue {
}
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.
ConstMemberIterator MemberBegin() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members; }
ConstMemberIterator MemberEnd() const { RAPIDJSON_ASSERT(IsObject()); return data_.o.members + data_.o.size; }
Expand All @@ -256,22 +269,40 @@ class GenericValue {
*/
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;
}
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 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.
/*! \param name A string value as name of member.
Expand All @@ -283,6 +314,7 @@ class GenericValue {
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) {
Expand Down Expand Up @@ -324,26 +356,49 @@ class GenericValue {
\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;
}
else
return false;
}

bool RemoveMember(const GenericValue& name) {
MemberIterator m = FindMember(name);
if (m) {
RemoveMember(m);
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;
}

//@}
Expand Down Expand Up @@ -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);
}
Expand Down
23 changes: 23 additions & 0 deletions test/unittest/valuetest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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
Expand All @@ -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()
Expand All @@ -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()
Expand Down

0 comments on commit 02673be

Please sign in to comment.