Mojo cpp bindings: validation logic for incoming messages

Mojo cpp bindings: validation logic for incoming messages

BUG=None
TEST=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274005

[yzshen1, 2014-05-22 06:51:46]

Hi, Darin and Tom.

Would you please take a look? Thanks!

Things that are not included in this CL (TODO's):
- remove redundant validation during decoding.
- add more validation tests.
- add some logging

[Tom Sepez, 2014-05-22 19:39:20]

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_serialization.cc (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_serialization.cc:108: // Currently our
binding code cannot handle structs of smaller size or with
nit: TODO()

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_serialization.h (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_serialization.h:21: bool IsAligned(const
void* ptr);
nit: is aligned to what? 4? 8? maybe a better name here.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_serialization.h:39: // Checks whether
decoding the pointer will overflow and produce a poniter
nit: sp. poniter

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.cc (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:46: // Do a binary search to find
the smallest index i that satisfies
can we use std::binary_search() instead of re-inventing this?

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:76: if (index ==
static_cast<uint32_t>(-1))
is there a kInvalidHandle constant somewhere?

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.h (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:24: // [|data|, |data_num_bytes|)
specifies the valid memory range.
nit: not sure vertical bars help inside the [) range syntax, but I may be wrong.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:35: // Claims the specified
encoded handle (which is basically a handle index).
nit: blank line here.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:38: // handle.
nit: while absolutely true, this is a strange sentence, we may have to call out
kInvalidHandle specifically as the placeholder for a valid "invalid handle" vs.
invalid invalid handles.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:58: // This vector is sorted in
increasing order, with an even number of elements.
I think you'll want a vector of struct Range { uinptr_t begin; uinptr_t end } or
equivalent here otherwise the arithmetic in the .cc file will be more confusing.
 Having done so the data_begin_ and data_end_ above might be ranges, too.

[yzshen1, 2014-05-22 20:56:22]

Thanks, Tom! PTAL

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_serialization.cc (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_serialization.cc:108: // Currently our
binding code cannot handle structs of smaller size or with
On 2014/05/22 19:39:21, Tom Sepez wrote:
> nit: TODO()

Done. Now it becomes my work item! :)

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bindings_serialization.h (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_serialization.h:21: bool IsAligned(const
void* ptr);
On 2014/05/22 19:39:21, Tom Sepez wrote:
> nit: is aligned to what? 4? 8? maybe a better name here.

I agree that it will be more clear as you suggested.
However, it is more consistent with the two methods above this way.

I feel that I probably should rename either all or none of them.

What do you think?

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bindings_serialization.h:39: // Checks whether
decoding the pointer will overflow and produce a poniter
On 2014/05/22 19:39:21, Tom Sepez wrote:
> nit: sp. poniter

Done.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.cc (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:46: // Do a binary search to find
the smallest index i that satisfies
On 2014/05/22 19:39:21, Tom Sepez wrote:
> can we use std::binary_search() instead of re-inventing this?

std::binary_search() is used to check whether an element exists.
Here, as the comment says, the purpose is different: we need to find the
smallest index i that satisfies |begin| < |claimed_ranges_[i]|.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:76: if (index ==
static_cast<uint32_t>(-1))
On 2014/05/22 19:39:21, Tom Sepez wrote:
> is there a kInvalidHandle constant somewhere?

We have kInvalidHandleValue but it is the "decoded" invalid handle, whose value
is 0.
Here we are dealing with the "encoded" invalid handle, whose value is -1.
I added a new constants kEncodedInvalidHandleValue.
Thanks!

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.h (right):

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:24: // [|data|, |data_num_bytes|)
specifies the valid memory range.
On 2014/05/22 19:39:21, Tom Sepez wrote:
> nit: not sure vertical bars help inside the [) range syntax, but I may be
wrong.

Done, I removed them.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:35: // Claims the specified
encoded handle (which is basically a handle index).
On 2014/05/22 19:39:21, Tom Sepez wrote:
> nit: blank line here.

Done.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:38: // handle.
On 2014/05/22 19:39:21, Tom Sepez wrote:
> nit: while absolutely true, this is a strange sentence, we may have to call
out
> kInvalidHandle specifically as the placeholder for a valid "invalid handle"
vs.
> invalid invalid handles.

Revised the comment.

https://codereview.chromium.org/289333002/diff/110001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:58: // This vector is sorted in
increasing order, with an even number of elements.
On 2014/05/22 19:39:21, Tom Sepez wrote:
> I think you'll want a vector of struct Range { uinptr_t begin; uinptr_t end }
or
> equivalent here otherwise the arithmetic in the .cc file will be more
confusing.
>  Having done so the data_begin_ and data_end_ above might be ranges, too.

I did consider this alternative, but think that the current approach is a little
cleaner: the binary search in BoundsChecker::ClaimMemory() doesn't have to care
about range begin and end. It simply treats all elements of |claimed_ranges_| in
the same way. I think it results in less comparisons as well.

What do you think?

[Tom Sepez, 2014-05-22 21:03:05]

On 2014/05/22 19:39:21, Tom Sepez wrote:
> > I think you'll want a vector of struct Range { uinptr_t begin; uinptr_t end
}
> or
> > equivalent here otherwise the arithmetic in the .cc file will be more
> confusing.
> >  Having done so the data_begin_ and data_end_ above might be ranges, too.
> 
> I did consider this alternative, but think that the current approach is a
little
> cleaner: the binary search in BoundsChecker::ClaimMemory() doesn't have to
care
> about range begin and end. It simply treats all elements of |claimed_ranges_|
in
> the same way. I think it results in less comparisons as well.
> 
> What do you think?
I'll defer to the other reviewers.  Many thanks for doing this, though.

[Tom Sepez, 2014-05-23 18:18:34]

Few more thoughts ...

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.cc (right):

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:30: }
maybe in debug builds we want to assert that the invariant that the
claimed_range vector is sorted isn't violated.

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:48: // |begin| <
|claimed_ranges_[i]|. (The previous if-block guarantees that such
std::upper_bound(), no?

[Tom Sepez, 2014-05-23 18:32:34]

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.cc (right):

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:116: claimed_ranges_.insert(pos,
pair, pair + 2);
this could be really slow if ranges aren't strictly increasing, no?

[yzshen1, 2014-05-23 20:04:06]

Thanks, Tom! PTAL

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.cc (right):

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:30: }
On 2014/05/23 18:18:35, Tom Sepez wrote:
> maybe in debug builds we want to assert that the invariant that the
> claimed_range vector is sorted isn't violated.

Done.

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:48: // |begin| <
|claimed_ranges_[i]|. (The previous if-block guarantees that such
On 2014/05/23 18:18:35, Tom Sepez wrote:
> std::upper_bound(), no?

Sounds good. I didn't know about this choice. :)
std::upper_bounds() might come with a slight cost, but we probably shouldn't
reinvent the wheel.

Done.

https://codereview.chromium.org/289333002/diff/130001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.cc:116: claimed_ranges_.insert(pos,
pair, pair + 2);
On 2014/05/23 18:32:34, Tom Sepez wrote:
> this could be really slow if ranges aren't strictly increasing, no?

I think this should be okay, because in most cases:
- |claimed_ranges_| is relatively short.
- we append/merge to the end.

I feel that:
- using a container that is more efficient for inserting slows down searching.
- introducing a more complex data structure like a tree may just add overhead
without much gain.

If you have good ideas please let me know. Thanks!

[yzshen1, 2014-05-23 22:29:10]

Out of curiosity, I compared the performance of home-made binary search and
std::upper_bound (i.e., the difference between patchset#8 and patchset#9):

I repeated the BoundsCheckerTest.ClaimMemory() test for 10000 times. And here
are the results:
--------------------------------------
home-made binary search:
debug: 1748ms
release: 64ms
--------------------------------------
std::upper_bound:
debug: 3188ms
release: 64ms
--------------------------------------

In debug build, home-made binary search is almost twice as fast as
std::upper_bound.
In release build, the two's performance are mostly the same.

Conclusion: Now that std::upper_bound() doesn't slow down the release build, I
think it is good to use.

[Tom Sepez, 2014-05-23 23:09:19]

> I think this should be okay, because in most cases:
> - |claimed_ranges_| is relatively short.
> - we append/merge to the end.
> 
Well, if that's the case, then binary search above is also overkill, and we can
simply do reverse linear search, getting rid of the upper_bound() usage.

If we needed a tree, the simplest way to get one is either std::set or std::map.
 Probably std::map<uintptr_t, bool> so that you can use the bool to track the
difference between begin/end markers.  I was pondering this before ... but I'm
not sure we want to go here.

I'll defer to the elders.  Maybe you should ping Darin.

[chromium-reviews, 2014-05-26 08:34:05]

Thanks Tom! Please see inlined comments.

Darin: would you please take a look? Thanks!

On Fri, May 23, 2014 at 4:09 PM, <tsepez@chromium.org> wrote:

> I think this should be okay, because in most cases:
>> - |claimed_ranges_| is relatively short.
>> - we append/merge to the end.
>>
>
> Well, if that's the case, then binary search above is also overkill, and
> we can
> simply do reverse linear search, getting rid of the upper_bound() usage.
>

I still think it makes sense to do the binary search.
As you can see, the first thing we do is to check whether we need to
append/merge to the end. So that is as good as reverse linear search in the
majority of cases.
If we need to insert/merge in the middle, on average binary search is more
efficient. I don't think it is overkill, since binary search is really
simple algorithm. (Not to mention we use a stl call to do it. :) )

I considered something like list, but decided that vector is better. In our
case, element number is usually quite small. The cost of moving elements
for insertions (vector) is smaller than following pointers to iterate over
elements (list).

In order to demonstrate that, I tested the performance of using reversed
linear search + std::list. And here are the results:
(Running BoundsCheckerTest.ClaimMemory() test for 10000 times.)
------------------------------
debug: 2383 ms
release: 104 ms (60% slower than the vector implementation)
------------------------------

If we needed a tree, the simplest way to get one is either std::set or
> std::map.
>  Probably std::map<uintptr_t, bool> so that you can use the bool to track
> the
> difference between begin/end markers.  I was pondering this before ... but
> I'm
> not sure we want to go here.
> I'll defer to the elders.  Maybe you should ping Darin.


Using binary search tree (std::set / std::map) won't speed up searching: it
is mostly the same as binary search in a sorted vector, plus cost of
following pointers.
And I think the insertion performance is probably worse than vector (if the
number of elements is small). It needs to do things like rebalancing the
tree.

https://codereview.chromium.org/289333002/



-- 
Best regards,
Yuzhu Shen.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[darin (slow to review), 2014-05-26 19:17:46]

https://codereview.chromium.org/289333002/diff/150001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.h (right):

https://codereview.chromium.org/289333002/diff/150001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:20: // BoundsChecker is used to
validate object sizes, pointers and handle indexes
Can we restrict the serialization format in ways that might trivialize bounds
checking? What if we had defined rules for serialization that required 1) that
pointers can only be non-negative, 2) that pointed to objects are serialized in
ordinal order, 3) that serialization is depth-first. Then we could probably
implement bounds checking with a min-offset counter, right?

[yzshen1, 2014-05-26 21:30:56]

Thanks, Darin!

https://codereview.chromium.org/289333002/diff/150001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.h (right):

https://codereview.chromium.org/289333002/diff/150001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:20: // BoundsChecker is used to
validate object sizes, pointers and handle indexes
On 2014/05/26 19:17:47, darin wrote:
> Can we restrict the serialization format in ways that might trivialize bounds
> checking? What if we had defined rules for serialization that required 1) that
> pointers can only be non-negative,
We are already enforcing this. (The offset is defined as an unsigned integer.
That ensures we never get into a loop when validating/decoding a message.)

> 2) that pointed to objects are serialized in
> ordinal order, 3) that serialization is depth-first.
With regard to (2) and (3), I also considered this approach. (In the mail thread
"a corrupt message".)
At that time, I thought:
- We might want to preserve the flexibility of serialization. For example, with
the more-strict-serialization-format approach we usually need to do a clone to
arrange object orders before sending out a message. Maybe in some cases we would
like to avoid that? (I don't have good examples in mind, maybe for in-process
message passing?)

