SPDY - Handle incomplete headers during server push.

If we receive a data frame without a status or without
a version header, we close the stream with a PROTOCOL ERROR.

Small bug fix to HttpNetworkTransaction to access the
ResponseHeaders only if headers are there.

In SpdyStream, retrun a SPDY_PROTOCOL_ERROR if we have pending
data frames, but we haven't received "status" and "version"
headers. (rch: removed the DCHECK for unit tests).

SpdyHttpStream's OnDataReceived used to expect that it would
be called only when all required headers are received. Converted
the DCHECK into an error condition and SpdyStream closes the
stream with PROTOCOL ERROR if OnDataReceived returns a network
error.

BUG=135485
R=rch@chromium.org
TEST=network unittests

Review URL: https://chromiumcodereview.appspot.com/10836084

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

14 files changed, 284 insertions, 16 deletions

diff --git a/net/spdy/spdy_http_stream.cc b/net/spdy/spdy_http_stream.cc
index 987f9eb..3c40a1a 100644
--- a/net/spdy/spdy_http_stream.cc
+++ b/net/spdy/spdy_http_stream.cc
@@ -411,11 +411,12 @@ int SpdyHttpStream::OnResponseReceived(const SpdyHeaderBlock& response,
   return status;
 }
 
-void SpdyHttpStream::OnDataReceived(const char* data, int length) {
+int SpdyHttpStream::OnDataReceived(const char* data, int length) {
   // SpdyStream won't call us with data if the header block didn't contain a
   // valid set of headers.  So we don't expect to not have headers received
   // here.
-  DCHECK(response_headers_received_);
+  if (!response_headers_received_)
+    return ERR_INCOMPLETE_SPDY_HEADERS;
 
   // Note that data may be received for a SpdyStream prior to the user calling
   // ReadResponseBody(), therefore user_buffer_ may be NULL.  This may often
@@ -433,6 +434,7 @@ void SpdyHttpStream::OnDataReceived(const char* data, int length) {
       ScheduleBufferedReadCallback();
     }
   }
+  return OK;
 }
 
 void SpdyHttpStream::OnDataSent(int length) {
diff --git a/net/spdy/spdy_http_stream.h b/net/spdy/spdy_http_stream.h
index 97cd173..a0fedaf 100644
--- a/net/spdy/spdy_http_stream.h
+++ b/net/spdy/spdy_http_stream.h
@@ -81,7 +81,7 @@ class NET_EXPORT_PRIVATE SpdyHttpStream : public SpdyStream::Delegate,
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
-  virtual void OnDataReceived(const char* buffer, int bytes) OVERRIDE;
+  virtual int OnDataReceived(const char* buffer, int bytes) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;
 
diff --git a/net/spdy/spdy_network_transaction_spdy2_unittest.cc b/net/spdy/spdy_network_transaction_spdy2_unittest.cc
index 06e64ba..2ed9500 100644
--- a/net/spdy/spdy_network_transaction_spdy2_unittest.cc
+++ b/net/spdy/spdy_network_transaction_spdy2_unittest.cc
@@ -5130,6 +5130,129 @@ TEST_P(SpdyNetworkTransactionSpdy2Test, ServerPushWithTwoHeaderFrames) {
   EXPECT_TRUE(data.at_write_eof());
 }
 
+TEST_P(SpdyNetworkTransactionSpdy2Test, ServerPushWithNoStatusHeaderFrames) {
+  // We push a stream and attempt to claim it before the headers come down.
+  static const unsigned char kPushBodyFrame[] = {
+    0x00, 0x00, 0x00, 0x02,                                      // header, ID
+    0x01, 0x00, 0x00, 0x06,                                      // FIN, length
+    'p', 'u', 's', 'h', 'e', 'd'                                 // "pushed"
+  };
+  scoped_ptr<SpdyFrame>
+      stream1_syn(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
+  scoped_ptr<SpdyFrame>
+      stream1_body(ConstructSpdyBodyFrame(1, true));
+  MockWrite writes[] = {
+    CreateMockWrite(*stream1_syn, 0, SYNCHRONOUS),
+  };
+
+  static const char* const kInitialHeaders[] = {
+    "url",
+    "http://www.google.com/foo.dat",
+  };
+  static const char* const kMiddleHeaders[] = {
+    "hello",
+    "bye",
+  };
+  scoped_ptr<SpdyFrame>
+      stream2_syn(ConstructSpdyControlFrame(kInitialHeaders,
+                                            arraysize(kInitialHeaders) / 2,
+                                            false,
+                                            2,
+                                            LOWEST,
+                                            SYN_STREAM,
+                                            CONTROL_FLAG_NONE,
+                                            NULL,
+                                            0,
+                                            1));
+  scoped_ptr<SpdyFrame>
+      stream2_headers1(ConstructSpdyControlFrame(kMiddleHeaders,
+                                                 arraysize(kMiddleHeaders) / 2,
+                                                 false,
+                                                 2,
+                                                 LOWEST,
+                                                 HEADERS,
+                                                 CONTROL_FLAG_NONE,
+                                                 NULL,
+                                                 0,
+                                                 0));
+
+  scoped_ptr<SpdyFrame>
+      stream1_reply(ConstructSpdyGetSynReply(NULL, 0, 1));
+  MockRead reads[] = {
+    CreateMockRead(*stream1_reply, 1),
+    CreateMockRead(*stream2_syn, 2),
+    CreateMockRead(*stream1_body, 3),
+    CreateMockRead(*stream2_headers1, 4),
+    MockRead(ASYNC, reinterpret_cast<const char*>(kPushBodyFrame),
+             arraysize(kPushBodyFrame), 5),
+    MockRead(ASYNC, 0, 6),  // EOF
+  };
+
+  DeterministicSocketData data(reads, arraysize(reads),
+                               writes, arraysize(writes));
+
+  NormalSpdyTransactionHelper helper(CreateGetRequest(),
+                                     BoundNetLog(), GetParam(), NULL);
+  helper.SetDeterministic();
+  helper.AddDeterministicData(&data);
+  helper.RunPreTestSetup();
+
+  HttpNetworkTransaction* trans = helper.trans();
+
+  // Run until we've received the primary SYN_STREAM, the pushed SYN_STREAM,
+  // the first HEADERS frame, and the body of the primary stream, but before
+  // we've received the final HEADERS for the pushed stream.
+  data.SetStop(4);
+
+  // Start the transaction.
+  TestCompletionCallback callback;
+  int rv = trans->Start(
+      &CreateGetRequest(), callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  data.Run();
+  rv = callback.WaitForResult();
+  EXPECT_EQ(0, rv);
+
+  // Request the pushed path.  At this point, we've received the push, but the
+  // headers are not yet complete.
+  scoped_ptr<HttpNetworkTransaction> trans2(
+      new HttpNetworkTransaction(helper.session()));
+  rv = trans2->Start(
+      &CreateGetPushRequest(), callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  data.RunFor(2);
+  MessageLoop::current()->RunAllPending();
+
+  // Read the server push body.
+  std::string result2;
+  ReadResult(trans2.get(), &data, &result2);
+  // Read the response body.
+  std::string result;
+  ReadResult(trans, &data, &result);
+  EXPECT_EQ("hello!", result);
+
+  // Verify that we haven't received any push data.
+  EXPECT_EQ("", result2);
+
+  // Verify the SYN_REPLY.
+  // Copy the response info, because trans goes away.
+  HttpResponseInfo response = *trans->GetResponseInfo();
+  ASSERT_TRUE(trans2->GetResponseInfo() == NULL);
+
+  VerifyStreamsClosed(helper);
+
+  // Verify the SYN_REPLY.
+  EXPECT_TRUE(response.headers != NULL);
+  EXPECT_EQ("HTTP/1.1 200 OK", response.headers->GetStatusLine());
+
+  // Read the final EOF (which will close the session).
+  data.RunFor(1);
+
+  // Verify that we consumed all test data.
+  EXPECT_TRUE(data.at_read_eof());
+  EXPECT_TRUE(data.at_write_eof());
+}
+
 TEST_P(SpdyNetworkTransactionSpdy2Test, SynReplyWithHeaders) {
   scoped_ptr<SpdyFrame> req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
   MockWrite writes[] = { CreateMockWrite(*req) };
diff --git a/net/spdy/spdy_network_transaction_spdy3_unittest.cc b/net/spdy/spdy_network_transaction_spdy3_unittest.cc
index 9fd4c04..3ef9a85 100644
--- a/net/spdy/spdy_network_transaction_spdy3_unittest.cc
+++ b/net/spdy/spdy_network_transaction_spdy3_unittest.cc
@@ -5689,6 +5689,130 @@ TEST_P(SpdyNetworkTransactionSpdy3Test, ServerPushWithTwoHeaderFrames) {
   EXPECT_TRUE(data.at_write_eof());
 }
 
+TEST_P(SpdyNetworkTransactionSpdy3Test, ServerPushWithNoStatusHeaderFrames) {
+  // We push a stream and attempt to claim it before the headers come down.
+  static const unsigned char kPushBodyFrame[] = {
+    0x00, 0x00, 0x00, 0x02,                                      // header, ID
+    0x01, 0x00, 0x00, 0x06,                                      // FIN, length
+    'p', 'u', 's', 'h', 'e', 'd'                                 // "pushed"
+  };
+  scoped_ptr<SpdyFrame>
+      stream1_syn(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
+  scoped_ptr<SpdyFrame>
+      stream1_body(ConstructSpdyBodyFrame(1, true));
+  MockWrite writes[] = {
+    CreateMockWrite(*stream1_syn, 0, SYNCHRONOUS),
+  };
+
+  static const char* const kInitialHeaders[] = {
+    ":scheme", "http",
+    ":host", "www.google.com",
+    ":path", "/foo.dat"
+  };
+  static const char* const kMiddleHeaders[] = {
+    "hello",
+    "bye",
+  };
+  scoped_ptr<SpdyFrame>
+      stream2_syn(ConstructSpdyControlFrame(kInitialHeaders,
+                                            arraysize(kInitialHeaders) / 2,
+                                            false,
+                                            2,
+                                            LOWEST,
+                                            SYN_STREAM,
+                                            CONTROL_FLAG_NONE,
+                                            NULL,
+                                            0,
+                                            1));
+  scoped_ptr<SpdyFrame>
+      stream2_headers1(ConstructSpdyControlFrame(kMiddleHeaders,
+                                                 arraysize(kMiddleHeaders) / 2,
+                                                 false,
+                                                 2,
+                                                 LOWEST,
+                                                 HEADERS,
+                                                 CONTROL_FLAG_NONE,
+                                                 NULL,
+                                                 0,
+                                                 0));
+
+  scoped_ptr<SpdyFrame>
+      stream1_reply(ConstructSpdyGetSynReply(NULL, 0, 1));
+  MockRead reads[] = {
+    CreateMockRead(*stream1_reply, 1),
+    CreateMockRead(*stream2_syn, 2),
+    CreateMockRead(*stream1_body, 3),
+    CreateMockRead(*stream2_headers1, 4),
+    MockRead(ASYNC, reinterpret_cast<const char*>(kPushBodyFrame),
+             arraysize(kPushBodyFrame), 5),
+    MockRead(ASYNC, 0, 6),  // EOF
+  };
+
+  DeterministicSocketData data(reads, arraysize(reads),
+                               writes, arraysize(writes));
+
+  NormalSpdyTransactionHelper helper(CreateGetRequest(),
+                                     BoundNetLog(), GetParam(), NULL);
+  helper.SetDeterministic();
+  helper.AddDeterministicData(&data);
+  helper.RunPreTestSetup();
+
+  HttpNetworkTransaction* trans = helper.trans();
+
+  // Run until we've received the primary SYN_STREAM, the pushed SYN_STREAM,
+  // the first HEADERS frame, and the body of the primary stream, but before
+  // we've received the final HEADERS for the pushed stream.
+  data.SetStop(4);
+
+  // Start the transaction.
+  TestCompletionCallback callback;
+  int rv = trans->Start(
+      &CreateGetRequest(), callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  data.Run();
+  rv = callback.WaitForResult();
+  EXPECT_EQ(0, rv);
+
+  // Request the pushed path.  At this point, we've received the push, but the
+  // headers are not yet complete.
+  scoped_ptr<HttpNetworkTransaction> trans2(
+      new HttpNetworkTransaction(helper.session()));
+  rv = trans2->Start(
+      &CreateGetPushRequest(), callback.callback(), BoundNetLog());
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+  data.RunFor(2);
+  MessageLoop::current()->RunAllPending();
+
+  // Read the server push body.
+  std::string result2;
+  ReadResult(trans2.get(), &data, &result2);
+  // Read the response body.
+  std::string result;
+  ReadResult(trans, &data, &result);
+  EXPECT_EQ("hello!", result);
+
+  // Verify that we haven't received any push data.
+  EXPECT_EQ("", result2);
+
+  // Verify the SYN_REPLY.
+  // Copy the response info, because trans goes away.
+  HttpResponseInfo response = *trans->GetResponseInfo();
+  ASSERT_TRUE(trans2->GetResponseInfo() == NULL);
+
+  VerifyStreamsClosed(helper);
+
+  // Verify the SYN_REPLY.
+  EXPECT_TRUE(response.headers != NULL);
+  EXPECT_EQ("HTTP/1.1 200 OK", response.headers->GetStatusLine());
+
+  // Read the final EOF (which will close the session).
+  data.RunFor(1);
+
+  // Verify that we consumed all test data.
+  EXPECT_TRUE(data.at_read_eof());
+  EXPECT_TRUE(data.at_write_eof());
+}
+
 TEST_P(SpdyNetworkTransactionSpdy3Test, SynReplyWithHeaders) {
   scoped_ptr<SpdyFrame> req(ConstructSpdyGet(NULL, 0, false, 1, LOWEST));
   MockWrite writes[] = { CreateMockWrite(*req) };
diff --git a/net/spdy/spdy_proxy_client_socket.cc b/net/spdy/spdy_proxy_client_socket.cc
index 251ada2..cffa3e4 100644
--- a/net/spdy/spdy_proxy_client_socket.cc
+++ b/net/spdy/spdy_proxy_client_socket.cc
@@ -489,7 +489,7 @@ int SpdyProxyClientSocket::OnResponseReceived(
 }
 
 // Called when data is received.
-void SpdyProxyClientSocket::OnDataReceived(const char* data, int length) {
+int SpdyProxyClientSocket::OnDataReceived(const char* data, int length) {
   if (length > 0) {
     // Save the received data.
     scoped_refptr<IOBuffer> io_buffer(new IOBuffer(length));
@@ -505,6 +505,7 @@ void SpdyProxyClientSocket::OnDataReceived(const char* data, int length) {
     user_buffer_ = NULL;
     c.Run(rv);
   }
+  return OK;
 }
 
 void SpdyProxyClientSocket::OnDataSent(int length)  {
diff --git a/net/spdy/spdy_proxy_client_socket.h b/net/spdy/spdy_proxy_client_socket.h
index 3859c61..d98ef2d 100644
--- a/net/spdy/spdy_proxy_client_socket.h
+++ b/net/spdy/spdy_proxy_client_socket.h
@@ -97,7 +97,7 @@ class NET_EXPORT_PRIVATE SpdyProxyClientSocket : public ProxyClientSocket,
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
-  virtual void OnDataReceived(const char* data, int length) OVERRIDE;
+  virtual int OnDataReceived(const char* data, int length) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;
 
diff --git a/net/spdy/spdy_session_spdy2_unittest.cc b/net/spdy/spdy_session_spdy2_unittest.cc
index 7f96b5f..ecddf9b 100644
--- a/net/spdy/spdy_session_spdy2_unittest.cc
+++ b/net/spdy/spdy_session_spdy2_unittest.cc
@@ -43,7 +43,9 @@ class ClosingDelegate : public SpdyStream::Delegate {
                                  int status) OVERRIDE {
     return OK;
   }
-  virtual void OnDataReceived(const char* data, int length) OVERRIDE {}
+  virtual int OnDataReceived(const char* data, int length) OVERRIDE {
+    return OK;
+  }
   virtual void OnDataSent(int length) OVERRIDE {}
   virtual void OnClose(int status) OVERRIDE {
     stream_->Close();
@@ -88,7 +90,8 @@ class TestSpdyStreamDelegate : public net::SpdyStream::Delegate {
     return status;
   }
 
-  virtual void OnDataReceived(const char* buffer, int bytes) {
+  virtual int OnDataReceived(const char* buffer, int bytes) {
+    return OK;
   }
 
   virtual void OnDataSent(int length) {
diff --git a/net/spdy/spdy_session_spdy3_unittest.cc b/net/spdy/spdy_session_spdy3_unittest.cc
index ae4d54e..54c0398 100644
--- a/net/spdy/spdy_session_spdy3_unittest.cc
+++ b/net/spdy/spdy_session_spdy3_unittest.cc
@@ -43,7 +43,9 @@ class ClosingDelegate : public SpdyStream::Delegate {
                                  int status) OVERRIDE {
     return OK;
   }
-  virtual void OnDataReceived(const char* data, int length) OVERRIDE {}
+  virtual int OnDataReceived(const char* data, int length) OVERRIDE {
+    return OK;
+  }
   virtual void OnDataSent(int length) OVERRIDE {}
   virtual void OnClose(int status) OVERRIDE {
     stream_->Close();
@@ -75,7 +77,8 @@ class TestSpdyStreamDelegate : public net::SpdyStream::Delegate {
     return status;
   }
 
-  virtual void OnDataReceived(const char* buffer, int bytes) {
+  virtual int OnDataReceived(const char* buffer, int bytes) {
+    return OK;
   }
 
   virtual void OnDataSent(int length) {
diff --git a/net/spdy/spdy_stream.cc b/net/spdy/spdy_stream.cc
index 00dc01e..1e2db2c 100644
--- a/net/spdy/spdy_stream.cc
+++ b/net/spdy/spdy_stream.cc
@@ -194,7 +194,11 @@ void SpdyStream::PushedStreamReplayData() {
     // We don't have complete headers.  Assume we're waiting for another
     // HEADERS frame.  Since we don't have headers, we had better not have
     // any pending data frames.
-    DCHECK_EQ(0U, pending_buffers_.size());
+    if (pending_buffers_.size() != 0U) {
+      LogStreamError(ERR_SPDY_PROTOCOL_ERROR,
+                     "HEADERS incomplete headers, but pending data frames.");
+      session_->CloseStream(stream_id_, ERR_SPDY_PROTOCOL_ERROR);
+    }
     return;
   }
 
@@ -508,7 +512,12 @@ void SpdyStream::OnDataReceived(const char* data, int length) {
     return;
   }
 
-  delegate_->OnDataReceived(data, length);
+  if (delegate_->OnDataReceived(data, length) != net::OK) {
+    // |delegate_| rejected the data.
+    LogStreamError(ERR_SPDY_PROTOCOL_ERROR, "Delegate rejected the data");
+    session_->CloseStream(stream_id_, ERR_SPDY_PROTOCOL_ERROR);
+    return;
+  }
 }
 
 // This function is only called when an entire frame is written.
diff --git a/net/spdy/spdy_stream.h b/net/spdy/spdy_stream.h
index 0e383b3..f72f147 100644
--- a/net/spdy/spdy_stream.h
+++ b/net/spdy/spdy_stream.h
@@ -72,7 +72,8 @@ class NET_EXPORT_PRIVATE SpdyStream
                                    int status) = 0;
 
     // Called when data is received.
-    virtual void OnDataReceived(const char* data, int length) = 0;
+    // Returns network error code. OK when it successfully receives data.
+    virtual int OnDataReceived(const char* data, int length) = 0;
 
     // Called when data is sent.
     virtual void OnDataSent(int length) = 0;
diff --git a/net/spdy/spdy_stream_test_util.cc b/net/spdy/spdy_stream_test_util.cc
index b973cf8..c39c46c 100644
--- a/net/spdy/spdy_stream_test_util.cc
+++ b/net/spdy/spdy_stream_test_util.cc
@@ -61,8 +61,9 @@ int TestSpdyStreamDelegate::OnResponseReceived(const SpdyHeaderBlock& response,
   return status;
 }
 
-void TestSpdyStreamDelegate::OnDataReceived(const char* buffer, int bytes) {
+int TestSpdyStreamDelegate::OnDataReceived(const char* buffer, int bytes) {
   received_data_ += std::string(buffer, bytes);
+  return OK;
 }
 
 void TestSpdyStreamDelegate::OnDataSent(int length) {
diff --git a/net/spdy/spdy_stream_test_util.h b/net/spdy/spdy_stream_test_util.h
index 293bd7a..2bf2936 100644
--- a/net/spdy/spdy_stream_test_util.h
+++ b/net/spdy/spdy_stream_test_util.h
@@ -28,7 +28,7 @@ class TestSpdyStreamDelegate : public SpdyStream::Delegate {
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
-  virtual void OnDataReceived(const char* buffer, int bytes) OVERRIDE;
+  virtual int OnDataReceived(const char* buffer, int bytes) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;
 
diff --git a/net/spdy/spdy_websocket_stream.cc b/net/spdy/spdy_websocket_stream.cc
index 7ceb829..32248ad 100644
--- a/net/spdy/spdy_websocket_stream.cc
+++ b/net/spdy/spdy_websocket_stream.cc
@@ -107,9 +107,10 @@ int SpdyWebSocketStream::OnResponseReceived(
   return delegate_->OnReceivedSpdyResponseHeader(response, status);
 }
 
-void SpdyWebSocketStream::OnDataReceived(const char* data, int length) {
+int SpdyWebSocketStream::OnDataReceived(const char* data, int length) {
   DCHECK(delegate_);
   delegate_->OnReceivedSpdyData(data, length);
+  return OK;
 }
 
 void SpdyWebSocketStream::OnDataSent(int length) {
diff --git a/net/spdy/spdy_websocket_stream.h b/net/spdy/spdy_websocket_stream.h
index 900232b..58c62b02 100644
--- a/net/spdy/spdy_websocket_stream.h
+++ b/net/spdy/spdy_websocket_stream.h
@@ -77,7 +77,7 @@ class NET_EXPORT_PRIVATE SpdyWebSocketStream
   virtual int OnResponseReceived(const SpdyHeaderBlock& response,
                                  base::Time response_time,
                                  int status) OVERRIDE;
-  virtual void OnDataReceived(const char* data, int length) OVERRIDE;
+  virtual int OnDataReceived(const char* data, int length) OVERRIDE;
   virtual void OnDataSent(int length) OVERRIDE;
   virtual void OnClose(int status) OVERRIDE;