A large Content-Length header followed by a connection close could trigger an...

A large Content-Length header followed by a connection close could trigger an out of memory condition.  Fixed problem, added unit test, and clarified the API.  This is probably the real problem in issue 25826.

BUG=28346, 25826
TEST=HttpNetworkTransactionTest.LargeContentLengthThenClose

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=32856

[vandebo (ex-Chrome), 2009-11-20 23:28:44]

The root of the problem was casting from int64 to int before validating the
result (checking that it was positive).

[eroman, 2009-11-20 23:55:59]

LGTM

http://codereview.chromium.org/418035/diff/4001/4004
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/418035/diff/4001/4004#newcode87
Line 87: DCHECK(buf_len <= kMaxHeaderBufSize);
DCHECK_LE

http://codereview.chromium.org/418035/diff/4001/4004#newcode386
Line 386: CHECK(save_amount + additional_save_amount <= kMaxHeaderBufSize);
I am not really familiar with how things work in the new HttpStreamParser, I
will try to review this more carefully after familiarizing myself.

[wtc, 2009-11-21 05:55:02]

LGTM.

http://codereview.chromium.org/418035/diff/4001/4003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/418035/diff/4001/4003#newcode3879
Line 3879: TEST_F(HttpNetworkTransactionTest, LargeContentLengthThenReset) {
Nit: Reset => Close
because reset implies ERR_CONNECTION_RESET to me.

Is it correct for us to return out.rv = OK when the
connection is closed prematurely?

http://codereview.chromium.org/418035/diff/4001/4004
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/418035/diff/4001/4004#newcode378
Line 378: if (extra_data_read < 0)
Since save_amount is initialized to 0, lines 378-383
are equivalent to the following code:

      if (extra_data_read > 0) {
        save_amount = static_cast<int>(extra_data_read);
        if (result > 0)
          result -= save_amount;
      }

[vandebo (ex-Chrome), 2009-11-23 21:32:47]

Checked in patch set 3.

http://codereview.chromium.org/418035/diff/4001/4003
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/418035/diff/4001/4003#newcode3879
net/http/http_network_transaction_unittest.cc:3879:
TEST_F(HttpNetworkTransactionTest, LargeContentLengthThenReset) {
On 2009/11/21 05:55:02, wtc wrote:
> Nit: Reset => Close
> because reset implies ERR_CONNECTION_RESET to me.

Done.

> Is it correct for us to return out.rv = OK when the
> connection is closed prematurely?

I'm not sure if it's correct, but it was the behavior before I checked in any of
this refactoring.

http://codereview.chromium.org/418035/diff/4001/4004
File net/http/http_stream_parser.cc (right):

http://codereview.chromium.org/418035/diff/4001/4004#newcode87
net/http/http_stream_parser.cc:87: DCHECK(buf_len <= kMaxHeaderBufSize);
On 2009/11/20 23:55:59, eroman wrote:
> DCHECK_LE

Done.

http://codereview.chromium.org/418035/diff/4001/4004#newcode378
net/http/http_stream_parser.cc:378: if (extra_data_read < 0)
On 2009/11/21 05:55:02, wtc wrote:
> Since save_amount is initialized to 0, lines 378-383
> are equivalent to the following code:
> 
>       if (extra_data_read > 0) {
>         save_amount = static_cast<int>(extra_data_read);
>         if (result > 0)
>           result -= save_amount;
>       }

Done.

[wtc, 2009-11-23 22:50:08]

On 2009/11/23 21:32:47, vandebo wrote:
> Checked in patch set 3.

Good.  Steve, please merge this fix to the 249 branch
this week.  Thanks.