commit f9ff46b6104850f0c26e04a974a1d9a4d01297f7
author bnc <bnc@google.com> Tue Nov 26 10:32:09 2019 -0800
committer Copybara-Service <copybara-worker@google.com> Tue Nov 26 10:32:50 2019 -0800
tree 658bad146fd5566bd1547d05ce6d1d80a59dede7
parent ef8416ae62ee1fe0d0a00d812bc377613df23009

Do not call MarkConsumed() from OnHeadersFramePayload() after decoding error.

gfe-relnote: n/a, change to QUIC v99-only code.  Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 282593548
Change-Id: I7d43611cb21d2059b5d4a0ec0d09078a3c084169

quic/core/http/quic_spdy_stream.cc

diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index fe990fc..b0c4343 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -919,10 +919,14 @@
   }
 
   qpack_decoded_headers_accumulator_->Decode(payload);
-  sequencer()->MarkConsumed(body_manager_.OnNonBody(payload.size()));
 
   // |qpack_decoded_headers_accumulator_| is reset if an error is detected.
-  return qpack_decoded_headers_accumulator_ != nullptr;
+  if (!qpack_decoded_headers_accumulator_) {
+    return false;
+  }
+
+  sequencer()->MarkConsumed(body_manager_.OnNonBody(payload.size()));
+  return true;
 }
 
 bool QuicSpdyStream::OnHeadersFrameEnd() {

quic/core/http/quic_spdy_stream_test.cc

diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 61e6f37..d7c6b93 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -1862,6 +1862,48 @@
   stream_->OnStreamFrame(frame);
 }
 
+// Regression test for https://crbug.com/1027895: a HEADERS frame triggers an
+// error in QuicSpdyStream::OnHeadersFramePayload().  This closes the
+// connection, freeing the buffer of QuicStreamSequencer.  Therefore
+// QuicStreamSequencer::MarkConsumed() must not be called from
+// QuicSpdyStream::OnHeadersFramePayload().
+TEST_P(QuicSpdyStreamTest, DoNotMarkConsumedAfterQpackDecodingError) {
+  if (!UsesHttp3()) {
+    return;
+  }
+
+  Initialize(kShouldProcessData);
+  connection_->AdvanceTime(QuicTime::Delta::FromSeconds(1));
+
+  testing::InSequence s;
+  EXPECT_CALL(
+      *connection_,
+      CloseConnection(QUIC_QPACK_DECOMPRESSION_FAILED,
+                      MatchesRegex("Error decoding headers on stream \\d+: "
+                                   "Invalid relative index."),
+                      _))
+      .WillOnce(
+          (Invoke([this](QuicErrorCode error, const std::string& error_details,
+                         ConnectionCloseBehavior connection_close_behavior) {
+            connection_->ReallyCloseConnection(error, error_details,
+                                               connection_close_behavior);
+          })));
+  EXPECT_CALL(*connection_, SendConnectionClosePacket(_, _));
+  EXPECT_CALL(*session_, OnConnectionClosed(_, _))
+      .WillOnce(Invoke([this](const QuicConnectionCloseFrame& frame,
+                              ConnectionCloseSource source) {
+        session_->ReallyOnConnectionClosed(frame, source);
+      }));
+  EXPECT_CALL(*session_, SendRstStream(stream_->id(), _, _));
+  EXPECT_CALL(*session_, SendRstStream(stream2_->id(), _, _));
+
+  // Invalid headers: Required Insert Count is zero, but the header block
+  // contains a dynamic table reference.
+  std::string headers = HeadersFrame(QuicTextUtils::HexDecode("000080"));
+  QuicStreamFrame frame(stream_->id(), false, 0, headers);
+  stream_->OnStreamFrame(frame);
+}
+
 TEST_P(QuicSpdyStreamTest, ImmediateHeaderDecodingWithDynamicTableEntries) {
   if (!UsesHttp3()) {
     return;