net: don't preserve 1xx responses in parser buffer.

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/strings/string_util.h"
 #include "base/values.h"
@@ skipping 61 matching lines @@
 
 namespace net {
 
 // Similar to DrainableIOBuffer(), but this version comes with its own
 // storage. The motivation is to avoid repeated allocations of
 // DrainableIOBuffer.
 //
 // Example:
 //
 // scoped_refptr<SeekableIOBuffer> buf = new SeekableIOBuffer(1024);
-// // capacity() == 1024. size() == BytesRemaining == BytesConsumed() == 0.
+// // capacity() == 1024. size() == BytesRemaining() == BytesConsumed() == 0.
 // // data() points to the beginning of the buffer.
 //
 // // Read() takes an IOBuffer.
 // int bytes_read = some_reader->Read(buf, buf->capacity());
 // buf->DidAppend(bytes_read);
 // // size() == BytesRemaining() == bytes_read. data() is unaffected.
 //
 // while (buf->BytesRemaining() > 0) {
 //   // Write() takes an IOBuffer. If it takes const char*, we could
 ///  // simply use the regular IOBuffer like buf->data() + offset.
 //   int bytes_written = Write(buf, buf->BytesRemaining());
 //   buf->DidConsume(bytes_written);
 // }
 // // BytesRemaining() == 0. BytesConsumed() == size().
-// // data() points to the end of the comsumed bytes (exclusive).
+// // data() points to the end of the consumed bytes (exclusive).
 //
 // // If you want to reuse the buffer, be sure to clear the buffer.
 // buf->Clear();
 // // size() == BytesRemaining() == BytesConsumed() == 0.
 // // data() points to the beginning of the buffer.
 //
 class HttpStreamParser::SeekableIOBuffer : public net::IOBuffer {
  public:
   explicit SeekableIOBuffer(int capacity)
     : IOBuffer(capacity),
@@ skipping 46 matching lines @@
   // the object is created.
   int capacity() const { return capacity_; };
 
  private:
   virtual ~SeekableIOBuffer() {
     // data_ will be deleted in IOBuffer::~IOBuffer().
     data_ = real_data_;
   }
 
   char* real_data_;
-  int capacity_;
+  const int capacity_;
   int size_;
   int used_;
 };
 
 // 2 CRLFs + max of 8 hex chars.
 const size_t HttpStreamParser::kChunkHeaderFooterSize = 12;
 
 HttpStreamParser::HttpStreamParser(ClientSocketHandle* connection,
                                    const HttpRequestInfo* request,
                                    GrowableIOBuffer* read_buffer,
@@ skipping 111 matching lines @@
   if (result == ERR_IO_PENDING)
     callback_ = callback;
 
   return result > 0 ? OK : result;
 }
 
 int HttpStreamParser::ReadResponseHeaders(const CompletionCallback& callback) {
   DCHECK(io_state_ == STATE_REQUEST_SENT || io_state_ == STATE_DONE);
   DCHECK(callback_.is_null());
   DCHECK(!callback.is_null());
+  DCHECK_EQ(0, read_buf_unused_offset_);
 
   // This function can be called with io_state_ == STATE_DONE if the
   // connection is closed after seeing just a 1xx response code.
   if (io_state_ == STATE_DONE)
     return ERR_CONNECTION_CLOSED;
 
   int result = OK;
   io_state_ = STATE_READ_HEADERS;
 
   if (read_buf_->offset() > 0) {
     // Simulate the state where the data was just read from the socket.
-    result = read_buf_->offset() - read_buf_unused_offset_;
-    read_buf_->set_offset(read_buf_unused_offset_);
+    result = read_buf_->offset();
+    read_buf_->set_offset(0);
   }
   if (result > 0)
     io_state_ = STATE_READ_HEADERS_COMPLETE;
 
   result = DoLoop(result);
   if (result == ERR_IO_PENDING)
     callback_ = callback;
 
   return result > 0 ? OK : result;
 }
@@ skipping 191 matching lines @@
 
   // http://crbug.com/16371: We're seeing |user_buf_->data()| return NULL.
   // See if the user is passing in an IOBuffer with a NULL |data_|.
   CHECK(read_buf_->data());
 
   return connection_->socket()
       ->Read(read_buf_.get(), read_buf_->RemainingCapacity(), io_callback_);
 }
 
 int HttpStreamParser::DoReadHeadersComplete(int result) {
+  DCHECK_EQ(0, read_buf_unused_offset_);
+
   if (result == 0)
     result = ERR_CONNECTION_CLOSED;
 
   if (result < 0 && result != ERR_CONNECTION_CLOSED) {
     io_state_ = STATE_DONE;
     return result;
   }
   // If we've used the connection before, then we know it is not a HTTP/0.9
   // response and return ERR_CONNECTION_CLOSED.
   if (result == ERR_CONNECTION_CLOSED && read_buf_->offset() == 0 &&
@@ skipping 43 matching lines @@
   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.
   if (end_of_header_offset < -1)
     return end_of_header_offset;
 
   if (end_of_header_offset == -1) {
     io_state_ = STATE_READ_HEADERS;
     // Prevent growing the headers buffer indefinitely.
-    if (read_buf_->offset() - read_buf_unused_offset_ >= kMaxHeaderBufSize) {
+    if (read_buf_->offset() >= kMaxHeaderBufSize) {
       io_state_ = STATE_DONE;
       return ERR_RESPONSE_HEADERS_TOO_BIG;
     }
   } else {
+    CalculateResponseBodySize();
+    // If the body is 0, the caller may not call ReadResponseBody, which

[wtc, 2013/10/01 18:12:57]

1. Nit: "If the body is 0" isn't clear. I think it means "If the body is 0
bytes" or "If the body is of zero length". (This comment comes from the original
code.)

2. It seems that this bug could also affect the other response codes that have a
zero-length response body, such as 204 and 304. I think it's the fact that we
transition to STATE_DONE for those response codes that make them immune to this
bug. Correct?

[agl, 2013/10/01 19:53:18]

Fixed.
I don't think that's the case. (You should be able to see the bug now.)

+    // is where any extra data is copied to read_buf_, so we move the
+    // data here.
+    if (response_body_length_ == 0) {
+      int extra_bytes = read_buf_->offset() - end_of_header_offset;
+      if (extra_bytes) {
+        CHECK_GT(extra_bytes, 0);
+        memmove(read_buf_->StartOfBuffer(),

[wtc, 2013/10/01 18:12:57]

IMPORTANT: is it correct to move the extra data for 1xx responses? The original
code doesn't do that.

[agl, 2013/10/01 19:53:18]

(see bug.)

+                read_buf_->StartOfBuffer() + end_of_header_offset,
+                extra_bytes);
+      }
+      read_buf_->SetCapacity(extra_bytes);
+      read_buf_unused_offset_ = 0;

[wtc, 2013/10/01 18:12:57]

This is not necessary in the new code. (It was necessary in the original code.)

[agl, 2013/10/01 19:53:18]

Done.

+      if (response_->headers->response_code() / 100 == 1) {
+        // After processing a 1xx response, the caller will ask for the next
+        // header, so reset state to support that. We don't completely ignore a

[wtc, 2013/10/01 18:12:57]

Nit: delete "a" at the end of this line.

[agl, 2013/10/01 19:53:18]

Done.

+        // 1xx responses because they cannot be returned in reply to a CONNECT
+        // request so we return OK here, which lets the caller inspect the
+        // response and reject it in the event that we're setting up a CONNECT
+        // tunnel.
+        response_header_start_offset_ = -1;
+        response_body_length_ = -1;

[wtc, 2013/10/01 18:12:57]

Why do we need to change response_body_length_ to -1 here?

[agl, 2013/10/01 19:53:18]

It will have been set to 0 by CalculateResponseBodySize() and 0 is a valid value
so, for the next response, it won't be updated again unless it's set to -1 here.

+        io_state_ = STATE_REQUEST_SENT;
+      } else {
+        io_state_ = STATE_DONE;
+      }
+      return OK;
+    }
+
     // Note where the headers stop.
     read_buf_unused_offset_ = end_of_header_offset;
-
-    if (response_->headers->response_code() / 100 == 1) {
-      // After processing a 1xx response, the caller will ask for the next
-      // header, so reset state to support that.  We don't just skip these
-      // completely because 1xx codes aren't acceptable when establishing a
-      // tunnel.
-      io_state_ = STATE_REQUEST_SENT;
-      response_header_start_offset_ = -1;
-    } else {
-      io_state_ = STATE_BODY_PENDING;
-      CalculateResponseBodySize();
-      // If the body is 0, the caller may not call ReadResponseBody, which
-      // is where any extra data is copied to read_buf_, so we move the
-      // data here and transition to DONE.
-      if (response_body_length_ == 0) {
-        io_state_ = STATE_DONE;
-        int extra_bytes = read_buf_->offset() - read_buf_unused_offset_;
-        if (extra_bytes) {
-          CHECK_GT(extra_bytes, 0);
-          memmove(read_buf_->StartOfBuffer(),
-                  read_buf_->StartOfBuffer() + read_buf_unused_offset_,
-                  extra_bytes);
-        }
-        read_buf_->SetCapacity(extra_bytes);
-        read_buf_unused_offset_ = 0;
-        return OK;
-      }
-    }
+    io_state_ = STATE_BODY_PENDING;
   }
   return result;
 }
 
 int HttpStreamParser::DoReadBody() {
   io_state_ = STATE_READ_BODY_COMPLETE;
 
   // There may be some data left over from reading the response headers.
   if (read_buf_->offset()) {
     int available = read_buf_->offset() - read_buf_unused_offset_;
@@ skipping 116 matching lines @@
     io_state_ = STATE_BODY_PENDING;
     user_read_buf_ = NULL;
     user_read_buf_len_ = 0;
   }
 
   return result;
 }
 
 int HttpStreamParser::ParseResponseHeaders() {
   int end_offset = -1;
+  DCHECK_EQ(0, read_buf_unused_offset_);
 
   // Look for the start of the status line, if it hasn't been found yet.
   if (response_header_start_offset_ < 0) {
     response_header_start_offset_ = HttpUtil::LocateStartOfStatusLine(
-        read_buf_->StartOfBuffer() + read_buf_unused_offset_,
-        read_buf_->offset() - read_buf_unused_offset_);
+        read_buf_->StartOfBuffer(), read_buf_->offset());
   }
 
   if (response_header_start_offset_ >= 0) {
-    end_offset = HttpUtil::LocateEndOfHeaders(
-        read_buf_->StartOfBuffer() + read_buf_unused_offset_,
-        read_buf_->offset() - read_buf_unused_offset_,
-        response_header_start_offset_);
-  } else if (read_buf_->offset() - read_buf_unused_offset_ >= 8) {
+    end_offset = HttpUtil::LocateEndOfHeaders(read_buf_->StartOfBuffer(),
+                                              read_buf_->offset(),
+                                              response_header_start_offset_);
+  } else if (read_buf_->offset() >= 8) {
     // Enough data to decide that this is an HTTP/0.9 response.
     // 8 bytes = (4 bytes of junk) + "http".length()
     end_offset = 0;
   }
 
   if (end_offset == -1)
     return -1;
 
   int rv = DoParseResponseHeaders(end_offset);
   if (rv < 0)
     return rv;
-  return end_offset + read_buf_unused_offset_;
+  return end_offset;
 }
 
 int HttpStreamParser::DoParseResponseHeaders(int end_offset) {
   scoped_refptr<HttpResponseHeaders> headers;
+  DCHECK_EQ(0, read_buf_unused_offset_);
+
   if (response_header_start_offset_ >= 0) {
     headers = new HttpResponseHeaders(HttpUtil::AssembleRawHeaders(
-        read_buf_->StartOfBuffer() + read_buf_unused_offset_, end_offset));
+        read_buf_->StartOfBuffer(), end_offset));
   } else {
     // Enough data was read -- there is no status line.
     headers = new HttpResponseHeaders(std::string("HTTP/0.9 200 OK"));
   }
 
   // Check for multiple Content-Length headers with no Transfer-Encoding header.
   // If they exist, and have distinct values, it's a potential response
   // smuggling attack.
   if (!headers->HasHeader("Transfer-Encoding")) {
     if (HeadersContainMultipleCopiesOfField(*headers.get(), "Content-Length"))
@@ skipping 26 matching lines @@
   // RFC 2616 Section 4.3 Message Body:
   //
   // For response messages, whether or not a message-body is included with
   // a message is dependent on both the request method and the response
   // status code (section 6.1.1). All responses to the HEAD request method
   // MUST NOT include a message-body, even though the presence of entity-
   // header fields might lead one to believe they do. All 1xx
   // (informational), 204 (no content), and 304 (not modified) responses
   // MUST NOT include a message-body. All other responses do include a
   // message-body, although it MAY be of zero length.
-  switch (response_->headers->response_code()) {
-    // Note that 1xx was already handled earlier.
-    case 204:  // No Content
-    case 205:  // Reset Content
-    case 304:  // Not Modified
-      response_body_length_ = 0;
-      break;
+  if (response_->headers->response_code() / 100 == 1) {
+    response_body_length_ = 0;
+  } else {
+    switch (response_->headers->response_code()) {
+      case 204:  // No Content
+      case 205:  // Reset Content
+      case 304:  // Not Modified
+        response_body_length_ = 0;
+        break;
+    }
   }
   if (request_->method == "HEAD")
     response_body_length_ = 0;
 
   if (response_body_length_ == -1) {
     // "Transfer-Encoding: chunked" trumps "Content-Length: N"
     if (response_->headers->IsChunkEncoded()) {
       chunked_decoder_.reset(new HttpChunkedDecoder());
     } else {
       response_body_length_ = response_->headers->GetContentLength();
@@ skipping 96 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