Integrate server change to SpdyFrame which eliminates the expandable control buffer.

Review URL: http://codereview.chromium.org/10083050

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132866 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 51 insertions, 128 deletions

diff --git a/net/spdy/spdy_framer.cc b/net/spdy/spdy_framer.cc
index 69a0e8b..6eb2163 100644
--- a/net/spdy/spdy_framer.cc
+++ b/net/spdy/spdy_framer.cc
@@ -56,26 +56,12 @@ bool g_enable_compression_default = true;
 
 }  // namespace
 
-// The initial size of the control frame buffer; this is used internally
-// as we parse through control frames. (It is exposed here for unit test
-// purposes.)
-size_t SpdyFramer::kControlFrameBufferInitialSize = 32 * 1024;
-
-// The maximum size of the control frame buffer that we support.
-// TODO(mbelshe): We should make this stream-based so there are no limits.
-size_t SpdyFramer::kControlFrameBufferMaxSize = 64 * 1024;
-
-// The initial size of the control frame buffer when compression is disabled.
-// This exists because we don't do stream (de)compressed control frame data to
-// our visitor; we instead buffer the entirety of the control frame and then
-// decompress in one fell swoop.
-// Since this is only used for control frame headers, the maximum control
-// frame header size (18B) is sufficient; all remaining control frame data is
-// streamed to the visitor.
-size_t SpdyFramer::kUncompressedControlFrameBufferInitialSize = 18;
-
 const SpdyStreamId SpdyFramer::kInvalidStream = -1;
 const size_t SpdyFramer::kHeaderDataChunkMaxSize = 1024;
+const size_t SpdyFramer::kControlFrameBufferSize =
+    sizeof(SpdySynStreamControlFrameBlock);
+const size_t SpdyFramer::kMaxControlFrameSize = 16 * 1024;
+
 
 #ifdef DEBUG_SPDY_STATE_CHANGES
 #define CHANGE_STATE(newstate) \
@@ -132,9 +118,8 @@ SpdyFramer::SpdyFramer(int version)
       remaining_data_(0),
       remaining_control_payload_(0),
       remaining_control_header_(0),
-      current_frame_buffer_(NULL),
+      current_frame_buffer_(new char[kControlFrameBufferSize]),
       current_frame_len_(0),
-      current_frame_capacity_(0),
       validate_control_frame_sizes_(true),
       enable_compression_(g_enable_compression_default),
       visitor_(NULL),
@@ -154,7 +139,6 @@ SpdyFramer::~SpdyFramer() {
     inflateEnd(header_decompressor_.get());
   }
   CleanupStreamCompressorsAndDecompressors();
