Go away on odd push and already seen push stream ids

BUG=377538
R=jgraettinger@chromium.org

TEST=SpdyNetworkTransactionTest.GoAwayOnPushStreamIdLesserOrEqualThanLastAccepted
     SpdyNetworkTransactionTest.GoAwayOnOddPushStreamId

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

Cr-Commit-Position: refs/heads/master@{#293002}

net/spdy/spdy_network_transaction_unittest.cc

@@ -6452,6 +6452,67 @@ TEST_P(SpdyNetworkTransactionTest, FlowControlNegativeSendWindowSize) {
   helper.VerifyDataConsumed();
 }
 
+TEST_P(SpdyNetworkTransactionTest, GoAwayOnOddPushStreamId) {
+  if (spdy_util_.spdy_version() < SPDY3)
+    return;
+
+  scoped_ptr<SpdyHeaderBlock> push_headers(new SpdyHeaderBlock);
+  spdy_util_.AddUrlToHeaderBlock("http://www.google.com/a.dat",
+                                 push_headers.get());
+  scoped_ptr<SpdyFrame> push(
+      spdy_util_.ConstructInitialSpdyPushFrame(push_headers.Pass(), 3, 1));
+  MockRead reads[] = {CreateMockRead(*push, 1)};
+
+  scoped_ptr<SpdyFrame> req(
+      spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
+  scoped_ptr<SpdyFrame> goaway(spdy_util_.ConstructSpdyGoAway(
+      0, GOAWAY_PROTOCOL_ERROR, "Odd push stream id."));
+  MockWrite writes[] = {
+      CreateMockWrite(*req, 0), CreateMockWrite(*goaway, 2),
+  };
+
+  DelayedSocketData data(1, reads, arraysize(reads), writes, arraysize(writes));
+  NormalSpdyTransactionHelper helper(
+      CreateGetRequest(), DEFAULT_PRIORITY, BoundNetLog(), GetParam(), NULL);
+  helper.RunToCompletion(&data);
+  TransactionHelperResult out = helper.output();
+  EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
+}
+
+TEST_P(SpdyNetworkTransactionTest,
+       GoAwayOnPushStreamIdLesserOrEqualThanLastAccepted) {
+  if (spdy_util_.spdy_version() < SPDY3)
+    return;
+
+  scoped_ptr<SpdyFrame> push_a(spdy_util_.ConstructSpdyPush(
+      NULL, 0, 4, 1, "http://www.google.com/a.dat"));
+  scoped_ptr<SpdyHeaderBlock> push_b_headers(new SpdyHeaderBlock);
+  spdy_util_.AddUrlToHeaderBlock("http://www.google.com/b.dat",
+                                 push_b_headers.get());
+  scoped_ptr<SpdyFrame> push_b(
+      spdy_util_.ConstructInitialSpdyPushFrame(push_b_headers.Pass(), 2, 1));
+  MockRead reads[] = {
+      CreateMockRead(*push_a, 1), CreateMockRead(*push_b, 2),
+  };
+
+  scoped_ptr<SpdyFrame> req(
+      spdy_util_.ConstructSpdyGet(NULL, 0, false, 1, LOWEST, true));
+  scoped_ptr<SpdyFrame> goaway(spdy_util_.ConstructSpdyGoAway(
+      4,
+      GOAWAY_PROTOCOL_ERROR,
+      "New push stream id must be greater than the last accepted."));
+  MockWrite writes[] = {
+      CreateMockWrite(*req, 0), CreateMockWrite(*goaway, 3),
+  };
+
+  DelayedSocketData data(1, reads, arraysize(reads), writes, arraysize(writes));
+  NormalSpdyTransactionHelper helper(
+      CreateGetRequest(), DEFAULT_PRIORITY, BoundNetLog(), GetParam(), NULL);
+  helper.RunToCompletion(&data);
+  TransactionHelperResult out = helper.output();
+  EXPECT_EQ(ERR_SPDY_PROTOCOL_ERROR, out.rv);
+}
+
 class SpdyNetworkTransactionNoTLSUsageCheckTest
     : public SpdyNetworkTransactionTest {
  protected:

net/spdy/spdy_session.cc

@@ -590,6 +590,7 @@ SpdySession::SpdySession(
       transport_security_state_(transport_security_state),
       read_buffer_(new IOBuffer(kReadBufferSize)),
       stream_hi_water_mark_(kFirstStreamId),
+      last_accepted_push_stream_id_(0),
       num_pushed_streams_(0u),
       num_active_pushed_streams_(0u),
       in_flight_write_frame_type_(DATA),
@@ -1677,7 +1678,7 @@ void SpdySession::DoDrainSession(Error err, const std::string& description) {
       err != ERR_SOCKET_NOT_CONNECTED &&
       err != ERR_CONNECTION_CLOSED && err != ERR_CONNECTION_RESET) {
     // Enqueue a GOAWAY to inform the peer of why we're closing the connection.
-    SpdyGoAwayIR goaway_ir(0,  // Last accepted stream ID.
+    SpdyGoAwayIR goaway_ir(last_accepted_push_stream_id_,
                            MapNetErrorToGoAwayStatus(err),
                            description);
     EnqueueSessionWrite(HIGHEST,
@@ -2360,7 +2361,10 @@ bool SpdySession::OnUnknownFrame(SpdyStreamId stream_id, int frame_type) {
   // Was the frame sent on a stream id that has not been used in this session?
   if (stream_id % 2 == 1 && stream_id > stream_hi_water_mark_)
     return false;
-  // TODO(bnc):  Track highest id for server initiated streams.
+
+  if (stream_id % 2 == 0 && stream_id > last_accepted_push_stream_id_)
+    return false;
+
   return true;
 }
 
@@ -2521,14 +2525,32 @@ bool SpdySession::TryCreatePushStream(SpdyStreamId stream_id,
   // Server-initiated streams should have even sequence numbers.
   if ((stream_id & 0x1) != 0) {
     LOG(WARNING) << "Received invalid push stream id " << stream_id;
+    if (GetProtocolVersion() > SPDY2)
+      CloseSessionOnError(ERR_SPDY_PROTOCOL_ERROR, "Odd push stream id.");
     return false;
   }
 
+  if (GetProtocolVersion() > SPDY2) {
+    if (stream_id <= last_accepted_push_stream_id_) {
+      LOG(WARNING) << "Received push stream id lesser or equal to the last "
+                   << "accepted before " << stream_id;
+      CloseSessionOnError(
+          ERR_SPDY_PROTOCOL_ERROR,
+          "New push stream id must be greater than the last accepted.");
+      return false;
+    }
+  }
+
   if (IsStreamActive(stream_id)) {
+    // For SPDY3 and higher we should not get here, we'll start going away
+    // earlier on |last_seen_push_stream_id_| check.
+    CHECK_GT(SPDY3, GetProtocolVersion());
     LOG(WARNING) << "Received push for active stream " << stream_id;
     return false;
   }
 
+  last_accepted_push_stream_id_ = stream_id;
+
   RequestPriority request_priority =
       ConvertSpdyPriorityToRequestPriority(priority, GetProtocolVersion());
 

net/spdy/spdy_session.h

@@ -984,6 +984,9 @@ class NET_EXPORT SpdySession : public BufferedSpdyFramerVisitorInterface,
 
   SpdyStreamId stream_hi_water_mark_;  // The next stream id to use.
 
+  // Used to ensure the server increments push stream ids correctly.
+  SpdyStreamId last_accepted_push_stream_id_;
+
   // Queue, for each priority, of pending stream requests that have
   // not yet been satisfied.
   PendingStreamRequestQueue pending_create_stream_queues_[NUM_PRIORITIES];

net/spdy/spdy_session_unittest.cc

@@ -4977,9 +4977,12 @@ TEST_P(SpdySessionTest, RejectInvalidUnknownFrames) {
   EXPECT_TRUE(session->OnUnknownFrame(3, 0));
   // Client id exceeding watermark.
   EXPECT_FALSE(session->OnUnknownFrame(9, 0));
-  // Currently we do not keep track of server initiated (even) ids.
+
+  session->last_accepted_push_stream_id_ = 6;
+  // Low server (even) ids are fine.
   EXPECT_TRUE(session->OnUnknownFrame(2, 0));
-  EXPECT_TRUE(session->OnUnknownFrame(8, 0));
+  // Server id exceeding last accepted id.
+  EXPECT_FALSE(session->OnUnknownFrame(8, 0));
 }
 
 TEST(MapFramerErrorToProtocolError, MapsValues) {