Pickle: handle invalid data on 64 bit systems....

Pickle: handle invalid data on 64 bit systems.

There was a problem with pointer arithmetic for 64 bit systems
so invalid data was not properly detected. Now we do explicit tests.

BUG=56449
TEST=base_unittests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=66149

[darin (slow to review), 2010-11-11 17:33:57]

What do you think about adding a static method to Pickle that can be used to
validate input data?  Then, in places where we need to validate untrusted input,
we can call that function to determine when we should nuke a renderer.

Then, we can also make Pickle's constructor CHECK that the input is valid.

http://codereview.chromium.org/4716006/diff/1/base/pickle.cc
File base/pickle.cc (right):

http://codereview.chromium.org/4716006/diff/1/base/pickle.cc#newcode58
base/pickle.cc:58: header_ = NULL;
I'm a little concerned about header_ being null.  Previously, we always
guaranteed that it would not be null.  Maybe we should CHECK(false) instead?

Can you tell me more about why you want this to be a silent runtime failure
instead of a CHECK(false)?  Are you trying to avoid crashing the browser process
when receiving a malformed IPC from a renderer?

http://codereview.chromium.org/4716006/diff/1/base/pickle.h
File base/pickle.h (right):

http://codereview.chromium.org/4716006/diff/1/base/pickle.h#newcode183
base/pickle.h:183: return header_ ? payload() + payload_size() : NULL;
what about the non-const version of end_of_payload?

[rvargas (doing something else), 2010-11-11 22:35:36]

I could add an IsValid() method that returns false when header_ is null, as a
convenience to interested parties, but I don't anticipate having any callers
though. The current pattern of just attempting to read and failing that
operation still works.

http://codereview.chromium.org/4716006/diff/1/base/pickle.cc
File base/pickle.cc (right):

http://codereview.chromium.org/4716006/diff/1/base/pickle.cc#newcode58
base/pickle.cc:58: header_ = NULL;
On 2010/11/11 17:33:57, darin wrote:
> I'm a little concerned about header_ being null.  Previously, we always
> guaranteed that it would not be null.  Maybe we should CHECK(false) instead?
> 
> Can you tell me more about why you want this to be a silent runtime failure
> instead of a CHECK(false)?  Are you trying to avoid crashing the browser
process
> when receiving a malformed IPC from a renderer?

This CL intends to fix a browser crash on Linux x64, not related to IPC.

The way I understand this class is that this is the constructor to be used with
previously pickled data, so the normal use would be to distrust the data
somehow. I could certainly add a static method to perform validation previously
to instantiation of an object, but that would require us to track all current
uses of this constructor to add the call to perform the validation.

I was also a little concerned about changing the invariants about header_, but
at this point I think it is the simplest solution... it allows us to handle the
error cases with the same behavior that we have in 32 bits (just instantiate the
object, and all methods that read from it will fail), without any extra
overhead.

... and attempting to write to an invalid pickle will crash immediately, which
may not be a bad thing at this time.

http://codereview.chromium.org/4716006/diff/1/base/pickle.h
File base/pickle.h (right):

http://codereview.chromium.org/4716006/diff/1/base/pickle.h#newcode183
base/pickle.h:183: return header_ ? payload() + payload_size() : NULL;
On 2010/11/11 17:33:57, darin wrote:
> what about the non-const version of end_of_payload?

The expectation for the non-const version is to have a valid header_ (it should
come from a different constructor). I could DCHECK that, but it will require
changing the signature and moving the method to the cc file. (As is, the process
will crash reading from 0 on payload_size(), so in practice we are CHECKing that
it is not null). Let me know if you prefer an explicit CHECK or DCHECK.

I added comments.

[darin (slow to review), 2010-11-12 22:14:57]

OK, LGTM

On Thu, Nov 11, 2010 at 2:35 PM, <rvargas@chromium.org> wrote:

> I could add an IsValid() method that returns false when header_ is null, as
> a
> convenience to interested parties, but I don't anticipate having any
> callers
> though. The current pattern of just attempting to read and failing that
> operation still works.
>
>
>
> http://codereview.chromium.org/4716006/diff/1/base/pickle.cc
> File base/pickle.cc (right):
>
> http://codereview.chromium.org/4716006/diff/1/base/pickle.cc#newcode58
> base/pickle.cc:58: header_ = NULL;
> On 2010/11/11 17:33:57, darin wrote:
>
>> I'm a little concerned about header_ being null.  Previously, we
>>
> always
>
>> guaranteed that it would not be null.  Maybe we should CHECK(false)
>>
> instead?
>
>  Can you tell me more about why you want this to be a silent runtime
>>
> failure
>
>> instead of a CHECK(false)?  Are you trying to avoid crashing the
>>
> browser process
>
>> when receiving a malformed IPC from a renderer?
>>
>
> This CL intends to fix a browser crash on Linux x64, not related to IPC.
>
> The way I understand this class is that this is the constructor to be
> used with previously pickled data, so the normal use would be to
> distrust the data somehow. I could certainly add a static method to
> perform validation previously to instantiation of an object, but that
> would require us to track all current uses of this constructor to add
> the call to perform the validation.
>
> I was also a little concerned about changing the invariants about
> header_, but at this point I think it is the simplest solution... it
> allows us to handle the error cases with the same behavior that we have
> in 32 bits (just instantiate the object, and all methods that read from
> it will fail), without any extra overhead.
>
> ... and attempting to write to an invalid pickle will crash immediately,
> which may not be a bad thing at this time.
>
>
> http://codereview.chromium.org/4716006/diff/1/base/pickle.h
> File base/pickle.h (right):
>
> http://codereview.chromium.org/4716006/diff/1/base/pickle.h#newcode183
> base/pickle.h:183: return header_ ? payload() + payload_size() : NULL;
> On 2010/11/11 17:33:57, darin wrote:
>
>> what about the non-const version of end_of_payload?
>>
>
> The expectation for the non-const version is to have a valid header_ (it
> should come from a different constructor). I could DCHECK that, but it
> will require changing the signature and moving the method to the cc
> file. (As is, the process will crash reading from 0 on payload_size(),
> so in practice we are CHECKing that it is not null). Let me know if you
> prefer an explicit CHECK or DCHECK.
>
> I added comments.
>
>
> http://codereview.chromium.org/4716006/
>