-  delete [] current_frame_buffer_;
 }
 
 void SpdyFramer::Reset() {
@@ -165,17 +149,6 @@ void SpdyFramer::Reset() {
   remaining_control_header_ = 0;
   current_frame_len_ = 0;
   settings_scratch_.Reset();
-  // TODO(hkhalil): Remove once initial_size == kControlFrameBufferInitialSize.
-  size_t initial_size = kControlFrameBufferInitialSize;
-  if (!enable_compression_) {
-    initial_size = kUncompressedControlFrameBufferInitialSize;
-  }
-  if (current_frame_capacity_ != initial_size) {
-    delete [] current_frame_buffer_;
-    current_frame_buffer_ = NULL;
-    current_frame_capacity_ = 0;
-    ExpandControlFrameBuffer(initial_size);
-  }
 }
 
 const char* SpdyFramer::StateToString(int state) {
@@ -383,7 +356,7 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
   DCHECK_EQ(state_, SPDY_READING_COMMON_HEADER);
 
   size_t original_len = len;
-  SpdyFrame current_frame(current_frame_buffer_, false);
+  SpdyFrame current_frame(current_frame_buffer_.get(), false);
 
   // Update current frame buffer as needed.
   if (current_frame_len_ < SpdyFrame::kHeaderSize) {
@@ -397,7 +370,7 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
              !current_frame.is_control_frame() &&
              current_frame.length() == 0) {
     // Empty data frame.
-    SpdyDataFrame data_frame(current_frame_buffer_, false);
+    SpdyDataFrame data_frame(current_frame_buffer_.get(), false);
     visitor_->OnDataFrameHeader(&data_frame);
     if (current_frame.flags() & DATA_FLAG_FIN) {
       visitor_->OnStreamFrameData(data_frame.stream_id(), NULL, 0);
@@ -411,7 +384,7 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
       // The strncmp for 5 is safe because we only hit this point if we
       // have SpdyFrame::kHeaderSize (8) bytes
       if (!syn_frame_processed_ &&
-          strncmp(current_frame_buffer_, "HTTP/", 5) == 0) {
+          strncmp(current_frame_buffer_.get(), "HTTP/", 5) == 0) {
         LOG(WARNING) << "Unexpected HTTP response to spdy request";
         probable_http_response_ = true;
       } else {
@@ -422,7 +395,7 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
 
     // if we're here, then we have the common header all received.
     if (!current_frame.is_control_frame()) {
-      SpdyDataFrame data_frame(current_frame_buffer_, false);
+      SpdyDataFrame data_frame(current_frame_buffer_.get(), false);
       visitor_->OnDataFrameHeader(&data_frame);
       CHANGE_STATE(SPDY_FORWARD_STREAM_FRAME);
     } else {
@@ -435,7 +408,7 @@ size_t SpdyFramer::ProcessCommonHeader(const char* data, size_t len) {
 void SpdyFramer::ProcessControlFrameHeader() {
   DCHECK_EQ(SPDY_NO_ERROR, error_code_);
   DCHECK_LE(static_cast<size_t>(SpdyFrame::kHeaderSize), current_frame_len_);
-  SpdyControlFrame current_control_frame(current_frame_buffer_, false);
+  SpdyControlFrame current_control_frame(current_frame_buffer_.get(), false);
 
   // We check version before we check validity: version can never be 'invalid',
   // it can only be unsupported.
@@ -533,8 +506,11 @@ void SpdyFramer::ProcessControlFrameHeader() {
   }
 
   remaining_control_payload_ = current_control_frame.length();
-  if (remaining_control_payload_ >
-      kControlFrameBufferMaxSize - SpdyFrame::kHeaderSize) {
+  const size_t total_frame_size =
+      remaining_control_payload_ + SpdyFrame::kHeaderSize;
+  if (total_frame_size > kMaxControlFrameSize) {
+    DLOG(WARNING) << "Received control frame with way too big of a payload: "
+                  << total_frame_size;
     set_error(SPDY_CONTROL_PAYLOAD_TOO_LARGE);
     return;
   }
@@ -575,13 +551,15 @@ void SpdyFramer::ProcessControlFrameHeader() {
       break;
   }
 
-  if (frame_size_without_variable_data == -1) {
-    LOG_IF(ERROR, remaining_control_payload_ + SpdyFrame::kHeaderSize >
-           current_frame_capacity_)
-        << display_protocol_
-        << " control frame buffer too small for fixed-length frame.";
-    // TODO(hkhalil): Remove ExpandControlFrameBuffer().
-    ExpandControlFrameBuffer(remaining_control_payload_);
+  if ((frame_size_without_variable_data == -1) &&
+      (total_frame_size > kControlFrameBufferSize)) {
+    DCHECK_EQ(SPDY_ERROR, state_);
+    if (state_ != SPDY_ERROR) {
+      LOG(DFATAL) << display_protocol_
+                  << " control frame buffer too small for fixed-length frame.";
+      set_error(SPDY_CONTROL_PAYLOAD_TOO_LARGE);
+    }
+    return;
   }
   if (frame_size_without_variable_data > 0) {
     // We have a control frame with a header block. We need to parse the
@@ -603,8 +581,10 @@ void SpdyFramer::ProcessControlFrameHeader() {
 size_t SpdyFramer::UpdateCurrentFrameBuffer(const char** data, size_t* len,
                                             size_t max_bytes) {
   size_t bytes_to_read = std::min(*len, max_bytes);
-  DCHECK_GE(current_frame_capacity_, current_frame_len_ + bytes_to_read);
-  memcpy(&current_frame_buffer_[current_frame_len_], *data, bytes_to_read);
+  DCHECK_GE(kControlFrameBufferSize, current_frame_len_ + bytes_to_read);
+  memcpy(current_frame_buffer_.get() + current_frame_len_,
+         *data,
+         bytes_to_read);
   current_frame_len_ += bytes_to_read;
   *data += bytes_to_read;
   *len -= bytes_to_read;
@@ -662,7 +642,7 @@ size_t SpdyFramer::ProcessControlFrameBeforeHeaderBlock(const char* data,
                                                  remaining_control_header_);
     remaining_control_header_ -= bytes_read;
     if (remaining_control_header_ == 0) {
-      SpdyControlFrame control_frame(current_frame_buffer_, false);
+      SpdyControlFrame control_frame(current_frame_buffer_.get(), false);
       DCHECK(control_frame.type() == SYN_STREAM ||
              control_frame.type() == SYN_REPLY ||
              control_frame.type() == HEADERS ||
@@ -686,7 +666,7 @@ size_t SpdyFramer::ProcessControlFrameBeforeHeaderBlock(const char* data,
 size_t SpdyFramer::ProcessControlFrameHeaderBlock(const char* data,
                                                   size_t data_len) {
   DCHECK_EQ(SPDY_CONTROL_FRAME_HEADER_BLOCK, state_);
-  SpdyControlFrame control_frame(current_frame_buffer_, false);
+  SpdyControlFrame control_frame(current_frame_buffer_.get(), false);
   bool processed_successfully = true;
   DCHECK(control_frame.type() == SYN_STREAM ||
          control_frame.type() == SYN_REPLY ||
@@ -731,7 +711,7 @@ size_t SpdyFramer::ProcessControlFrameHeaderBlock(const char* data,
 size_t SpdyFramer::ProcessSettingsFramePayload(const char* data,
                                                size_t data_len) {
   DCHECK_EQ(SPDY_SETTINGS_FRAME_PAYLOAD, state_);
-  SpdyControlFrame control_frame(current_frame_buffer_, false);
+  SpdyControlFrame control_frame(current_frame_buffer_.get(), false);
   DCHECK_EQ(control_frame.type(), SETTINGS);
   size_t unprocessed_bytes = std::min(data_len, remaining_control_payload_);
   size_t processed_bytes = 0;
@@ -840,7 +820,7 @@ size_t SpdyFramer::ProcessControlFramePayload(const char* data, size_t len) {
     remaining_control_payload_ -= bytes_read;
     remaining_data_ -= bytes_read;
     if (remaining_control_payload_ == 0) {
-      SpdyControlFrame control_frame(current_frame_buffer_, false);
+      SpdyControlFrame control_frame(current_frame_buffer_.get(), false);
       DCHECK(!control_frame.has_header_block());
       visitor_->OnControl(&control_frame);
 
@@ -868,7 +848,7 @@ size_t SpdyFramer::ProcessCredentialFramePayload(const char* data, size_t len) {
 size_t SpdyFramer::ProcessDataFramePayload(const char* data, size_t len) {
   size_t original_len = len;
 
-  SpdyDataFrame current_data_frame(current_frame_buffer_, false);
+  SpdyDataFrame current_data_frame(current_frame_buffer_.get(), false);
   if (remaining_data_) {
     size_t amount_to_forward = std::min(remaining_data_, len);
     if (amount_to_forward && state_ != SPDY_IGNORE_REMAINING_PAYLOAD) {
@@ -927,20 +907,6 @@ size_t SpdyFramer::ProcessDataFramePayload(const char* data, size_t len) {
   return original_len - len;
 }
 
-void SpdyFramer::ExpandControlFrameBuffer(size_t size) {
-  size_t alloc_size = size + SpdyFrame::kHeaderSize;
-  DCHECK_LE(alloc_size, kControlFrameBufferMaxSize);
-  if (alloc_size <= current_frame_capacity_)
-    return;
-  char* new_buffer = new char[alloc_size];
-  if (current_frame_buffer_ != NULL) {
-    memcpy(new_buffer, current_frame_buffer_, current_frame_len_);
-    delete [] current_frame_buffer_;
-  }
-  current_frame_capacity_ = alloc_size;
-  current_frame_buffer_ = new_buffer;
-}
-
 bool SpdyFramer::ParseHeaderBlockInBuffer(const char* header_data,
                                           size_t header_length,
                                           SpdyHeaderBlock* block) {
diff --git a/net/spdy/spdy_framer.h b/net/spdy/spdy_framer.h
index faaf404..d98f253a 100644
--- a/net/spdy/spdy_framer.h
+++ b/net/spdy/spdy_framer.h
@@ -505,9 +505,6 @@ class NET_EXPORT_PRIVATE SpdyFramer {
   // Set the error code and moves the framer into the error state.
   void set_error(SpdyError error);
 
-  // Expands the control frame buffer to accomodate a particular payload size.
-  void ExpandControlFrameBuffer(size_t size);
-
   // Given a frame, breakdown the variable payload length, the static header
   // header length, and variable payload pointer.
   bool GetFrameBoundaries(const SpdyFrame& frame, int* payload_length,
@@ -516,27 +513,17 @@ class NET_EXPORT_PRIVATE SpdyFramer {
   int num_stream_compressors() const { return stream_compressors_.size(); }
   int num_stream_decompressors() const { return stream_decompressors_.size(); }
 
-  // The initial size of the control frame buffer; this is used internally
-  // as we parse through control frames. (It is exposed here for unit test
-  // purposes.)
-  // This is only used when compression is enabled; otherwise,
-  // kUncompressedControlFrameBufferInitialSize is used.
-  static size_t kControlFrameBufferInitialSize;
-
-  // The initial size of the control frame buffer when compression is disabled.
-  // This exists because we don't do stream (de)compressed control frame data to
-  // our visitor; we instead buffer the entirety of the control frame and then
-  // decompress in one fell swoop.
+  // The size of the control frame buffer.
   // Since this is only used for control frame headers, the maximum control
-  // frame header size (18B) is sufficient; all remaining control frame data is
-  // streamed to the visitor.
-  // TODO(hkhalil): Remove post code-yellow once streamed inflate is properly
-  // implemented.
-  static size_t kUncompressedControlFrameBufferInitialSize;
+  // frame header size (SYN_STREAM) is sufficient; all remaining control
+  // frame data is streamed to the visitor.
+  static const size_t kControlFrameBufferSize;
 
-  // The maximum size of the control frame buffer that we support.
-  // TODO(mbelshe): We should make this stream-based so there are no limits.
-  static size_t kControlFrameBufferMaxSize;
+  // The maximum size of the control frames that we support.
+  // This limit is arbitrary. We can enforce it here or at the application
+  // layer. We chose the framing layer, but this can be changed (or removed)
+  // if necessary later down the line.
+  static const size_t kMaxControlFrameSize;
 
   SpdyState state_;
   SpdyError error_code_;
@@ -551,9 +538,8 @@ class NET_EXPORT_PRIVATE SpdyFramer {
   // are part of the frame's payload, and not the frame's headers.
   size_t remaining_control_header_;
 
-  char* current_frame_buffer_;
+  scoped_array<char> current_frame_buffer_;
   size_t current_frame_len_;  // Number of bytes read into the current_frame_.
-  size_t current_frame_capacity_;
 
   // Scratch space for handling SETTINGS frames.
   // TODO(hkhalil): Unify memory for this scratch space with
@@ -574,6 +560,10 @@ class NET_EXPORT_PRIVATE SpdyFramer {
 
   std::string display_protocol_;
 
+  // The SPDY version to be spoken/understood by this framer. We support only
+  // integer versions here, as major version numbers indicate framer-layer
+  // incompatibility and minor version numbers indicate application-layer
+  // incompatibility.
   const int spdy_version_;
 
   // Tracks if we've ever gotten far enough in framing to see a control frame of
diff --git a/net/spdy/spdy_framer_test.cc b/net/spdy/spdy_framer_test.cc
index c4a0a1b..738eacf 100644
--- a/net/spdy/spdy_framer_test.cc
+++ b/net/spdy/spdy_framer_test.cc
@@ -387,7 +387,7 @@ class TestSpdyVisitor : public SpdyFramerVisitorInterface  {
   }
 
   static size_t control_frame_buffer_max_size() {
-    return SpdyFramer::kControlFrameBufferMaxSize;
+    return SpdyFramer::kControlFrameBufferSize;
   }
 
   static size_t header_data_chunk_max_size() {
@@ -2240,44 +2240,11 @@ TEST_P(SpdyFramerTest, DuplicateFrame) {
   }
 }
 
-// This test case reproduces conditions that caused ExpandControlFrameBuffer to
-// fail to expand the buffer control frame buffer when it should have, allowing
-// the framer to overrun the buffer, and smash other heap contents. This test
-// relies on the debug version of the heap manager, which checks for buffer
-// overrun errors during delete processing. Regression test for b/2974814.
-TEST_P(SpdyFramerTest, ExpandBuffer_HeapSmash) {
-  // Sweep through the area of problematic values, to make sure we always cover
-  // the danger zone, even if it moves around at bit due to SPDY changes.
-  for (uint16 val2_len = SpdyFramer::kControlFrameBufferInitialSize - 50;
-       val2_len < SpdyFramer::kControlFrameBufferInitialSize;
-       val2_len++) {
-    std::string val2 = std::string(val2_len, 'a');
-    SpdyHeaderBlock headers;
-    headers["bar"] = "foo";
-    headers["foo"] = "baz";
-    headers["grue"] = val2.c_str();
-    SpdyFramer framer(spdy_version_);
-    scoped_ptr<SpdySynStreamControlFrame> template_frame(
-        framer.CreateSynStream(1,                      // stream_id
-                               0,                      // associated_stream_id
-                               1,                      // priority
-                               0,                      // credential slot
-                               CONTROL_FLAG_NONE,
-                               false,                  // compress
-                               &headers));
-    EXPECT_TRUE(template_frame.get() != NULL);
-    TestSpdyVisitor visitor(spdy_version_);
-    visitor.SimulateInFramer(
-        reinterpret_cast<unsigned char*>(template_frame.get()->data()),
-         template_frame.get()->length() + SpdyControlFrame::kHeaderSize);
-    EXPECT_EQ(1, visitor.syn_frame_count_);
-  }
-}
-
 TEST_P(SpdyFramerTest, ControlFrameSizesAreValidated) {
   // Create a GoAway frame that has a few extra bytes at the end.
   // We create enough overhead to overflow the framer's control frame buffer.
-  size_t overhead = SpdyFramer::kUncompressedControlFrameBufferInitialSize;
+  size_t overhead = SpdyFramer::kControlFrameBufferSize;
+
   SpdyFramer framer(spdy_version_);
   scoped_ptr<SpdyGoAwayControlFrame> goaway(framer.CreateGoAway(1, GOAWAY_OK));
   goaway->set_length(goaway->length() + overhead);
@@ -2340,7 +2307,7 @@ TEST_P(SpdyFramerTest, ReadLargeSettingsFrame) {
       SettingsFlagsAndValue(flags, 0x00000003);
   settings[SETTINGS_ROUND_TRIP_TIME] = SettingsFlagsAndValue(flags, 0x00000004);
   scoped_ptr<SpdyFrame> control_frame(framer.CreateSettings(settings));
-  EXPECT_LT(SpdyFramer::kUncompressedControlFrameBufferInitialSize,
+  EXPECT_LT(SpdyFramer::kControlFrameBufferSize,
             control_frame->length() + SpdyControlFrame::kHeaderSize);
   TestSpdyVisitor visitor(spdy_version_);
   visitor.use_compression_ = false;