Add Read/WriteSizeT() functions to the pickle layer, plus one consumer.

Add Read/WriteSizeT() functions to the pickle layer, plus one consumer.

This eliminates the need for callers to do explicit conversions, and also
ensures callers don't try to implement pickling of a size_t using a 32-bit type,
leading to truncation on 64-bit targets.  The pickle layer will ensure 64-bit
types are always used.

I'll be changing other callsites to use this in future patches.

BUG=none
TEST=none

Committed: https://crrev.com/89a19f1430afa495acfccbc3ed7da9f51a7911d1
Cr-Commit-Position: refs/heads/master@{#297774}

[Peter Kasting, 2014-09-23 23:54:47]

pkasting@chromium.org changed reviewers:
+ darin@chromium.org

[Peter Kasting, 2014-09-30 23:17:02]

pkasting@chromium.org changed reviewers:
+ brettw@chromium.org
- darin@chromium.org

[Peter Kasting, 2014-09-30 23:17:03]

Changing to brettw at Darin's request (flooded).

[brettw, 2014-10-01 18:08:23]

brettw@chromium.org changed reviewers:
+ tsepez@chromium.org

[brettw, 2014-10-01 18:08:24]

I'm adding Tom to this to think about security (Justin is out, I would normally
ask him about truncations). It's not clear to me that when a 64-bit process is
sending a 32-bit process a size_t, that it's a good idea to silently truncate
the high bits like you do with the static cast.

It shouldn't be a security issue since if it's malicious it could always send
the smaller truncated version in the first place. But it seems potentially
surprising.

[Tom Sepez, 2014-10-01 19:12:45]

https://codereview.chromium.org/601563003/diff/1/base/pickle.cc
File base/pickle.cc (right):

https://codereview.chromium.org/601563003/diff/1/base/pickle.cc#newcode114
base/pickle.cc:114: *result = static_cast<size_t>(result_uint64);
I'd be OK with this so long as we did a check that result_uint64 <
std::numeric_limits<size_t>::max, returning false if it isnt.  In that case,
when you are sending too big a value from 64 to 32, we fail hard.

[Tom Sepez, 2014-10-01 19:15:37]

Also, where are you planning on calling the new method?  From IPC?  If so, then
you may get conflicts between unsigned long and size_t if you want to introduce
template specializations for both, since size_t isn't really a type but an alias
for an underlying integral type.

[Peter Kasting, 2014-10-01 19:42:08]

PTAL.

On 2014/10/01 19:15:37, Tom Sepez wrote:
> Also, where are you planning on calling the new method?  From IPC?

I'd have to look through a bunch of other changes to find all the places, but I
don't think I actually made any calls from the low-level IPC layer, but rather
from a bunch of other consumers of these APIs.  I don't recall adding any
template specializations at all, and I verified that the complete resulting set
of changes compiled cleanly.

(I'd have a better answer for you, but Visual Studio lost about five hours worth
of my work isolating the different changes in my checkout and I'd have to redo
it before I could reply precisely.)

https://codereview.chromium.org/601563003/diff/1/base/pickle.cc
File base/pickle.cc (right):

https://codereview.chromium.org/601563003/diff/1/base/pickle.cc#newcode114
base/pickle.cc:114: *result = static_cast<size_t>(result_uint64);
On 2014/10/01 19:12:45, Tom Sepez wrote:
> I'd be OK with this so long as we did a check that result_uint64 <
> std::numeric_limits<size_t>::max, returning false if it isnt.  In that case,
> when you are sending too big a value from 64 to 32, we fail hard.

Sure.  Changed static_cast to checked_cast to achieve this.

[Tom Sepez, 2014-10-01 19:45:47]

> Sure.  Changed static_cast to checked_cast to achieve this.

Not sure you want to CHECK() here.  The convention was for these to return false
when the pickle can't be unpickled, e.g. we may want to handle corrupt pickles
elegantly.

[Peter Kasting, 2014-10-01 20:22:31]

On 2014/10/01 19:45:47, Tom Sepez wrote:
> > Sure.  Changed static_cast to checked_cast to achieve this.
> 
> Not sure you want to CHECK() here.  The convention was for these to return
false
> when the pickle can't be unpickled, e.g. we may want to handle corrupt pickles
> elegantly.

OK.  Changed to doing that instead.

[brettw, 2014-10-01 20:26:50]

base owners lgtm when tom is happy.

[Tom Sepez, 2014-10-01 20:28:29]

LGTM.  Just noticed there's a pickle_unittest.cc, we may want to see that
write/read of size_t's work as expected.

[Peter Kasting, 2014-10-01 21:13:49]

On 2014/10/01 20:28:29, Tom Sepez wrote:
> Just noticed there's a pickle_unittest.cc, we may want to see that
> write/read of size_t's work as expected.

Good call.  I added more tests there, both of the size_t stuff and of some of
the other read/write functions that weren't being at least smoketested.

[Peter Kasting, 2014-10-01 21:13:53]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-01 21:14:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/601563003/60001

[commit-bot: I haz the power, 2014-10-01 23:22:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-01 23:22:39]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Peter Kasting, 2014-10-02 00:55:19]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-02 00:57:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/601563003/80001

[commit-bot: I haz the power, 2014-10-02 03:01:11]

Committed patchset #5 (id:80001) as 34bbb984965ac961bc25be1feb7d28f6e7338b44

[commit-bot: I haz the power, 2014-10-02 03:01:51]

Patchset 5 (id:??) landed as
https://crrev.com/89a19f1430afa495acfccbc3ed7da9f51a7911d1
Cr-Commit-Position: refs/heads/master@{#297774}