Avoid integer overflows in BlobURLRequestJob.

Avoid integer overflows in BlobURLRequestJob.

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

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

[aedla, 2013-01-22 10:40:55]

@kinuko: maybe you could take a look at this or suggest someone else?

[aedla, 2013-01-22 10:41:22]

Main changes:

1) No negative values in ComputeBytesToRead as everything is unsigned.
2) total_size_ can't overflow when adding up item sizes.
3) length and offset should have sane values for file items.

[aedla, 2013-01-22 10:43:05]

If you think going to uint64 causes problems, I can go back to int64 and check
that nothing goes negative.

[kinuko, 2013-01-22 11:48:12]

Thanks so much to work on this, this issue was (kinda) impressive!

On 2013/01/22 10:43:05, aedla wrote:
> If you think going to uint64 causes problems, I can go back to int64 and check
> that nothing goes negative.

Yeah using uint64 concerns me a bit, especially because the special item length
value '-1' in type==FILE case originally comes from WebKit code and I'm afraid
one could easily get lost if we use different notion (e.g.
std::numeric_limits<uint64>::max()) to indicate the value. At least we'll need
more specific comment about that.

[aedla, 2013-01-23 14:47:14]

> Thanks so much to work on this, this issue was (kinda) impressive!

Thanks!

> Yeah using uint64 concerns me a bit, especially because the special item
length
> value '-1' in type==FILE case originally comes from WebKit code and I'm afraid
> one could easily get lost if we use different notion (e.g.
> std::numeric_limits<uint64>::max()) to indicate the value. At least we'll need
> more specific comment about that.

I went back to int64.

[kinuko, 2013-01-25 08:01:03]

lgtm w/ some style nits

https://codereview.chromium.org/12047012/diff/2002/webkit/blob/blob_url_reque...
File webkit/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/12047012/diff/2002/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:193: if (enough_space) {
nit: it'd be simpler to just early return in false case?  (They we may not need
the bool variable either)

https://codereview.chromium.org/12047012/diff/2002/webkit/blob/blob_url_reque...
webkit/blob/blob_url_request_job.cc:285: if (item_length ==
static_cast<uint64>(-1))
nit: to be consistent in all if/else clauses please use { } here too

[commit-bot: I haz the power, 2013-01-28 14:18:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/12047012/8001

[commit-bot: I haz the power, 2013-01-28 16:24:44]

Change committed as 179154

[aedla, 2013-01-29 10:56:26]

Reverted because of a static initializer. Removed it, going for another run.

[commit-bot: I haz the power, 2013-01-29 10:58:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/12047012/20001

[commit-bot: I haz the power, 2013-01-29 11:46:07]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[aedla, 2013-01-29 12:40:36]

Hm, the compile failure looks unrelated to this change. Also, yesterday all the
tries succeeded.

[kinuko, 2013-01-29 12:58:30]

win try bot is broken right now and never compiles (trying some reverts now)
http://build.chromium.org/p/tryserver.chromium/builders/win/

[michaeln, 2013-01-29 20:22:43]

https://codereview.chromium.org/12047012/diff/20001/webkit/blob/blob_url_requ...
File webkit/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/12047012/diff/20001/webkit/blob/blob_url_requ...
webkit/blob/blob_url_request_job.cc:189: if (item_length >
std::numeric_limits<int64>::max() - total_size_) {
base/basictypes.h defines some constants like this... kint64max

[commit-bot: I haz the power, 2013-01-31 14:21:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aedla@chromium.org/12047012/14002

[commit-bot: I haz the power, 2013-01-31 17:30:40]

Change committed as 179901