- Instead of enforcing the traversal order in the message format spec, maybe we
could make it some "best effort" behavior, and suggest bindings to follow the
rules whenever possible. The current cpp binding implementation follows these
rules for both validating and decoding, so BoundsChecker should be (almost) as
efficient as a min-offset counter.

That being said, I agree that the more-strict-serialization-format approach
results in simpler code. If we don't mind being a little less flexible, I am
fine switching to that approach. The change should be trivial.

What do you think?  Thanks!

> Then we could probably
> implement bounds checking with a min-offset counter, right?

[darin (slow to review), 2014-05-27 17:29:25]

On 2014/05/26 21:30:56, yzshen1 wrote:
> Thanks, Darin!
> 
>
https://codereview.chromium.org/289333002/diff/150001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/bounds_checker.h (right):
> 
>
https://codereview.chromium.org/289333002/diff/150001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/bounds_checker.h:20: // BoundsChecker is used to
> validate object sizes, pointers and handle indexes
> On 2014/05/26 19:17:47, darin wrote:
> > Can we restrict the serialization format in ways that might trivialize
bounds
> > checking? What if we had defined rules for serialization that required 1)
that
> > pointers can only be non-negative,
> We are already enforcing this. (The offset is defined as an unsigned integer.
> That ensures we never get into a loop when validating/decoding a message.)
> 
> > 2) that pointed to objects are serialized in
> > ordinal order, 3) that serialization is depth-first.
> With regard to (2) and (3), I also considered this approach. (In the mail
thread
> "a corrupt message".)
> At that time, I thought:
> - We might want to preserve the flexibility of serialization. For example,
with
> the more-strict-serialization-format approach we usually need to do a clone to
> arrange object orders before sending out a message. Maybe in some cases we
would
> like to avoid that? (I don't have good examples in mind, maybe for in-process
> message passing?)

