gfe-relnote: Do not include PUSH_PROMISE header block length when calling QuicSpdyStream::OnStreamHeaderList(). Not protected. Before this CL, PUSH_PROMISE header block size is incorrectly added to headers_payload_length_ or trailers_payload_length_, which is then passed to QuicSpdyStream::OnStreamHeaderList(). This CL instead saves the payload_length argument of OnHeadersFrameStart(). The |frame_len| argument of OnStreamHeaderList() is only used in a VLOG statement in QuicEventManagerStream::OnInitialHeadersComplete() and for incrementing |header_bytes_read_| in QuicSpdyClientStream::OnInitialHeadersComplete(). QuicSpdyClientStream::header_bytes_read() is only called from QuicTestClient. Therefore production code is not affected by this change. PiperOrigin-RevId: 305281258 Change-Id: I65be9886574f849bf25fe0f1e5c05ee2c6a453a9
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index c376d01..06962b0 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -192,7 +192,6 @@ headers_decompressed_(false), header_list_size_limit_exceeded_(false), headers_payload_length_(0), - trailers_payload_length_(0), trailers_decompressed_(false), trailers_consumed_(false), http_decoder_visitor_(std::make_unique<HttpDecoderVisitor>(this)), @@ -229,7 +228,6 @@ headers_decompressed_(false), header_list_size_limit_exceeded_(false), headers_payload_length_(0), - trailers_payload_length_(0), trailers_decompressed_(false), trailers_consumed_(false), http_decoder_visitor_(std::make_unique<HttpDecoderVisitor>(this)), @@ -569,10 +567,7 @@ debug_visitor->OnHeadersDecoded(id(), headers); } - const QuicByteCount frame_length = headers_decompressed_ - ? trailers_payload_length_ - : headers_payload_length_; - OnStreamHeaderList(/* fin = */ false, frame_length, headers); + OnStreamHeaderList(/* fin = */ false, headers_payload_length_, headers); } else { if (debug_visitor) { debug_visitor->OnPushPromiseDecoded(id(), promised_stream_id, headers); @@ -962,6 +957,8 @@ payload_length); } + headers_payload_length_ = payload_length; + if (trailers_decompressed_) { stream_delegate()->OnStreamError( QUIC_HTTP_INVALID_FRAME_SEQUENCE_ON_SPDY_STREAM, @@ -983,15 +980,6 @@ DCHECK(VersionUsesHttp3(transport_version())); DCHECK(qpack_decoded_headers_accumulator_); - // TODO(b/152518220): Save |payload_length| argument of OnHeadersFrameStart() - // instead of accumulating payload length in |headers_payload_length_| or - // |trailers_payload_length_|. - if (headers_decompressed_) { - trailers_payload_length_ += payload.length(); - } else { - headers_payload_length_ += payload.length(); - } - qpack_decoded_headers_accumulator_->Decode(payload); // |qpack_decoded_headers_accumulator_| is reset if an error is detected. @@ -1040,7 +1028,7 @@ id(), push_id, header_block_length); } - // TODO(renjietang): Check max push id and handle errors. + // TODO(b/151749109): Check max push id and handle errors. spdy_session_->OnPushPromise(id(), push_id); sequencer()->MarkConsumed(body_manager_.OnNonBody(push_id_length));
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h index abf3ef4..61bc1fb 100644 --- a/quic/core/http/quic_spdy_stream.h +++ b/quic/core/http/quic_spdy_stream.h
@@ -300,10 +300,8 @@ // Contains a copy of the decompressed header (name, value) pairs until they // are consumed via Readv. QuicHeaderList header_list_; - // Length of HEADERS frame payload. + // Length of most recently received HEADERS frame payload. QuicByteCount headers_payload_length_; - // Length of TRAILERS frame payload. - QuicByteCount trailers_payload_length_; // True if the trailers have been completely decompressed. bool trailers_decompressed_;
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc index 4f9aefa..f46d914 100644 --- a/quic/core/http/quic_spdy_stream_test.cc +++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -144,7 +144,8 @@ QuicSpdySession* session, bool should_process_data) : QuicSpdyStream(id, session, BIDIRECTIONAL), - should_process_data_(should_process_data) {} + should_process_data_(should_process_data), + headers_payload_length_(0) {} ~TestStream() override = default; using QuicSpdyStream::set_ack_listener; @@ -187,10 +188,20 @@ return QuicStream::sequencer(); } + void OnStreamHeaderList(bool fin, + size_t frame_len, + const QuicHeaderList& header_list) override { + headers_payload_length_ = frame_len; + QuicSpdyStream::OnStreamHeaderList(fin, frame_len, header_list); + } + + size_t headers_payload_length() const { return headers_payload_length_; } + private: bool should_process_data_; spdy::SpdyHeaderBlock saved_headers_; std::string data_; + size_t headers_payload_length_; }; class TestSession : public MockQuicSpdySession { @@ -391,6 +402,21 @@ return quiche::QuicheStrCat(headers_frame_header, payload); } + // Construct PUSH_PROMISE frame with given payload. + std::string SerializePushPromiseFrame(PushId push_id, + quiche::QuicheStringPiece payload) { + PushPromiseFrame frame; + frame.push_id = push_id; + frame.headers = payload; + std::unique_ptr<char[]> push_promise_buffer; + QuicByteCount push_promise_frame_header_length = + HttpEncoder::SerializePushPromiseFrameWithOnlyPushId( + frame, &push_promise_buffer); + quiche::QuicheStringPiece push_promise_frame_header( + push_promise_buffer.get(), push_promise_frame_header_length); + return quiche::QuicheStrCat(push_promise_frame_header, payload); + } + std::string DataFrame(quiche::QuicheStringPiece payload) { std::unique_ptr<char[]> data_buffer; QuicByteCount data_frame_header_length = @@ -2618,13 +2644,7 @@ std::string headers = EncodeQpackHeaders(pushed_headers); const QuicStreamId push_id = 1; - PushPromiseFrame push_promise; - push_promise.push_id = push_id; - push_promise.headers = headers; - std::unique_ptr<char[]> buffer; - uint64_t length = HttpEncoder::SerializePushPromiseFrameWithOnlyPushId( - push_promise, &buffer); - std::string data = std::string(buffer.get(), length) + headers; + std::string data = SerializePushPromiseFrame(push_id, headers); QuicStreamFrame frame(stream_->id(), false, 0, data); EXPECT_CALL(debug_visitor, OnPushPromiseFrameReceived(stream_->id(), push_id, @@ -2633,11 +2653,40 @@ OnPushPromiseDecoded(stream_->id(), push_id, AsHeaderList(pushed_headers))); EXPECT_CALL(*session_, - OnPromiseHeaderList(stream_->id(), push_promise.push_id, - headers.length(), _)); + OnPromiseHeaderList(stream_->id(), push_id, headers.length(), _)); stream_->OnStreamFrame(frame); } +// Regression test for b/152518220. +TEST_P(QuicSpdyStreamTest, + OnStreamHeaderBlockArgumentDoesNotIncludePushedHeaderBlock) { + Initialize(kShouldProcessData); + if (!UsesHttp3()) { + return; + } + + std::string pushed_headers = EncodeQpackHeaders({{"foo", "bar"}}); + const QuicStreamId push_id = 1; + std::string push_promise_frame = + SerializePushPromiseFrame(push_id, pushed_headers); + QuicStreamOffset offset = 0; + QuicStreamFrame frame1(stream_->id(), /* fin = */ false, offset, + push_promise_frame); + offset += push_promise_frame.length(); + + EXPECT_CALL(*session_, OnPromiseHeaderList(stream_->id(), push_id, + pushed_headers.length(), _)); + stream_->OnStreamFrame(frame1); + + std::string headers = + EncodeQpackHeaders({{":method", "GET"}, {":path", "/"}}); + std::string headers_frame = HeadersFrame(headers); + QuicStreamFrame frame2(stream_->id(), /* fin = */ false, offset, + headers_frame); + stream_->OnStreamFrame(frame2); + EXPECT_EQ(headers.length(), stream_->headers_payload_length()); +} + // Close connection if a DATA frame is received before a HEADERS frame. TEST_P(QuicSpdyStreamTest, DataBeforeHeaders) { if (!UsesHttp3()) {