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