The new bindings system requires a copy on the sender side.


> - Instead of enforcing the traversal order in the message format spec, maybe
we
> could make it some "best effort" behavior, and suggest bindings to follow the
> rules whenever possible. The current cpp binding implementation follows these
> rules for both validating and decoding, so BoundsChecker should be (almost) as
> efficient as a min-offset counter.

I see.


> That being said, I agree that the more-strict-serialization-format approach
> results in simpler code. If we don't mind being a little less flexible, I am
> fine switching to that approach. The change should be trivial.

I would probably try the stricter, simpler approach until we learn that we
need more flexibility. It is easier to add flexibility later, whereas it is
hard to take away flexibility.

-Darin

[Tom Sepez, 2014-05-27 17:50:36]

> I would probably try the stricter, simpler approach until we learn that we
> need more flexibility. It is easier to add flexibility later, whereas it is
> hard to take away flexibility.
> 
> -Darin

Sounds like we should keep the bounds_checker.h API, though ...

[yzshen1, 2014-05-28 08:10:37]

Hi, Darin and Tom.

I have updated the BoundsChecker and tests. PTAL. Thanks!

[Tom Sepez, 2014-05-28 17:12:51]

LGTM

https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.h (right):

https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:45: bool
IsWithinUnclaimedRange(const void* position, uint32_t num_bytes) const;
nit: Looks like when this is used outside of tests, its in a "if (!...)" clause,
and there's an implied "not" in the term "unclaimed". Maybe inverting it to be 
IsWithinClaimedRange() gets rid of some double or triple negatives.

