Simple Cache: validate lengths before allocations.

Simple Cache: validate lengths before allocations.

This bug was most unfortunate. A corrupt entry could cause Chrome to
crash when opening it, without removing the entry. Not a good loop to
be in. Now we are much more careful with casts around the data coming
from disk to confirm sanity before proceeding.

R=juliatuttle@chromium.org
BUG=541720
Committed: https://crrev.com/bf840cc6c21c5a5b6e95d120e5493e0a7eb61498
Cr-Commit-Position: refs/heads/master@{#408134}

[gavinp, 2016-06-22 15:06:56]

ptal

[gavinp, 2016-06-27 18:57:38]

On 2016/06/22 15:06:56, gavinp wrote:
> ptal

Ping

[Julia Tuttle, 2016-06-27 20:39:10]

Sorry for the latency.

https://codereview.chromium.org/2086053003/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/2086053003/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_synchronous_entry.cc:1191: if (stream_0_size >
stream_1_size)
This conditional sounds nonsensical. It makes sense in the context of
"stream_1_size is actually the length of the whole thing and I'm about to
subtract stream_0_size from it", but it'd make more sense if you used another
name for stream_1_size and then did "int32_t stream_1_size = total_size -
stream_0_size" or something like that.

https://codereview.chromium.org/2086053003/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_synchronous_entry.cc:1269: if
(!base::IsValueInRangeForNumericType<int>(eof_record.stream_size))
int, or int32_t? (Also, do you want to check for negative here?)

[Julia Tuttle, 2016-07-18 14:49:05]

Ping?

[Julia Tuttle, 2016-07-25 19:35:07]

Ping?

[gavinp, 2016-07-26 14:10:45]

On 2016/07/25 19:35:07, Julia Tuttle wrote:
> Ping?

I've been out quite a bit since I wrote this; I'm returning now and look forward
to cycling it shortly.

[gavinp, 2016-07-26 17:28:20]

Description was changed from

==========
Simple Cache: validate lengths before allocations.

This bug was most unfortunate. A corrupt entry could cause Chrome to
crash when opening it, without removing the entry. Not a good loop to
be in. Now we are much more careful with casts around the data coming
from disk to confirm sanity before proceeding.

R=juliatuttle@chromium.org
BUG=547864
==========

to

==========
Simple Cache: validate lengths before allocations.

This bug was most unfortunate. A corrupt entry could cause Chrome to
crash when opening it, without removing the entry. Not a good loop to
be in. Now we are much more careful with casts around the data coming
from disk to confirm sanity before proceeding.

R=juliatuttle@chromium.org
BUG=541720
==========

[gavinp, 2016-07-26 17:28:31]

The CQ bit was checked by gavinp@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-26 17:28:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gavinp, 2016-07-26 17:32:35]

Julia,

PTAL. I believe I remediated to your comments. Thanks!

https://codereview.chromium.org/2086053003/diff/1/net/disk_cache/simple/simpl...
File net/disk_cache/simple/simple_synchronous_entry.cc (right):

https://codereview.chromium.org/2086053003/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_synchronous_entry.cc:1191: if (stream_0_size >
stream_1_size)
On 2016/06/27 20:39:10, Julia Tuttle wrote:
> This conditional sounds nonsensical. It makes sense in the context of
> "stream_1_size is actually the length of the whole thing and I'm about to
> subtract stream_0_size from it", but it'd make more sense if you used another
> name for stream_1_size and then did "int32_t stream_1_size = total_size -
> stream_0_size" or something like that.

Done.

https://codereview.chromium.org/2086053003/diff/1/net/disk_cache/simple/simpl...
net/disk_cache/simple/simple_synchronous_entry.cc:1269: if
(!base::IsValueInRangeForNumericType<int>(eof_record.stream_size))
On 2016/06/27 20:39:10, Julia Tuttle wrote:
> int, or int32_t? (Also, do you want to check for negative here?)

Great catch. Fixed the signature. Thanks.

Don't need to check for negative. eof_record.steam_size is an unsigned, so it's
UNPOSSIBLE.

[Julia Tuttle, 2016-07-26 18:15:06]

lgtm.

[commit-bot: I haz the power, 2016-07-26 18:25:53]

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

[commit-bot: I haz the power, 2016-07-26 18:25:54]

Dry run: This issue passed the CQ dry run.

[gavinp, 2016-07-27 15:27:28]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2016-07-27 15:27:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-27 15:31:23]

Description was changed from

==========
Simple Cache: validate lengths before allocations.

This bug was most unfortunate. A corrupt entry could cause Chrome to
crash when opening it, without removing the entry. Not a good loop to
be in. Now we are much more careful with casts around the data coming
from disk to confirm sanity before proceeding.

R=juliatuttle@chromium.org
BUG=541720
==========

to

==========
Simple Cache: validate lengths before allocations.

This bug was most unfortunate. A corrupt entry could cause Chrome to
crash when opening it, without removing the entry. Not a good loop to
be in. Now we are much more careful with casts around the data coming
from disk to confirm sanity before proceeding.

R=juliatuttle@chromium.org
BUG=541720
==========

[commit-bot: I haz the power, 2016-07-27 15:31:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-27 15:33:40]

Description was changed from

==========
Simple Cache: validate lengths before allocations.

This bug was most unfortunate. A corrupt entry could cause Chrome to
crash when opening it, without removing the entry. Not a good loop to
be in. Now we are much more careful with casts around the data coming
from disk to confirm sanity before proceeding.

R=juliatuttle@chromium.org
BUG=541720
==========

to

==========
Simple Cache: validate lengths before allocations.

This bug was most unfortunate. A corrupt entry could cause Chrome to
crash when opening it, without removing the entry. Not a good loop to
be in. Now we are much more careful with casts around the data coming
from disk to confirm sanity before proceeding.

R=juliatuttle@chromium.org
BUG=541720

Committed: https://crrev.com/bf840cc6c21c5a5b6e95d120e5493e0a7eb61498
Cr-Commit-Position: refs/heads/master@{#408134}
==========

[commit-bot: I haz the power, 2016-07-27 15:33:42]

Patchset 2 (id:??) landed as
https://crrev.com/bf840cc6c21c5a5b6e95d120e5493e0a7eb61498
Cr-Commit-Position: refs/heads/master@{#408134}