commit 0a8ff62c342ba5d8351882d79a90a94e35a44486
author ianswett <ianswett@google.com> Wed Feb 05 23:28:58 2020 -0800
committer Copybara-Service <copybara-worker@google.com> Wed Feb 05 23:29:36 2020 -0800
tree 01b514465c30a24e5cca806ea932f0c386b54ff7
parent 161bb4953763a74573394356374e0646d14f2cf6

gfe-relnote: No longer send a RESET_STREAM in response to a STOP_SENDING if the stream is write closed.  Protected by disabled gfe2_reloadable_flag_quic_enable_version_99.

PiperOrigin-RevId: 293529515
Change-Id: I0a837b79a776233d7a21fb8f87dbb7e173241a26

quic/core/quic_session.cc

diff --git a/quic/core/quic_session.cc b/quic/core/quic_session.cc
index b8de367..b63f729 100644
--- a/quic/core/quic_session.cc
+++ b/quic/core/quic_session.cc
@@ -221,9 +221,13 @@
 void QuicSession::OnStopSendingFrame(const QuicStopSendingFrame& frame) {
   // STOP_SENDING is in IETF QUIC only.
   DCHECK(VersionHasIetfQuicFrames(transport_version()));
+  DCHECK(QuicVersionUsesCryptoFrames(transport_version()));
 
   QuicStreamId stream_id = frame.stream_id;
   // If Stream ID is invalid then close the connection.
+  // TODO(ianswett): This check is redundant to checks for IsClosedStream,
+  // but removing it requires removing multiple DCHECKs.
+  // TODO(ianswett): Multiple QUIC_DVLOGs could be QUIC_PEER_BUGs.
   if (stream_id == QuicUtils::GetInvalidStreamId(transport_version())) {
     QUIC_DVLOG(1) << ENDPOINT
                   << "Received STOP_SENDING with invalid stream_id: "
@@ -234,6 +238,19 @@
     return;
   }
 
+  // If stream_id is READ_UNIDIRECTIONAL, close the connection.
+  if (QuicUtils::GetStreamType(stream_id, perspective(),
+                               IsIncomingStream(stream_id)) ==
+      READ_UNIDIRECTIONAL) {
+    QUIC_DVLOG(1) << ENDPOINT
+                  << "Received STOP_SENDING for a read-only stream_id: "
+                  << stream_id << ".";
+    connection()->CloseConnection(
+        QUIC_INVALID_STREAM_ID, "Received STOP_SENDING for a read-only stream",
+        ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+    return;
+  }
+
   if (visitor_) {
     visitor_->OnStopSendingReceived(frame);
   }
@@ -246,7 +263,9 @@
         << stream_id << " Ignoring.";
     return;
   }
-  // If stream is non-existent, close the connection
+  // If stream is non-existent, close the connection.
+  // TODO(b/148842616): IETF QUIC allows a STOP_SENDING to arrive before a
+  // STREAM frame for peer-intiated bidirectional steams
   StreamMap::iterator it = stream_map_.find(stream_id);
   if (it == stream_map_.end()) {
     QUIC_DVLOG(1) << ENDPOINT
@@ -260,10 +279,22 @@
   }
 
   QuicStream* stream = it->second.get();
+  // If the stream is null, it's an implementation error.
   if (stream == nullptr) {
     QUIC_BUG << ENDPOINT
              << "Received STOP_SENDING for NULL QuicStream, stream_id: "
              << stream_id << ". Ignoring.";
+    connection()->CloseConnection(
+        QUIC_INTERNAL_ERROR, "Received STOP_SENDING for a null stream",
+        ConnectionCloseBehavior::SEND_CONNECTION_CLOSE_PACKET);
+    return;
+  }
+
+  // Do not reset the sream if all data has been sent and acknowledged.
+  if (stream->write_side_closed() && !stream->IsWaitingForAcks()) {
+    QUIC_DVLOG(1) << ENDPOINT
+                  << "Ignoring STOP_SENDING for a write closed stream, id: "
+                  << stream_id;
     return;
   }