[yzshen1, 2014-05-28 17:27:51]

https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/bounds_checker.h (right):

https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/bounds_checker.h:45: bool
IsWithinUnclaimedRange(const void* position, uint32_t num_bytes) const;
On 2014/05/28 17:12:52, Tom Sepez wrote:
> nit: Looks like when this is used outside of tests, its in a "if (!...)"
clause,
> and there's an implied "not" in the term "unclaimed". Maybe inverting it to be

> IsWithinClaimedRange() gets rid of some double or triple negatives.   

One issue is that the opposite of "within unclaimed range" is not "within
claimed range": it could be:
- contained inside / overlapping the claimed range.
- not entirely contained inside the valid memory range (the range that we passed
into the constructor).

One nice thing about IsWithinUnclaimedRange() is that it is more consistent with
ClaimMemory(). (If a call to the former returns true, a call to the latter will
succeed.)

What do you think? Thanks!

[darin (slow to review), 2014-05-28 19:50:12]

On 2014/05/28 17:27:51, yzshen1 wrote:
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> File mojo/public/cpp/bindings/lib/bounds_checker.h (right):
> 
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> mojo/public/cpp/bindings/lib/bounds_checker.h:45: bool
> IsWithinUnclaimedRange(const void* position, uint32_t num_bytes) const;
> On 2014/05/28 17:12:52, Tom Sepez wrote:
> > nit: Looks like when this is used outside of tests, its in a "if (!...)"
> clause,
> > and there's an implied "not" in the term "unclaimed". Maybe inverting it to
be
> 
> > IsWithinClaimedRange() gets rid of some double or triple negatives.   
> 
> One issue is that the opposite of "within unclaimed range" is not "within
> claimed range": it could be:
> - contained inside / overlapping the claimed range.
> - not entirely contained inside the valid memory range (the range that we
passed
> into the constructor).
> 
> One nice thing about IsWithinUnclaimedRange() is that it is more consistent
with
> ClaimMemory(). (If a call to the former returns true, a call to the latter
will
> succeed.)
> 
> What do you think? Thanks!

