Fix use-after-free in RESET_STREAM_AT processing. PiperOrigin-RevId: 949147658
diff --git a/quiche/quic/core/http/quic_spdy_stream.cc b/quiche/quic/core/http/quic_spdy_stream.cc index ed57d71..f58b650 100644 --- a/quiche/quic/core/http/quic_spdy_stream.cc +++ b/quiche/quic/core/http/quic_spdy_stream.cc
@@ -1027,8 +1027,7 @@ } sequencer()->MarkConsumed(body_manager_.OnNonBody(header_length)); - - return true; + return !reading_stopped(); } bool QuicSpdyStream::OnDataFramePayload(absl::string_view payload) { @@ -1151,7 +1150,7 @@ id(), spdy_session_->qpack_decoder(), this, spdy_session_->max_inbound_header_list_size()); - return true; + return !reading_stopped(); } bool QuicSpdyStream::OnHeadersFramePayload(absl::string_view payload) { @@ -1172,7 +1171,7 @@ } sequencer()->MarkConsumed(body_manager_.OnNonBody(payload.size())); - return true; + return !reading_stopped(); } bool QuicSpdyStream::OnHeadersFrameEnd() { @@ -1274,7 +1273,7 @@ QUIC_DVLOG(1) << ENDPOINT << "Consuming " << header_length << " byte long frame header of METADATA."; sequencer()->MarkConsumed(body_manager_.OnNonBody(header_length)); - return true; + return !reading_stopped(); } bool QuicSpdyStream::OnMetadataFramePayload(absl::string_view payload) { @@ -1296,7 +1295,7 @@ QUIC_DVLOG(1) << ENDPOINT << "Consuming " << payload.size() << " bytes of payload of METADATA."; sequencer()->MarkConsumed(body_manager_.OnNonBody(payload.size())); - return true; + return !reading_stopped(); } bool QuicSpdyStream::OnMetadataFrameEnd() { @@ -1335,7 +1334,7 @@ << " byte long frame header of frame of unknown type " << frame_type << "."; sequencer()->MarkConsumed(body_manager_.OnNonBody(header_length)); - return true; + return !reading_stopped(); } bool QuicSpdyStream::OnUnknownFramePayload(absl::string_view payload) { @@ -1345,7 +1344,7 @@ QUIC_DVLOG(1) << ENDPOINT << "Consuming " << payload.size() << " bytes of payload of frame of unknown type."; sequencer()->MarkConsumed(body_manager_.OnNonBody(payload.size())); - return true; + return !reading_stopped(); } bool QuicSpdyStream::OnUnknownFrameEnd() { return true; }
diff --git a/quiche/quic/core/http/quic_spdy_stream_test.cc b/quiche/quic/core/http/quic_spdy_stream_test.cc index 58406c0..266824a 100644 --- a/quiche/quic/core/http/quic_spdy_stream_test.cc +++ b/quiche/quic/core/http/quic_spdy_stream_test.cc
@@ -18,22 +18,21 @@ #include "absl/strings/str_cat.h" #include "absl/strings/string_view.h" #include "quiche/quic/core/crypto/null_encrypter.h" +#include "quiche/quic/core/frames/quic_reset_stream_at_frame.h" #include "quiche/quic/core/frames/quic_stream_frame.h" #include "quiche/quic/core/http/http_constants.h" #include "quiche/quic/core/http/http_encoder.h" #include "quiche/quic/core/http/http_frames.h" #include "quiche/quic/core/http/quic_header_list.h" #include "quiche/quic/core/http/quic_spdy_session.h" -#include "quiche/quic/core/http/spdy_utils.h" #include "quiche/quic/core/http/web_transport_http3.h" #include "quiche/quic/core/qpack/value_splitting_header_list.h" #include "quiche/quic/core/quic_connection.h" +#include "quiche/quic/core/quic_constants.h" #include "quiche/quic/core/quic_stream_priority.h" -#include "quiche/quic/core/quic_stream_sequencer_buffer.h" #include "quiche/quic/core/quic_types.h" #include "quiche/quic/core/quic_utils.h" #include "quiche/quic/core/quic_versions.h" -#include "quiche/quic/core/quic_write_blocked_list.h" #include "quiche/quic/platform/api/quic_expect_bug.h" #include "quiche/quic/platform/api/quic_flags.h" #include "quiche/quic/platform/api/quic_ip_address.h" @@ -3895,6 +3894,52 @@ EXPECT_TRUE(stream_->read_side_closed()); } +// Regression test: RESET_STREAM_AT with reliable_offset equal to the H3 +// HEADERS frame-header length causes MaybeCloseStreamWithBufferedReset() to +// synchronously free the sequencer buffer from inside HttpDecoder::ProcessInput +// while the QuicDataReader still points into it. The decoder then continues to +// read the (now freed) payload region as QPACK bytes -> heap-use-after-free. +TEST_P(QuicSpdyStreamTest, ResetStreamAtDuringHeadersFrameDecode) { + if (!IsIetfQuic()) { + return; + } + Initialize(kShouldProcessData); + + // The QPACK decoder stream may want to write a Stream Cancellation + // instruction when the stream is reset; and the recreated accumulator's + // decoder may error out on the (freed / poisoned) bytes and try to + // reset the stream / close the connection. Absorb any such side-effects. + EXPECT_CALL(*session_, WritevData).Times(AnyNumber()); + EXPECT_CALL(*session_, MaybeSendStopSendingFrame).Times(AnyNumber()); + EXPECT_CALL(*session_, MaybeSendRstStreamFrame).Times(AnyNumber()); + EXPECT_CALL(*connection_, CloseConnection(_, _, _)).Times(AnyNumber()); + EXPECT_CALL(*connection_, SendControlFrame).Times(AnyNumber()); + + // 1. Server sends RESET_STREAM_AT{reliable_size=2, final_size=34}. It is + // buffered because NumBytesConsumed()==0 < 2. + QuicResetStreamAtFrame rst(kInvalidControlFrameId, stream_->id(), + /*error=*/0, /*final_offset=*/34, + /*reliable_offset=*/2); + stream_->OnResetStreamAtFrame(rst); + ASSERT_FALSE(stream_->rst_received()); + ASSERT_FALSE(stream_->read_side_closed()); + + // 2. Server sends STREAM data: an H3 HEADERS frame. + // type=0x01, length=0x20 (2-byte header) + 32-byte QPACK payload. + // Payload begins with a valid QPACK prefix (0x00 0x00) so the decoder + // actually dereferences the buffer. + std::string payload = std::string("\x00\x00", 2) + std::string(30, 'Q'); + ASSERT_EQ(32u, payload.size()); + std::string frame = HeadersFrame(payload); + ASSERT_EQ(34u, frame.size()); + ASSERT_EQ('\x01', frame[0]); // HEADERS type varint (1 byte) + ASSERT_EQ('\x20', frame[1]); // length varint (1 byte) + // The second byte causes the RESET_STREAM to execute. If the decoder keeps + // reading, it will cause a crash. + stream_->OnStreamFrame( + QuicStreamFrame(stream_->id(), /*fin=*/false, /*offset=*/0, frame)); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quiche/quic/core/http/web_transport_http3.cc b/quiche/quic/core/http/web_transport_http3.cc index c7157d2..f5613e9 100644 --- a/quiche/quic/core/http/web_transport_http3.cc +++ b/quiche/quic/core/http/web_transport_http3.cc
@@ -383,7 +383,7 @@ session_id_ = session_id; adapter_.SetSessionId(session_id); session_->AssociateIncomingWebTransportStreamWithSession(session_id, id()); - return true; + return !reading_stopped(); } void WebTransportHttp3UnidirectionalStream::OnDataAvailable() {
diff --git a/quiche/quic/core/quic_stream_sequencer.h b/quiche/quic/core/quic_stream_sequencer.h index 3451b43..53458b3 100644 --- a/quiche/quic/core/quic_stream_sequencer.h +++ b/quiche/quic/core/quic_stream_sequencer.h
@@ -102,7 +102,9 @@ size_t Readv(const struct iovec* iov, size_t iov_len); // Consumes |num_bytes| data. Used in conjunction with |GetReadableRegions| - // to do zero-copy reads. + // to do zero-copy reads. This can result in a close of the read side (due to + // a pending RESET_STREAM_AT), so callers should check !reading_stopped() or + // GetReadableRegion() before continuing to read. void MarkConsumed(size_t num_bytes); // Appends all of the readable data to |buffer| and marks all of the appended