New issue
Advanced search Search tips

Issue 603140 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Chrome reuses sockets after reading junk data from them.

Project Member Reported by mmenke@chromium.org, Apr 13 2016

Issue description

If a server sends us junk data after sending us a valid response, and HttpStreamParser consumers all of that junk data while reading the response, then we'll return the socket back to the socket pool, ignoring the extra data.  We should throw away the socket as not reuseable.  It looks like other browsers do this.

This can happen in 2 cases:  If we read beyond the end of a response while trying to read the headers, and the response (headers and body, if any) is shorter than what we read.  Or we could be reading beyond the end of a chunked response.

There was a bug report a couple months ago where a server was sending response bodies with HEAD responses, which is broken behavior, but only Chrome had issues with it.  I suspect this is the reason.  I'm not so concerned about broken servers, but Chrome's behavior here seems really wrong.  I'd link the bug, but I can't find it.
 

Comment 1 by mmenke@chromium.org, Apr 13 2016

Summary: Chrome reuses sockets after reading junk data from them. (was: Chrome reuses socks after reading junk data from them.)
Project Member

Comment 2 by bugdroid1@chromium.org, Jun 2 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5f94fda863b8540f99eb571b4f94c8dd24dba406

commit 5f94fda863b8540f99eb571b4f94c8dd24dba406
Author: mmenke <mmenke@chromium.org>
Date: Thu Jun 02 20:54:13 2016

HttpStreamParser:  Don't reuse sockets which receive unparsed data.

If HttpStreamParser has extra data left over after consuming a valid
HTTP/1.x response on a socket, it would still allow the socket to be
reused if it was connected and idle.  Since we have no idea what the
data actually was, this just seems like a bad idea.  This CL changes
that behavior, so such sockets are no longer considered reuseable.

This does have the downside of papering over server bugs, but it
still seems like the right way to go.

BUG= 603140 

Review-Url: https://codereview.chromium.org/1884943003
Cr-Commit-Position: refs/heads/master@{#397504}

[modify] https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406/net/http/http_network_transaction_unittest.cc
[modify] https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406/net/http/http_response_body_drainer.cc
[modify] https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406/net/http/http_response_body_drainer_unittest.cc
[modify] https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406/net/http/http_stream_parser.cc
[modify] https://crrev.com/5f94fda863b8540f99eb571b4f94c8dd24dba406/net/http/http_stream_parser.h

Status: Fixed (was: Assigned)
Components: Internals>Network
Components: -Internals>Network>HTTP

Sign in to add a comment