IsOverlappingWithClaimedRange or IntersectsClaimedRange?

[darin (slow to review), 2014-05-28 19:56:29]

On 2014/05/28 19:50:12, darin wrote:
> On 2014/05/28 17:27:51, yzshen1 wrote:
> >
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> > File mojo/public/cpp/bindings/lib/bounds_checker.h (right):
> > 
> >
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> > mojo/public/cpp/bindings/lib/bounds_checker.h:45: bool
> > IsWithinUnclaimedRange(const void* position, uint32_t num_bytes) const;
> > On 2014/05/28 17:12:52, Tom Sepez wrote:
> > > nit: Looks like when this is used outside of tests, its in a "if (!...)"
> > clause,
> > > and there's an implied "not" in the term "unclaimed". Maybe inverting it
to
> be
> > 
> > > IsWithinClaimedRange() gets rid of some double or triple negatives.   
> > 
> > One issue is that the opposite of "within unclaimed range" is not "within
> > claimed range": it could be:
> > - contained inside / overlapping the claimed range.
> > - not entirely contained inside the valid memory range (the range that we
> passed
> > into the constructor).
> > 
> > One nice thing about IsWithinUnclaimedRange() is that it is more consistent
> with
> > ClaimMemory(). (If a call to the former returns true, a call to the latter
> will
> > succeed.)
> > 
> > What do you think? Thanks!
> 
> IsOverlappingWithClaimedRange or IntersectsClaimedRange?

Oh, wait... I see that this function also checks that the given range stays
within
the bounds of the message too ("end <= data_end_"). Maybe you could just call
this
method IsValidRange, IsLegalRange, IsAllowedRange?

[yzshen1, 2014-05-28 20:08:08]

