Abort QPACK decompression when stream is reset.
Incorrect header acknowledgements are still detected in the wild. QPACK
decompression is currently aborted in QuicStream::OnClose() (gated by
quic_abort_qpack_on_stream_close flag). However, when a RESET_STREAM frame is
received closing the read side of the stream, it is possible that the write side
is still open and therefore OnClose() is not called immediately.
This CL destroys qpack_decoded_headers_accumulator_ in both places where a reset
stream instruction is sent on the decoder stream, making sure dynamic table
entries received later do not trigger sending a bogus header acknowledgement
instruction.
Protected by FLAGS_quic_reloadable_flag_quic_abort_qpack_on_stream_reset.
PiperOrigin-RevId: 343167912
Change-Id: Iad27fff7e3a78d62a4137312d4e79eff436eae98
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index 7059fc0..bdb5907 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -717,6 +717,10 @@
if (VersionUsesHttp3(transport_version()) && !fin_received() &&
spdy_session_->qpack_decoder()) {
spdy_session_->qpack_decoder()->OnStreamReset(id());
+ if (GetQuicReloadableFlag(quic_abort_qpack_on_stream_reset)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_abort_qpack_on_stream_reset, 1, 2);
+ qpack_decoded_headers_accumulator_.reset();
+ }
}
QuicStream::OnStreamReset(frame);
@@ -735,6 +739,10 @@
if (VersionUsesHttp3(transport_version()) && !fin_received() &&
spdy_session_->qpack_decoder()) {
spdy_session_->qpack_decoder()->OnStreamReset(id());
+ if (GetQuicReloadableFlag(quic_abort_qpack_on_stream_reset)) {
+ QUIC_RELOADABLE_FLAG_COUNT_N(quic_abort_qpack_on_stream_reset, 2, 2);
+ qpack_decoded_headers_accumulator_.reset();
+ }
}
QuicStream::Reset(error);
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 526b987..74bc30c 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -2474,7 +2474,7 @@
WritevData(decoder_send_stream->id(), /* write_length = */ 1,
/* offset = */ 1, _, _, _));
- // Reset stream.
+ // Reset stream by this endpoint, for example, due to stream cancellation.
if (!session_->split_up_send_rst()) {
EXPECT_CALL(*session_,
SendRstStream(stream_->id(), QUIC_STREAM_CANCELLED, _, _));
@@ -2486,7 +2486,8 @@
}
stream_->Reset(QUIC_STREAM_CANCELLED);
- if (!GetQuicReloadableFlag(quic_abort_qpack_on_stream_close)) {
+ if (!GetQuicReloadableFlag(quic_abort_qpack_on_stream_close) &&
+ !GetQuicReloadableFlag(quic_abort_qpack_on_stream_reset)) {
// Header acknowledgement.
EXPECT_CALL(*session_,
WritevData(decoder_send_stream->id(), /* write_length = */ 1,
@@ -2497,7 +2498,63 @@
// Deliver dynamic table entry to decoder.
session_->qpack_decoder()->OnInsertWithoutNameReference("foo", "bar");
- if (GetQuicReloadableFlag(quic_abort_qpack_on_stream_close)) {
+ if (GetQuicReloadableFlag(quic_abort_qpack_on_stream_close) ||
+ GetQuicReloadableFlag(quic_abort_qpack_on_stream_reset)) {
+ EXPECT_FALSE(stream_->headers_decompressed());
+ } else {
+ // Verify headers.
+ EXPECT_TRUE(stream_->headers_decompressed());
+ EXPECT_THAT(stream_->header_list(), ElementsAre(Pair("foo", "bar")));
+ }
+}
+
+TEST_P(QuicSpdyStreamTest, HeaderDecodingUnblockedAfterResetReceived) {
+ if (!UsesHttp3()) {
+ return;
+ }
+
+ Initialize(kShouldProcessData);
+ testing::InSequence s;
+ session_->qpack_decoder()->OnSetDynamicTableCapacity(1024);
+ StrictMock<MockHttp3DebugVisitor> debug_visitor;
+ session_->set_debug_visitor(&debug_visitor);
+
+ // HEADERS frame referencing first dynamic table entry.
+ std::string encoded_headers = absl::HexStringToBytes("020080");
+ std::string headers = HeadersFrame(encoded_headers);
+ EXPECT_CALL(debug_visitor,
+ OnHeadersFrameReceived(stream_->id(), encoded_headers.length()));
+ stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, 0, headers));
+
+ // Decoding is blocked because dynamic table entry has not been received yet.
+ EXPECT_FALSE(stream_->headers_decompressed());
+
+ // Decoder stream type and stream cancellation instruction.
+ auto decoder_send_stream =
+ QuicSpdySessionPeer::GetQpackDecoderSendStream(session_.get());
+ EXPECT_CALL(*session_,
+ WritevData(decoder_send_stream->id(), /* write_length = */ 1,
+ /* offset = */ 0, _, _, _));
+ EXPECT_CALL(*session_,
+ WritevData(decoder_send_stream->id(), /* write_length = */ 1,
+ /* offset = */ 1, _, _, _));
+
+ // OnStreamReset() is called when RESET_STREAM frame is received from peer.
+ stream_->OnStreamReset(QuicRstStreamFrame(
+ kInvalidControlFrameId, stream_->id(), QUIC_STREAM_CANCELLED, 0));
+
+ if (!GetQuicReloadableFlag(quic_abort_qpack_on_stream_reset)) {
+ // Header acknowledgement.
+ EXPECT_CALL(*session_,
+ WritevData(decoder_send_stream->id(), /* write_length = */ 1,
+ /* offset = */ 2, _, _, _));
+ EXPECT_CALL(debug_visitor, OnHeadersDecoded(stream_->id(), _));
+ }
+
+ // Deliver dynamic table entry to decoder.
+ session_->qpack_decoder()->OnInsertWithoutNameReference("foo", "bar");
+
+ if (GetQuicReloadableFlag(quic_abort_qpack_on_stream_reset)) {
EXPECT_FALSE(stream_->headers_decompressed());
} else {
// Verify headers.
diff --git a/quic/core/quic_flags_list.h b/quic/core/quic_flags_list.h
index 6ccb354..f9d12df 100644
--- a/quic/core/quic_flags_list.h
+++ b/quic/core/quic_flags_list.h
@@ -6,6 +6,7 @@
QUIC_FLAG(FLAGS_quic_reloadable_flag_http2_use_fast_huffman_encoder, true)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_abort_qpack_on_stream_close, true)
+QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_abort_qpack_on_stream_reset, true)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_ack_delay_alarm_granularity, false)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_add_stream_info_to_idle_close_detail, true)
QUIC_FLAG(FLAGS_quic_reloadable_flag_quic_allocate_stream_sequencer_buffer_blocks_on_demand, false)
