Do not crash if `qpack_decoded_headers_accumulator_` is nullptr. 1. Do not crash if QuicSpdyStream::OnHeadersFramePayload() or OnHeadersFrameEnd() is called when `qpack_decoded_headers_accumulator_` is nullptr. 2. Do not crash if QpackDecodedHeadersAccumulator::EndHeaderBlock() is called when `decoder_` is nullptr. 3. Add GFE_BUGs and log where `qpack_decoded_headers_accumulator_` was last reset. PiperOrigin-RevId: 423164997
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index afeeef0..5317137 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -185,6 +185,8 @@ headers_payload_length_(0), trailers_decompressed_(false), trailers_consumed_(false), + qpack_decoded_headers_accumulator_reset_reason_( + QpackDecodedHeadersAccumulatorResetReason::kUnSet), http_decoder_visitor_(std::make_unique<HttpDecoderVisitor>(this)), decoder_(http_decoder_visitor_.get(), HttpDecoderOptionsForBidiStream(spdy_session)), @@ -225,6 +227,8 @@ headers_payload_length_(0), trailers_decompressed_(false), trailers_consumed_(false), + qpack_decoded_headers_accumulator_reset_reason_( + QpackDecodedHeadersAccumulatorResetReason::kUnSet), http_decoder_visitor_(std::make_unique<HttpDecoderVisitor>(this)), decoder_(http_decoder_visitor_.get()), sequencer_offset_(sequencer()->NumBytesConsumed()), @@ -563,6 +567,8 @@ bool header_list_size_limit_exceeded) { header_list_size_limit_exceeded_ = header_list_size_limit_exceeded; qpack_decoded_headers_accumulator_.reset(); + qpack_decoded_headers_accumulator_reset_reason_ = + QpackDecodedHeadersAccumulatorResetReason::kResetInOnHeadersDecoded; QuicSpdySession::LogHeaderCompressionRatioHistogram( /* using_qpack = */ true, @@ -592,6 +598,8 @@ void QuicSpdyStream::OnHeaderDecodingError(QuicErrorCode error_code, absl::string_view error_message) { qpack_decoded_headers_accumulator_.reset(); + qpack_decoded_headers_accumulator_reset_reason_ = + QpackDecodedHeadersAccumulatorResetReason::kResetInOnHeaderDecodingError; std::string connection_close_error_message = absl::StrCat( "Error decoding ", headers_decompressed_ ? "trailers" : "headers", @@ -756,6 +764,8 @@ 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(); + qpack_decoded_headers_accumulator_reset_reason_ = + QpackDecodedHeadersAccumulatorResetReason::kResetInOnStreamReset1; } } @@ -769,6 +779,8 @@ if (!fin_received() && spdy_session_->qpack_decoder()) { spdy_session_->qpack_decoder()->OnStreamReset(id()); qpack_decoded_headers_accumulator_.reset(); + qpack_decoded_headers_accumulator_reset_reason_ = + QpackDecodedHeadersAccumulatorResetReason::kResetInOnStreamReset2; } QuicStream::OnStreamReset(frame); @@ -791,6 +803,8 @@ 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(); + qpack_decoded_headers_accumulator_reset_reason_ = + QpackDecodedHeadersAccumulatorResetReason::kResetInResetWithError; } } @@ -893,6 +907,8 @@ QuicStream::OnClose(); qpack_decoded_headers_accumulator_.reset(); + qpack_decoded_headers_accumulator_reset_reason_ = + QpackDecodedHeadersAccumulatorResetReason::kResetInOnClose; if (visitor_) { Visitor* visitor = visitor_; @@ -1088,7 +1104,14 @@ bool QuicSpdyStream::OnHeadersFramePayload(absl::string_view payload) { QUICHE_DCHECK(VersionUsesHttp3(transport_version())); - QUICHE_DCHECK(qpack_decoded_headers_accumulator_); + + if (!qpack_decoded_headers_accumulator_) { + QUIC_BUG(b215142466_OnHeadersFramePayload) + << static_cast<int>(qpack_decoded_headers_accumulator_reset_reason_); + OnHeaderDecodingError(QUIC_INTERNAL_ERROR, + "qpack_decoded_headers_accumulator_ is nullptr"); + return false; + } qpack_decoded_headers_accumulator_->Decode(payload); @@ -1103,7 +1126,14 @@ bool QuicSpdyStream::OnHeadersFrameEnd() { QUICHE_DCHECK(VersionUsesHttp3(transport_version())); - QUICHE_DCHECK(qpack_decoded_headers_accumulator_); + + if (!qpack_decoded_headers_accumulator_) { + QUIC_BUG(b215142466_OnHeadersFrameEnd) + << static_cast<int>(qpack_decoded_headers_accumulator_reset_reason_); + OnHeaderDecodingError(QUIC_INTERNAL_ERROR, + "qpack_decoded_headers_accumulator_ is nullptr"); + return false; + } qpack_decoded_headers_accumulator_->EndHeaderBlock();
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h index 8ec56d5..14ac2f2 100644 --- a/quic/core/http/quic_spdy_stream.h +++ b/quic/core/http/quic_spdy_stream.h
@@ -391,6 +391,20 @@ WebTransportStreamAdapter adapter; }; + // Reason codes for `qpack_decoded_headers_accumulator_` being nullptr. + enum class QpackDecodedHeadersAccumulatorResetReason { + // `qpack_decoded_headers_accumulator_` was default constructed to nullptr. + kUnSet = 0, + // `qpack_decoded_headers_accumulator_` was reset in the corresponding + // method. + kResetInOnHeadersDecoded = 1, + kResetInOnHeaderDecodingError = 2, + kResetInOnStreamReset1 = 3, + kResetInOnStreamReset2 = 4, + kResetInResetWithError = 5, + kResetInOnClose = 6, + }; + // Called by HttpDecoderVisitor. bool OnDataFrameStart(QuicByteCount header_length, QuicByteCount payload_length); @@ -462,6 +476,9 @@ // Headers accumulator for decoding HEADERS frame payload. std::unique_ptr<QpackDecodedHeadersAccumulator> qpack_decoded_headers_accumulator_; + // Reason for `qpack_decoded_headers_accumulator_` being nullptr. + QpackDecodedHeadersAccumulatorResetReason + qpack_decoded_headers_accumulator_reset_reason_; // Visitor of the HttpDecoder. std::unique_ptr<HttpDecoderVisitor> http_decoder_visitor_; // HttpDecoder for processing raw incoming stream frames.
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index 3bad6ef..6ac7fe0 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -3424,6 +3424,31 @@ EXPECT_EQ(size - 1, size_with_context); } +TEST_P(QuicSpdyStreamTest, HeadersAccumulatorNullptr) { + if (!UsesHttp3()) { + return; + } + + Initialize(kShouldProcessData); + + // Creates QpackDecodedHeadersAccumulator in + // `qpack_decoded_headers_accumulator_`. + std::string headers = HeadersFrame({std::make_pair("foo", "bar")}); + stream_->OnStreamFrame(QuicStreamFrame(stream_->id(), false, 0, headers)); + + // Resets `qpack_decoded_headers_accumulator_`. + stream_->OnHeadersDecoded({}, false); + + // This private method should never be called when + // `qpack_decoded_headers_accumulator_` is nullptr. The number 1 identifies + // the site where `qpack_decoded_headers_accumulator_` was last reset. + EXPECT_CALL(*connection_, CloseConnection(_, _, _)); + bool result = true; + EXPECT_QUIC_BUG(result = QuicSpdyStreamPeer::OnHeadersFrameEnd(stream_), + "b215142466_OnHeadersFrameEnd.: 1$"); + EXPECT_FALSE(result); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator.cc b/quic/core/qpack/qpack_decoded_headers_accumulator.cc index 4e50e72..934cc5f 100644 --- a/quic/core/qpack/qpack_decoded_headers_accumulator.cc +++ b/quic/core/qpack/qpack_decoded_headers_accumulator.cc
@@ -7,6 +7,7 @@ #include "absl/strings/string_view.h" #include "quic/core/qpack/qpack_decoder.h" #include "quic/core/qpack/qpack_header_table.h" +#include "quic/platform/api/quic_bug_tracker.h" namespace quic { @@ -88,6 +89,11 @@ QUICHE_DCHECK(!error_detected_); QUICHE_DCHECK(!headers_decoded_); + if (!decoder_) { + QUIC_BUG(b215142466_EndHeaderBlock); + return; + } + // Might destroy |this|. decoder_->EndHeaderBlock(); }
diff --git a/quic/test_tools/quic_spdy_stream_peer.cc b/quic/test_tools/quic_spdy_stream_peer.cc index 6db9a4a..ea21081 100644 --- a/quic/test_tools/quic_spdy_stream_peer.cc +++ b/quic/test_tools/quic_spdy_stream_peer.cc
@@ -28,5 +28,10 @@ return stream->use_datagram_contexts_; } +// static +bool QuicSpdyStreamPeer::OnHeadersFrameEnd(QuicSpdyStream* stream) { + return stream->OnHeadersFrameEnd(); +} + } // namespace test } // namespace quic
diff --git a/quic/test_tools/quic_spdy_stream_peer.h b/quic/test_tools/quic_spdy_stream_peer.h index 8a50426..692cbc3 100644 --- a/quic/test_tools/quic_spdy_stream_peer.h +++ b/quic/test_tools/quic_spdy_stream_peer.h
@@ -24,6 +24,7 @@ static const QuicIntervalSet<QuicStreamOffset>& unacked_frame_headers_offsets( QuicSpdyStream* stream); static bool use_datagram_contexts(QuicSpdyStream* stream); + static bool OnHeadersFrameEnd(QuicSpdyStream* stream); }; } // namespace test