On 2014/05/28 19:50:12, darin wrote:
> On 2014/05/28 17:27:51, yzshen1 wrote:
> >
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> > File mojo/public/cpp/bindings/lib/bounds_checker.h (right):
> > 
> >
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> > mojo/public/cpp/bindings/lib/bounds_checker.h:45: bool
> > IsWithinUnclaimedRange(const void* position, uint32_t num_bytes) const;
> > On 2014/05/28 17:12:52, Tom Sepez wrote:
> > > nit: Looks like when this is used outside of tests, its in a "if (!...)"
> > clause,
> > > and there's an implied "not" in the term "unclaimed". Maybe inverting it
to
> be
> > 
> > > IsWithinClaimedRange() gets rid of some double or triple negatives.   
> > 
> > One issue is that the opposite of "within unclaimed range" is not "within
> > claimed range": it could be:
> > - contained inside / overlapping the claimed range.
> > - not entirely contained inside the valid memory range (the range that we
> passed
> > into the constructor).
> > 
> > One nice thing about IsWithinUnclaimedRange() is that it is more consistent
> with
> > ClaimMemory(). (If a call to the former returns true, a call to the latter
> will
> > succeed.)
> > 
> > What do you think? Thanks!
> 
> IsOverlappingWithClaimedRange or IntersectsClaimedRange?

I still feel that these names are not very accurate:
- the areas outside of the valid memory range is not considered neither
"claimed" nor "unclaimed". Currently, we describe the "unclaimed range" as
"greater than the max claimed address and contained inside the valid memory
range". These new names imply that the areas outside of the valid memory range
are "claimed". If we use them, we will need to redefine "unclaimed range",
because "greater than the max claimed address" is not true anymore.
- IsOverlappingWithClaimedRage()/IntersectsClaimedRange() will need to return
true for any empty range. That seems a little confusing. (Or we need to
introduce a separate check for that.)

What do you think? Thanks!

[yzshen1, 2014-05-28 20:12:09]

On 2014/05/28 19:56:29, darin wrote:
> On 2014/05/28 19:50:12, darin wrote:
> > On 2014/05/28 17:27:51, yzshen1 wrote:
> > >
> >
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> > > File mojo/public/cpp/bindings/lib/bounds_checker.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/289333002/diff/170001/mojo/public/cpp/binding...
> > > mojo/public/cpp/bindings/lib/bounds_checker.h:45: bool
> > > IsWithinUnclaimedRange(const void* position, uint32_t num_bytes) const;
> > > On 2014/05/28 17:12:52, Tom Sepez wrote:
> > > > nit: Looks like when this is used outside of tests, its in a "if (!...)"
> > > clause,
> > > > and there's an implied "not" in the term "unclaimed". Maybe inverting it
> to
> > be
> > > 
> > > > IsWithinClaimedRange() gets rid of some double or triple negatives.   
> > > 
> > > One issue is that the opposite of "within unclaimed range" is not "within
> > > claimed range": it could be:
> > > - contained inside / overlapping the claimed range.
> > > - not entirely contained inside the valid memory range (the range that we
> > passed
> > > into the constructor).
> > > 
> > > One nice thing about IsWithinUnclaimedRange() is that it is more
consistent
> > with
> > > ClaimMemory(). (If a call to the former returns true, a call to the latter
> > will
> > > succeed.)
> > > 
> > > What do you think? Thanks!
> > 
> > IsOverlappingWithClaimedRange or IntersectsClaimedRange?
> 
> Oh, wait... I see that this function also checks that the given range stays
> within
> the bounds of the message too ("end <= data_end_"). Maybe you could just call
> this
> method IsValidRange, IsLegalRange, IsAllowedRange?

I think IsValidRange sounds good to me. I will make the change.
Thanks!

[yzshen1, 2014-05-28 20:49:29]

Done.

I changed quite some comments to go with this renaming. Please take another
look. Thanks!

[Tom Sepez, 2014-05-28 21:03:48]

Still LGTM.

[darin (slow to review), 2014-05-28 23:47:20]

LGTM

[yzshen1, 2014-05-30 20:00:32]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2014-05-30 20:05:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/yzshen@chromium.org/289333002/230001

[commit-bot: I haz the power, 2014-05-31 04:07:06]

Change committed as 274005