HttpChunkedDecoder: Support chunks longer than 2^31-1 bytes.

HttpChunkedDecoder: Support chunks longer than 2^31-1 bytes.

We were using HexStringToInt to parse chunk size, which returns a
32-bit int.  This CL switches to using HexStringToInt64, which uses
64-bit ints, so we can now support chunks up to 2^63-1 bytes.

That should be enough for anybody. [Cue dramatic music]

BUG=630680
Committed: https://crrev.com/8ea249f8b70f154f9995ed538fc853fe9cf46503
Cr-Commit-Position: refs/heads/master@{#407549}

[mmenke, 2016-07-22 22:43:55]

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

[commit-bot: I haz the power, 2016-07-22 22:44:34]

Dry run: 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-22 23:56:03]

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

[commit-bot: I haz the power, 2016-07-22 23:56:04]

Dry run: This issue passed the CQ dry run.

[mmenke, 2016-07-25 15:33:01]

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

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

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

[mmenke, 2016-07-25 15:36:00]

mmenke@chromium.org changed reviewers:
+ eroman@chromium.org

[mmenke, 2016-07-25 15:36:01]

CL seems a bit silly, but failing a request because the chunk size is too big
seems even sillier.

Sounds like FireFox is using an unsigned int, so supports chunks over twice as
big as we do!  We must out-chunk-size them!

https://codereview.chromium.org/2170133004/diff/20001/net/http/http_chunked_d...
File net/http/http_chunked_decoder.cc (right):

https://codereview.chromium.org/2170133004/diff/20001/net/http/http_chunked_d...
net/http/http_chunked_decoder.cc:207: if
(chunk_size.find_first_not_of("0123456789abcdefABCDEF")
Could consider adding this to net/base/parse_number.h, though we don't exactly
parse a whole lot of hex strings in net.  ParseSSLCipherString is the only other
code that parses hex strings, and it expects a leading 0x.

[commit-bot: I haz the power, 2016-07-25 16:27:38]

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

[commit-bot: I haz the power, 2016-07-25 16:27:39]

Dry run: This issue passed the CQ dry run.

[eroman, 2016-07-25 18:11:38]

lgtm

https://codereview.chromium.org/2170133004/diff/20001/net/http/http_chunked_d...
File net/http/http_chunked_decoder.cc (right):

https://codereview.chromium.org/2170133004/diff/20001/net/http/http_chunked_d...
net/http/http_chunked_decoder.cc:73: if (chunk_remaining_) {
side-comment: These sort of "if" statements on signed types are an anti-pattern
IMO, since they assume non-negativity is preserved throughout.

Would probably be better to express them as >0 tests, or change the underlying
type to unsigned (which has other fallout).

But I guess that style is being used throughout this file, certainly no shortage
of casts..

https://codereview.chromium.org/2170133004/diff/20001/net/http/http_chunked_d...
net/http/http_chunked_decoder.cc:74: // Since |chunk_remaining_| is positive and
|buf_len| an int, the minimum
See comment above (hard to reason about at this line if it is actually positive
or simply "not zero").

[mmenke, 2016-07-25 18:19:28]

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

[commit-bot: I haz the power, 2016-07-25 18:19:45]

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

[mmenke, 2016-07-25 18:23:35]

Thanks for the feedback!

https://codereview.chromium.org/2170133004/diff/20001/net/http/http_chunked_d...
File net/http/http_chunked_decoder.cc (right):

https://codereview.chromium.org/2170133004/diff/20001/net/http/http_chunked_d...
net/http/http_chunked_decoder.cc:73: if (chunk_remaining_) {
On 2016/07/25 18:11:38, eroman wrote:
> side-comment: These sort of "if" statements on signed types are an
anti-pattern
> IMO, since they assume non-negativity is preserved throughout.
> 
> Would probably be better to express them as >0 tests, or change the underlying
> type to unsigned (which has other fallout).
> 
> But I guess that style is being used throughout this file, certainly no
shortage
> of casts..

I completely agree with you.  I've gone ahead and converted if([int]) to
if([int] > 0) and if(![int]) to if(int==0) throughout this file.  Doesn't seem
worth a separate CL for this, but makes this code clearer, so is worth doing.

Changes are sufficiently trivial that I don't think a new review is needed, so
I'll plan to go ahead and land.

[mmenke, 2016-07-25 18:23:47]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-07-25 18:23:47]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/2170133004/#ps60001
(title: "Missed two")

[commit-bot: I haz the power, 2016-07-25 18:24:13]

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

[eroman, 2016-07-25 19:08:23]

patchset4 LGTM

[commit-bot: I haz the power, 2016-07-25 19:49:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-25 19:52:09]

Description was changed from

==========
HttpChunkedDecoder: Support chunks longer than 2^31-1 bytes.

We were using HexStringToInt to parse chunk size, which returns a
32-bit int.  This CL switches to using HexStringToInt64, which uses
64-bit ints, so we can now support chunks up to 2^63-1 bytes.

That should be enough for anybody. [Cue dramatic music]

BUG=630680
==========

to

==========
HttpChunkedDecoder: Support chunks longer than 2^31-1 bytes.

We were using HexStringToInt to parse chunk size, which returns a
32-bit int.  This CL switches to using HexStringToInt64, which uses
64-bit ints, so we can now support chunks up to 2^63-1 bytes.

That should be enough for anybody. [Cue dramatic music]

BUG=630680

Committed: https://crrev.com/8ea249f8b70f154f9995ed538fc853fe9cf46503
Cr-Commit-Position: refs/heads/master@{#407549}
==========

[commit-bot: I haz the power, 2016-07-25 19:52:11]

Patchset 4 (id:??) landed as
https://crrev.com/8ea249f8b70f154f9995ed538fc853fe9cf46503
Cr-Commit-Position: refs/heads/master@{#407549}

[Mark P, 2016-07-26 00:31:19]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2183433003/ by mpearson@chromium.org.

The reason for reverting is: Speculative revert for bug 631246.  Have no better
ideas.

Apologies if this wasn't the cause.
.