net: don't process truncated headers.

net/http/http_stream_parser.cc

 // Copyright (c) 2012 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.
 
 #include "net/http/http_stream_parser.h"
 
 #include "base/bind.h"
 #include "base/compiler_specific.h"
 #include "base/string_util.h"
 #include "base/values.h"
@@ skipping 523 matching lines @@
     return result;
   }
 
   // Record our best estimate of the 'response time' as the time when we read
   // the first bytes of the response headers.
   if (read_buf_->offset() == 0 && result != ERR_CONNECTION_CLOSED)
     response_->response_time = base::Time::Now();
 
   if (result == ERR_CONNECTION_CLOSED) {
     // The connection closed before we detected the end of the headers.
-    // parse things as well as we can and let the caller decide what to do.
     if (read_buf_->offset() == 0) {
       // The connection was closed before any data was sent. Likely an error
       // rather than empty HTTP/0.9 response.
       io_state_ = STATE_DONE;
       return ERR_EMPTY_RESPONSE;
+    } else if (request_->url.SchemeIs("https")) {

[Ryan Sleevi, 2013/05/29 19:59:33]

Seems like we should be looking at SchemeIsSecure - that is, also counting in
wss:// (can't they set cookies during their initial Upgrade negotiation?)

I'm curious if we attempt a wss:// request and the server doesn't support it, if
it's possible to fall back to an HTTP response (with a Set-Cookie), thus
allowing this to work again.

[wtc, 2013/05/30 00:38:30]

Yes, this sounds like a good idea. In fact, I am wondering if we
should also do this for HTTP.

Nit: omit "else" after a return statement.

[agl, 2013/06/03 18:18:29]

After discussions with Darin, I only did this for HTTPS in order to limit compat
concerns.

r.e. SchemeIsSecure - should the other uses of SchemeIs("https") in this file be
changed too? (I'm guessing yes, but wanted to check.)

[wtc, 2013/06/03 18:36:47]

It occurred to me that this file, http_stream_parser.cc, is probably only used
for the "http" and "https" schemes. If so, then SchemeIs("https") vs.
SchemeIsSecure()
is just a cosmetic issue.

SchemeIsSecure is defined as:

  // If the scheme indicates a secure connection
  bool SchemeIsSecure() const {
    return SchemeIs("https") || SchemeIs("wss") ||
        (SchemeIsFileSystem() && inner_url() && inner_url()->SchemeIsSecure());
  }

So there is also a SchemeIsFileSystem() test, which I don't fully understand.

Since this file isn't using SchemeIsSecure, I now think it is fine to just
use SchemeIs("https") here.

+      // The connection was closed in the middle of the headers. For HTTPS we
+      // don't parse partial headers. Return a different error code so that we

[wtc, 2013/05/30 00:38:30]

By "a different error code", do you mean an error code different
from ERR_EMPTY_RESPONSE?

[agl, 2013/06/03 18:18:29]

Yes, will clarify.

+      // know that we shouldn't attempt to retry the request.
+      io_state_ = STATE_DONE;
+      return ERR_HEADERS_TRUNCATED;
+    }
+    // Parse things as well as we can and let the caller decide what to do.
+    int end_offset;
+    if (response_header_start_offset_ >= 0) {
+      io_state_ = STATE_READ_BODY_COMPLETE;
+      end_offset = read_buf_->offset();
     } else {
-      int end_offset;
-      if (response_header_start_offset_ >= 0) {
-        io_state_ = STATE_READ_BODY_COMPLETE;
-        end_offset = read_buf_->offset();
-      } else {
-        io_state_ = STATE_BODY_PENDING;
-        end_offset = 0;
-      }
-      int rv = DoParseResponseHeaders(end_offset);
-      if (rv < 0)
-        return rv;
-      return result;
+      io_state_ = STATE_BODY_PENDING;
+      end_offset = 0;
     }
+    int rv = DoParseResponseHeaders(end_offset);
+    if (rv < 0)
+      return rv;
+    return result;
   }
 
   read_buf_->set_offset(read_buf_->offset() + result);
   DCHECK_LE(read_buf_->offset(), read_buf_->capacity());
   DCHECK_GE(result,  0);
 
   int end_of_header_offset = ParseResponseHeaders();
 
   // Note: -1 is special, it indicates we haven't found the end of headers.
   // Anything less than -1 is a net::Error, so we bail out.
@@ skipping 368 matching lines @@
       request_body->IsInMemory() &&
       request_body->size() > 0) {
     size_t merged_size = request_headers.size() + request_body->size();
     if (merged_size <= kMaxMergedHeaderAndBodySize)
       return true;
   }
   return false;
 }
 
 }  // namespace net