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);
