QuicServer: Use Chrome's header parsing rather than Balsa

net/quic/quic_spdy_server_stream.cc

 // Copyright 2014 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/quic/quic_spdy_server_stream.h"
 
 #include "base/memory/singleton.h"
+#include "base/strings/string_number_conversions.h"
 #include "net/quic/quic_in_memory_cache.h"
 #include "net/quic/quic_session.h"
 #include "net/spdy/spdy_framer.h"
-#include "net/tools/quic/spdy_utils.h"
+#include "net/spdy/spdy_header_block.h"
+#include "net/spdy/spdy_http_utils.h"
 
 using base::StringPiece;
 using std::string;
-using net::tools::SpdyUtils;
 
 namespace net {
 
 static const size_t kHeaderBufInitialSize = 4096;
 
 QuicSpdyServerStream::QuicSpdyServerStream(QuicStreamId id,
                                            QuicSession* session)
     : QuicDataStream(id, session),
       read_buf_(new GrowableIOBuffer()),
       request_headers_received_(false) {
 }
 
 QuicSpdyServerStream::~QuicSpdyServerStream() {
 }
 
 uint32 QuicSpdyServerStream::ProcessData(const char* data, uint32 data_len) {
   uint32 total_bytes_processed = 0;

[wtc, 2014/06/25 23:17:32]

The internal version of this code indeed has a while loop. It looks very
different.

 
   // Are we still reading the request headers.
   if (!request_headers_received_) {
     // Grow the read buffer if necessary.
     if (read_buf_->RemainingCapacity() < (int)data_len) {
       read_buf_->SetCapacity(read_buf_->capacity() + kHeaderBufInitialSize);
     }
     memcpy(read_buf_->data(), data, data_len);
     read_buf_->set_offset(read_buf_->offset() + data_len);
-    ParseRequestHeaders();
-  } else {
-    body_.append(data + total_bytes_processed,
-                 data_len - total_bytes_processed);
+    total_bytes_processed = ParseRequestHeaders();

[wtc, 2014/06/25 22:22:24]

Check for -1 return value.

[dmziegler, 2014/06/26 01:23:29]

Now it's void.

   }
+  body_.append(data + total_bytes_processed,
+               data_len - total_bytes_processed);

[wtc, 2014/06/25 23:17:32]

IMPORTANT: I think this code is wrong if read_buf_ already has some data when we
memcpy more data into read_buf_ on line 41. In that case, ParseRequestHeaders()
will process all the data, not just the |data| added in this call. So
total_bytes_processed may be > data_len.

So the original way of appending to data_ inside ParseRequestHeaders seems
safer.

[dmziegler, 2014/06/26 01:23:29]

Done.

   return data_len;
 }
 
 void QuicSpdyServerStream::OnFinRead() {
   ReliableQuicStream::OnFinRead();
   if (write_side_closed() || fin_buffered()) {
     return;
   }
 
   if (!request_headers_received_) {
     SendErrorResponse();  // We're not done reading headers.
-  } else if ((headers_.content_length_status() ==
-              BalsaHeadersEnums::VALID_CONTENT_LENGTH) &&
-             body_.size() != headers_.content_length()) {
-    SendErrorResponse();  // Invalid content length
   } else {
-    SendResponse();
+    SpdyHeaderBlock::iterator it = headers_.find("content-length");

[wtc, 2014/06/25 22:22:24]

Use a const_iterator.

[dmziegler, 2014/06/26 01:23:29]

Done.

+    uint64 content_length;
+    if(it != headers_.end() &&

[wtc, 2014/06/25 22:22:24]

Add a space after "if".

[dmziegler, 2014/06/26 01:23:29]

Done.

+       (!base::StringToUint64(it->second, &content_length) ||

[wtc, 2014/06/25 22:22:24]

I wonder if it's better to use base::StringToSizeT instead.

[dmziegler, 2014/06/26 01:23:29]

Done.

+        body_.size() != content_length)) {
+      SendErrorResponse();  // Invalid content length
+    } else {
+      SendResponse();
+    }
   }

[wtc, 2014/06/25 22:22:24]

Nit: to avoid nesting the if statements, it may be better to use early returns:

  if (!request_headers_received_) {
    SendErrorResponse(); // We're not done reading headers.
    return;
  }

  SpdyHeaderBlock::const_iterator it = headers_.find("content-length");
  uint64 content_length;
  if (it != headers_.end() &&
      ...) {
    SendErrorResponse(); // Invalid content length
    return;
  }

  SendResponse();

[dmziegler, 2014/06/26 01:23:29]

Done.

 }
 
 size_t QuicSpdyServerStream::ParseRequestHeaders() {

[wtc, 2014/06/25 22:22:24]

IMPORTANT: since this function may return -1, the size_t return type may be
inappropriate.

[dmziegler, 2014/06/26 01:23:29]

Now I've made it void since the return value wasn't used anyway.

   size_t read_buf_len = static_cast<size_t>(read_buf_->offset());
   SpdyFramer framer(SPDY3);
-  SpdyHeaderBlock headers;
   char* data = read_buf_->StartOfBuffer();

[wtc, 2014/06/25 22:22:24]

Move line 72 after this line.

[dmziegler, 2014/06/26 01:23:29]

Done.

-  size_t len = framer.ParseHeaderBlockInBuffer(data, read_buf_->offset(),
-                                               &headers);
+  size_t len = framer.ParseHeaderBlockInBuffer(data, read_buf_len, &headers_);
   if (len == 0) {
     return -1;
   }
 
-  if (!SpdyUtils::FillBalsaRequestHeaders(headers, &headers_)) {
+  request_url_ = GetUrlFromHeaderBlock(headers_, SPDY3, false);

[wtc, 2014/06/25 22:22:24]

Is it OK to hardcode the version SPDY3?

[dmziegler, 2014/06/26 01:23:29]

All the test code seems to standardize on SPDY3 -- I've added a constant to
spdy_utils.h .

+  if (!request_url_.is_valid()) {
     SendErrorResponse();
     return -1;
   }
 
-  size_t delta = read_buf_len - len;
-  if (delta > 0) {
-    body_.append(data + len, delta);
-  }

[wtc, 2014/06/25 22:22:24]

IMPORTANT: Why is this code deleted?

[dmziegler, 2014/06/26 01:23:29]

No longer deleted.

-
   request_headers_received_ = true;
   return len;
 }
 
 void QuicSpdyServerStream::SendResponse() {
   // Find response in cache. If not found, send error response.
   const QuicInMemoryCache::Response* response =
-      QuicInMemoryCache::GetInstance()->GetResponse(headers_);
+      QuicInMemoryCache::GetInstance()->GetResponse(request_url_);
   if (response == NULL) {
     SendErrorResponse();
     return;
   }
 
   if (response->response_type() == QuicInMemoryCache::CLOSE_CONNECTION) {
     DVLOG(1) << "Special response: closing connection.";
     CloseConnection(QUIC_NO_ERROR);
     return;
   }
 
   if (response->response_type() == QuicInMemoryCache::IGNORE_REQUEST) {
     DVLOG(1) << "Special response: ignoring request.";
     return;
   }
 
   DVLOG(1) << "Sending response for stream " << id();
   SendHeadersAndBody(response->headers(), response->body());
 }
 
 void QuicSpdyServerStream::SendErrorResponse() {
   DVLOG(1) << "Sending error response for stream " << id();
-  BalsaHeaders headers;
-  headers.SetResponseFirstlineFromStringPieces(
-      "HTTP/1.1", "500", "Server Error");
-  headers.ReplaceOrAppendHeader("content-length", "3");
+  scoped_refptr<HttpResponseHeaders> headers
+      = new HttpResponseHeaders("HTTP/1.1 500 Server Error\n"
+                                "content-length: 3\n");

[wtc, 2014/06/25 22:22:24]

IMPORTANT: according to the comments in http_response_headers.h, the lines in
this string should be \0-terminated, and the string should end with two \0s in a
row.

[dmziegler, 2014/06/26 01:23:29]

Done.

   SendHeadersAndBody(headers, "bad");
 }
 
 void QuicSpdyServerStream::SendHeadersAndBody(
-    const BalsaHeaders& response_headers,
+    scoped_refptr<HttpResponseHeaders> response_headers,
     StringPiece body) {
   // We only support SPDY and HTTP, and neither handles bidirectional streaming.
   if (!read_side_closed()) {
     CloseReadSide();
   }
 
-  SpdyHeaderBlock header_block =
-      SpdyUtils::ResponseHeadersToSpdyHeaders(response_headers);
+  SpdyHeaderBlock header_block;
+  CreateSpdyHeadersFromHttpResponse(response_headers, SPDY3, &header_block);

[wtc, 2014/06/25 22:22:24]

IMPORTANT: is it OK to hardcode the version SPDY3?

[dmziegler, 2014/06/26 01:23:29]

Seems to be, since it's used only used for testing purposes and done
consistently. I've made a constant for it in quic/spdy_utils.h .

 
   WriteHeaders(header_block, body.empty(), NULL);
 
   if (!body.empty()) {
     WriteOrBufferData(body, true, NULL);
   }
 }
 
 }  // namespace net