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

net/http/http_chunked_decoder.cc

Index: net/http/http_chunked_decoder.cc
diff --git a/net/http/http_chunked_decoder.cc b/net/http/http_chunked_decoder.cc
index 7aa4a965cd707cc705a693863bb75a6f26ef91b1..322c4a6773943d28ee92ca55e36019f85b4f3272 100644
--- a/net/http/http_chunked_decoder.cc
+++ b/net/http/http_chunked_decoder.cc
@@ -71,7 +71,10 @@ int HttpChunkedDecoder::FilterBuf(char* buf, int buf_len) {
 
   while (buf_len) {
     if (chunk_remaining_) {

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

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..

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

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.

-      int num = std::min(chunk_remaining_, buf_len);
+      // Since |chunk_remaining_| is positive and |buf_len| an int, the minimum

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

See comment above (hard to reason about at this line if it is actually positive
or simply "not zero").

+      // of the two must be an int.
+      int num = static_cast<int>(
+          std::min(chunk_remaining_, static_cast<int64_t>(buf_len)));
 
       buf_len -= num;
       chunk_remaining_ -= num;
@@ -79,7 +82,7 @@ int HttpChunkedDecoder::FilterBuf(char* buf, int buf_len) {
       result += num;
       buf += num;
 
-      // After each chunk's data there should be a CRLF
+      // After each chunk's data there should be a CRLF.
       if (!chunk_remaining_)
         chunk_terminator_remaining_ = true;
       continue;
@@ -189,14 +192,16 @@ int HttpChunkedDecoder::ScanForChunkRemaining(const char* buf, int buf_len) {
 // known sites.
 //
 //         Us: ^\X+[ ]*$
-bool HttpChunkedDecoder::ParseChunkSize(const char* start, int len, int* out) {
+bool HttpChunkedDecoder::ParseChunkSize(const char* start,
+                                        int len,
+                                        int64_t* out) {
   DCHECK_GE(len, 0);
 
   // Strip trailing spaces
   while (len && start[len - 1] == ' ')
     len--;
 
-  // Be more restrictive than HexStringToInt;
+  // Be more restrictive than HexStringToInt64;
   // don't allow inputs with leading "-", "+", "0x", "0X"
   base::StringPiece chunk_size(start, len);
   if (chunk_size.find_first_not_of("0123456789abcdefABCDEF")

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

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.

@@ -204,8 +209,8 @@ bool HttpChunkedDecoder::ParseChunkSize(const char* start, int len, int* out) {
     return false;
   }
 
-  int parsed_number;
-  bool ok = base::HexStringToInt(chunk_size, &parsed_number);
+  int64_t parsed_number;
+  bool ok = base::HexStringToInt64(chunk_size, &parsed_number);
   if (ok && parsed_number >= 0) {
     *out = parsed_number;
     return true;