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

net/http/http_chunked_decoder.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Derived from:
 //   mozilla/netwerk/protocol/http/src/nsHttpChunkedDecoder.cpp
 // The license block is:
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
@@ skipping 52 matching lines @@
       chunk_terminator_remaining_(false),
       reached_last_chunk_(false),
       reached_eof_(false),
       bytes_after_eof_(0) {
 }
 
 int HttpChunkedDecoder::FilterBuf(char* buf, int buf_len) {
   int result = 0;
 
   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;
 
       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;
     } else if (reached_eof_) {
       bytes_after_eof_ += buf_len;
       break;  // Done!
     }
 
     int bytes_consumed = ScanForChunkRemaining(buf, buf_len);
     if (bytes_consumed < 0)
@@ skipping 89 matching lines @@
 //   RFC 7230: ^\X+$
 //        IE7: ^\X+[^\X]*$
 // Safari 3.1: ^[\t\r ]*\X+[\t ]*$
 //  Firefox 3: ^[\t\f\v\r ]*[+]?(0x)?\X+[^\X]*$
 // Opera 9.51: ^[\t\f\v ]*[+]?(0x)?\X+[^\X]*$
 //
 // Our strategy is to be as strict as possible, while not breaking
 // 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.

       != base::StringPiece::npos) {
     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;
   }
   return false;
 }
 
 }  // namespace net