Signal error in SpdyFramer if receive a header block frame with stream ID zero

This lands server change 48914206.

R=rch@chromium.org

Review URL: https://codereview.chromium.org/19678026

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

2 files changed, 143 insertions, 0 deletions

diff --git a/net/spdy/spdy_framer.cc b/net/spdy/spdy_framer.cc
index e081595..cfeb86e 100644
--- a/net/spdy/spdy_framer.cc
+++ b/net/spdy/spdy_framer.cc
@@ -1097,6 +1097,10 @@ size_t SpdyFramer::ProcessControlFrameBeforeHeaderBlock(const char* data,
             successful_read = reader.ReadUInt31(&current_frame_stream_id_);
             DCHECK(successful_read);
           }
+          if (current_frame_stream_id_ == 0) {
+            set_error(SPDY_INVALID_CONTROL_FRAME);
+            break;
+          }
 
           SpdyStreamId associated_to_stream_id = kInvalidStream;
           successful_read = reader.ReadUInt31(&associated_to_stream_id);
@@ -1140,10 +1144,18 @@ size_t SpdyFramer::ProcessControlFrameBeforeHeaderBlock(const char* data,
       case PUSH_PROMISE:
         {
           DCHECK_LE(4, protocol_version());
+          if (current_frame_stream_id_ == 0) {
+            set_error(SPDY_INVALID_CONTROL_FRAME);
+            break;
+          }
           SpdyStreamId promised_stream_id = kInvalidStream;
           bool successful_read = reader.ReadUInt31(&promised_stream_id);
           DCHECK(successful_read);
           DCHECK(reader.IsDoneReading());
+          if (promised_stream_id == 0) {
+            set_error(SPDY_INVALID_CONTROL_FRAME);
+            break;
+          }
           if (debug_visitor_) {
             debug_visitor_->OnReceiveCompressedFrame(
                 current_frame_stream_id_,
@@ -1163,6 +1175,10 @@ size_t SpdyFramer::ProcessControlFrameBeforeHeaderBlock(const char* data,
             successful_read = reader.ReadUInt31(&current_frame_stream_id_);
             DCHECK(successful_read);
           }
+          if (current_frame_stream_id_ == 0) {
+            set_error(SPDY_INVALID_CONTROL_FRAME);
+            break;
+          }
           if (protocol_version() < 3) {
             // SPDY 2 had two unused bytes here. Seek past them.
             reader.Seek(2);
@@ -2175,6 +2191,9 @@ bool SpdyFramer::IncrementallyDecompressControlFrameHeaderData(
 
   decomp->next_in = reinterpret_cast<Bytef*>(const_cast<char*>(data));
   decomp->avail_in = len;
+  // If we get a SYN_STREAM/SYN_REPLY/HEADERS frame with stream ID zero, we
+  // signal an error back in ProcessControlFrameBeforeHeaderBlock.  So if we've
+  // reached this method successfully, stream_id should be nonzero.
   DCHECK_LT(0u, stream_id);
   while (decomp->avail_in > 0 && processed_successfully) {
     decomp->next_out = reinterpret_cast<Bytef*>(buffer);
diff --git a/net/spdy/spdy_framer_test.cc b/net/spdy/spdy_framer_test.cc
index 48f76366..bc8b505 100644
--- a/net/spdy/spdy_framer_test.cc
+++ b/net/spdy/spdy_framer_test.cc
@@ -751,6 +751,130 @@ TEST_P(SpdyFramerTest, OutOfOrderHeaders) {
                                               &new_headers));
 }
 
+// Test that if we receive a SYN_STREAM with stream ID zero, we signal an error
+// (but don't crash).
+TEST_P(SpdyFramerTest, SynStreamWithStreamIdZero) {
+  testing::StrictMock<net::test::MockVisitor> visitor;
+  SpdyFramer framer(spdy_version_);
+  framer.set_visitor(&visitor);
+
+  SpdyHeaderBlock headers;
+  headers["alpha"] = "beta";
+  scoped_ptr<SpdySerializedFrame> frame(
+      framer.CreateSynStream(0,  // stream id
+                             0,  // associated stream id
+                             1,  // priority
+                             0,  // credential slot
+                             CONTROL_FLAG_NONE,
+                             true,  // compress
+                             &headers));
+  ASSERT_TRUE(frame.get() != NULL);
+
+  // We shouldn't have to read the whole frame before we signal an error.
+  EXPECT_CALL(visitor, OnError(testing::Eq(&framer)));
+  EXPECT_GT(frame->size(), framer.ProcessInput(frame->data(), frame->size()));
+  EXPECT_TRUE(framer.HasError());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME, framer.error_code());
+}
+
+// Test that if we receive a SYN_REPLY with stream ID zero, we signal an error
+// (but don't crash).
+TEST_P(SpdyFramerTest, SynReplyWithStreamIdZero) {
+  testing::StrictMock<net::test::MockVisitor> visitor;
+  SpdyFramer framer(spdy_version_);
+  framer.set_visitor(&visitor);
+
+  SpdyHeaderBlock headers;
+  headers["alpha"] = "beta";
+  scoped_ptr<SpdySerializedFrame> frame(
+      framer.CreateSynReply(0,  // stream id
+                            CONTROL_FLAG_NONE,
+                            true,  // compress
+                            &headers));
+  ASSERT_TRUE(frame.get() != NULL);
+
+  // We shouldn't have to read the whole frame before we signal an error.
+  EXPECT_CALL(visitor, OnError(testing::Eq(&framer)));
+  EXPECT_GT(frame->size(), framer.ProcessInput(frame->data(), frame->size()));
+  EXPECT_TRUE(framer.HasError());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME, framer.error_code());
+}
+
+// Test that if we receive a HEADERS with stream ID zero, we signal an error
+// (but don't crash).
+TEST_P(SpdyFramerTest, HeadersWithStreamIdZero) {
+  testing::StrictMock<net::test::MockVisitor> visitor;
+  SpdyFramer framer(spdy_version_);
+  framer.set_visitor(&visitor);
+
+  SpdyHeaderBlock headers;
+  headers["alpha"] = "beta";
+  scoped_ptr<SpdySerializedFrame> frame(
+      framer.CreateHeaders(0,  // stream id
+                           CONTROL_FLAG_NONE,
+                           true,  // compress
+                           &headers));
+  ASSERT_TRUE(frame.get() != NULL);
+
+  // We shouldn't have to read the whole frame before we signal an error.
+  EXPECT_CALL(visitor, OnError(testing::Eq(&framer)));
+  EXPECT_GT(frame->size(), framer.ProcessInput(frame->data(), frame->size()));
+  EXPECT_TRUE(framer.HasError());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME, framer.error_code());
+}
+
+// Test that if we receive a PUSH_PROMISE with stream ID zero, we signal an
+// error (but don't crash).
+TEST_P(SpdyFramerTest, PushPromiseWithStreamIdZero) {
+  if (spdy_version_ < SPDY4) {
+    return;
+  }
+
+  testing::StrictMock<net::test::MockVisitor> visitor;
+  SpdyFramer framer(spdy_version_);
+  framer.set_visitor(&visitor);
+
+  SpdyHeaderBlock headers;
+  headers["alpha"] = "beta";
+  scoped_ptr<SpdySerializedFrame> frame(
+      framer.CreatePushPromise(0,  // stream id
+                               4,  // promised stream id
+                               &headers));
+  ASSERT_TRUE(frame.get() != NULL);
+
+  // We shouldn't have to read the whole frame before we signal an error.
+  EXPECT_CALL(visitor, OnError(testing::Eq(&framer)));
+  EXPECT_GT(frame->size(), framer.ProcessInput(frame->data(), frame->size()));
+  EXPECT_TRUE(framer.HasError());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME, framer.error_code());
+}
+
+// Test that if we receive a PUSH_PROMISE with promised stream ID zero, we
+// signal an error (but don't crash).
+TEST_P(SpdyFramerTest, PushPromiseWithPromisedStreamIdZero) {
+  if (spdy_version_ < SPDY4) {
+    return;
+  }
+
+  testing::StrictMock<net::test::MockVisitor> visitor;
+  SpdyFramer framer(spdy_version_);
+  framer.set_visitor(&visitor);
+
+  SpdyHeaderBlock headers;
+  headers["alpha"] = "beta";
+  scoped_ptr<SpdySerializedFrame> frame(
+      framer.CreatePushPromise(3,  // stream id
+                               0,  // promised stream id
+                               &headers));
+  ASSERT_TRUE(frame.get() != NULL);
+
+  // We shouldn't have to read the whole frame before we signal an error.
+  EXPECT_CALL(visitor, OnError(testing::Eq(&framer)));
+  EXPECT_GT(frame->size(), framer.ProcessInput(frame->data(), frame->size()));
+  EXPECT_TRUE(framer.HasError());
+  EXPECT_EQ(SpdyFramer::SPDY_INVALID_CONTROL_FRAME, framer.error_code());
+}
+
 TEST_P(SpdyFramerTest, CreateCredential) {
   SpdyFramer framer(spdy_version_);