Fix QPACK crash by unregistering observers with QpackHeaderTable. QpackHeaderTable is owned by QpackDecoder, owned by QuicSpdySession. Each observer is a QpackProgressiveDecoder, owned by QpackHeaderAccumulator, owned by QuicSpdyStream. If a stream is deleted (local cancellation, reset received from peer), then the observer must be deregistered with QpackHeaderTable in order to avoid a crash. gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 272562981 Change-Id: Ia49f2ef9aeeebcccb83405107f0224ee7d23694f
diff --git a/quic/core/http/quic_spdy_session_test.cc b/quic/core/http/quic_spdy_session_test.cc index 1d3092c..3e0a6f8 100644 --- a/quic/core/http/quic_spdy_session_test.cc +++ b/quic/core/http/quic_spdy_session_test.cc
@@ -2255,6 +2255,41 @@ EXPECT_EQ(5u, session_.max_outbound_header_list_size()); } +// Regression test for https://crbug.com/1009551. +TEST_P(QuicSpdySessionTestServer, StreamClosedWhileHeaderDecodingBlocked) { + if (!VersionUsesQpack(transport_version())) { + return; + } + + session_.qpack_decoder()->OnSetDynamicTableCapacity(1024); + + QuicStreamId stream_id = GetNthClientInitiatedBidirectionalId(0); + TestStream* stream = session_.CreateIncomingStream(stream_id); + + // HEADERS frame referencing first dynamic table entry. + std::string headers_payload = QuicTextUtils::HexDecode("020080"); + std::unique_ptr<char[]> headers_buffer; + HttpEncoder encoder; + QuicByteCount headers_frame_header_length = + encoder.SerializeHeadersFrameHeader(headers_payload.length(), + &headers_buffer); + QuicStringPiece headers_frame_header(headers_buffer.get(), + headers_frame_header_length); + std::string headers = QuicStrCat(headers_frame_header, headers_payload); + 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()); + + // Stream is closed and destroyed. + CloseStream(stream_id); + session_.CleanUpClosedStreams(); + + // Dynamic table entry arrived on the decoder stream. + // The destroyed stream object must not be referenced. + session_.qpack_decoder()->OnInsertWithoutNameReference("foo", "bar"); +} + TEST_P(QuicSpdySessionTestClient, ResetAfterInvalidIncomingStreamType) { if (!VersionHasStreamType(transport_version())) { return;
diff --git a/quic/core/qpack/qpack_header_table.cc b/quic/core/qpack/qpack_header_table.cc index 30f15a5..7274ce4 100644 --- a/quic/core/qpack/qpack_header_table.cc +++ b/quic/core/qpack/qpack_header_table.cc
@@ -197,6 +197,21 @@ observers_.insert({required_insert_count, observer}); } +void QpackHeaderTable::UnregisterObserver(uint64_t required_insert_count, + Observer* observer) { + auto it = observers_.lower_bound(required_insert_count); + while (it != observers_.end() && it->first == required_insert_count) { + if (it->second == observer) { + observers_.erase(it); + return; + } + ++it; + } + + // |observer| must have been registered. + QUIC_NOTREACHED(); +} + uint64_t QpackHeaderTable::draining_index(float draining_fraction) const { DCHECK_LE(0.0, draining_fraction); DCHECK_LE(draining_fraction, 1.0);
diff --git a/quic/core/qpack/qpack_header_table.h b/quic/core/qpack/qpack_header_table.h index 919a4eb..34dfd1c 100644 --- a/quic/core/qpack/qpack_header_table.h +++ b/quic/core/qpack/qpack_header_table.h
@@ -99,6 +99,11 @@ // gets unregistered. Each observer must only be registered at most once. void RegisterObserver(uint64_t required_insert_count, Observer* observer); + // Unregister previously registered observer. Must be called before an + // observer still waiting for notification is destroyed. Must be called with + // the same |required_insert_count| value that |observer| was registered with. + void UnregisterObserver(uint64_t required_insert_count, Observer* observer); + // Used on request streams to encode and decode Required Insert Count. uint64_t max_entries() const { return max_entries_; }
diff --git a/quic/core/qpack/qpack_header_table_test.cc b/quic/core/qpack/qpack_header_table_test.cc index 5a97c20..907c2db 100644 --- a/quic/core/qpack/qpack_header_table_test.cc +++ b/quic/core/qpack/qpack_header_table_test.cc
@@ -94,6 +94,11 @@ table_.RegisterObserver(required_insert_count, observer); } + void UnregisterObserver(uint64_t required_insert_count, + QpackHeaderTable::Observer* observer) { + table_.UnregisterObserver(required_insert_count, observer); + } + uint64_t max_entries() const { return table_.max_entries(); } uint64_t inserted_entry_count() const { return table_.inserted_entry_count(); @@ -416,7 +421,7 @@ table.MaxInsertSizeWithoutEvictingGivenEntry(1)); } -TEST_F(QpackHeaderTableTest, Observer) { +TEST_F(QpackHeaderTableTest, RegisterObserver) { StrictMock<MockObserver> observer1; RegisterObserver(1, &observer1); EXPECT_CALL(observer1, OnInsertCountReachedThreshold); @@ -455,6 +460,27 @@ Mock::VerifyAndClearExpectations(&observer5); } +TEST_F(QpackHeaderTableTest, UnregisterObserver) { + StrictMock<MockObserver> observer1; + StrictMock<MockObserver> observer2; + StrictMock<MockObserver> observer3; + StrictMock<MockObserver> observer4; + RegisterObserver(1, &observer1); + RegisterObserver(2, &observer2); + RegisterObserver(2, &observer3); + RegisterObserver(3, &observer4); + + UnregisterObserver(2, &observer3); + + EXPECT_CALL(observer1, OnInsertCountReachedThreshold); + EXPECT_CALL(observer2, OnInsertCountReachedThreshold); + EXPECT_CALL(observer4, OnInsertCountReachedThreshold); + InsertEntry("foo", "bar"); + InsertEntry("foo", "bar"); + InsertEntry("foo", "bar"); + EXPECT_EQ(3u, inserted_entry_count()); +} + TEST_F(QpackHeaderTableTest, DrainingIndex) { QpackHeaderTable table; table.SetMaximumDynamicTableCapacity(kMaximumDynamicTableCapacityForTesting);
diff --git a/quic/core/qpack/qpack_progressive_decoder.cc b/quic/core/qpack/qpack_progressive_decoder.cc index 5632286..7037f15 100644 --- a/quic/core/qpack/qpack_progressive_decoder.cc +++ b/quic/core/qpack/qpack_progressive_decoder.cc
@@ -38,6 +38,12 @@ decoding_(true), error_detected_(false) {} +QpackProgressiveDecoder::~QpackProgressiveDecoder() { + if (blocked_) { + header_table_->UnregisterObserver(required_insert_count_, this); + } +} + void QpackProgressiveDecoder::Decode(QuicStringPiece data) { DCHECK(decoding_); @@ -290,11 +296,11 @@ prefix_decoded_ = true; if (required_insert_count_ > header_table_->inserted_entry_count()) { - blocked_ = true; if (!enforcer_->OnStreamBlocked(stream_id_)) { OnError("Limit on number of blocked streams exceeded."); return false; } + blocked_ = true; header_table_->RegisterObserver(required_insert_count_, this); }
diff --git a/quic/core/qpack/qpack_progressive_decoder.h b/quic/core/qpack/qpack_progressive_decoder.h index 5c116b1..dbb4093 100644 --- a/quic/core/qpack/qpack_progressive_decoder.h +++ b/quic/core/qpack/qpack_progressive_decoder.h
@@ -74,7 +74,7 @@ HeadersHandlerInterface* handler); QpackProgressiveDecoder(const QpackProgressiveDecoder&) = delete; QpackProgressiveDecoder& operator=(const QpackProgressiveDecoder&) = delete; - ~QpackProgressiveDecoder() override = default; + ~QpackProgressiveDecoder() override; // Provide a data fragment to decode. void Decode(QuicStringPiece data);