Add header_list_size_limit_exceeded argument to OnHeadersDecoded(). The motivation is to allow tests to call OnHeadersDecoded() directly without QuicSpdyStream having a QpackDecodedHeadersAccumulator object. This happens in Envoy at https://github.com/envoyproxy/envoy/blob/master/test/extensions/quic_listeners/quiche/envoy_quic_server_stream_test.cc#L110, which crashes since cr/282822239 added a reference to |qpack_decoded_headers_accumulator_| in QuicSpdyStream::OnHeadersDecoded(). gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 284213476 Change-Id: I71737f85d8a00db900565dd16c69510844ef3999
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc index f7a13f6..78d8593 100644 --- a/quic/core/http/quic_spdy_stream.cc +++ b/quic/core/http/quic_spdy_stream.cc
@@ -538,9 +538,9 @@ } } -void QuicSpdyStream::OnHeadersDecoded(QuicHeaderList headers) { - header_list_size_limit_exceeded_ = - qpack_decoded_headers_accumulator_->header_list_size_limit_exceeded(); +void QuicSpdyStream::OnHeadersDecoded(QuicHeaderList headers, + bool header_list_size_limit_exceeded) { + header_list_size_limit_exceeded_ = header_list_size_limit_exceeded; qpack_decoded_headers_accumulator_.reset(); QuicSpdySession::LogHeaderCompressionRatioHistogram(
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h index 8610dbb..c4333f3 100644 --- a/quic/core/http/quic_spdy_stream.h +++ b/quic/core/http/quic_spdy_stream.h
@@ -214,7 +214,8 @@ bool IsClosed() { return sequencer()->IsClosed(); } // QpackDecodedHeadersAccumulator::Visitor implementation. - void OnHeadersDecoded(QuicHeaderList headers) override; + void OnHeadersDecoded(QuicHeaderList headers, + bool header_list_size_limit_exceeded) override; void OnHeaderDecodingError(QuicStringPiece error_message) override; protected:
diff --git a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc index 43a637d..018bf09 100644 --- a/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc +++ b/quic/core/qpack/fuzzer/qpack_round_trip_fuzzer.cc
@@ -264,8 +264,10 @@ virtual ~VerifyingDecoder() = default; // QpackDecodedHeadersAccumulator::Visitor implementation. - void OnHeadersDecoded(QuicHeaderList headers) override { + void OnHeadersDecoded(QuicHeaderList headers, + bool header_list_size_limit_exceeded) override { // Verify headers. + CHECK(!header_list_size_limit_exceeded); CHECK(expected_header_list_ == headers); // Might destroy |this|.
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator.cc b/quic/core/qpack/qpack_decoded_headers_accumulator.cc index e16aa5a..b1f8012 100644 --- a/quic/core/qpack/qpack_decoded_headers_accumulator.cc +++ b/quic/core/qpack/qpack_decoded_headers_accumulator.cc
@@ -62,7 +62,8 @@ uncompressed_header_bytes_without_overhead_, compressed_header_bytes_); // Might destroy |this|. - visitor_->OnHeadersDecoded(std::move(quic_header_list_)); + visitor_->OnHeadersDecoded(std::move(quic_header_list_), + header_list_size_limit_exceeded_); } void QpackDecodedHeadersAccumulator::OnDecodingErrorDetected(
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator.h b/quic/core/qpack/qpack_decoded_headers_accumulator.h index 0a2db6a..96b527e 100644 --- a/quic/core/qpack/qpack_decoded_headers_accumulator.h +++ b/quic/core/qpack/qpack_decoded_headers_accumulator.h
@@ -34,13 +34,15 @@ public: virtual ~Visitor() = default; - // Called when headers are successfully decoded. If header list size - // exceeds the limit specified via |max_header_list_size| in - // QpackDecodedHeadersAccumulator constructor, then |headers| will be empty, - // but will still have the correct compressed and uncompressed size - // information. However, header_list_size_limit_exceeded() is recommended - // instead of headers.empty() to check whether header size exceeds limit. - virtual void OnHeadersDecoded(QuicHeaderList headers) = 0; + // Called when headers are successfully decoded. If the uncompressed header + // list size including an overhead for each header field exceeds the limit + // specified via |max_header_list_size| in QpackDecodedHeadersAccumulator + // constructor, then |header_list_size_limit_exceeded| will be true, and + // |headers| will be empty but will still have the correct compressed and + // uncompressed size + // information. + virtual void OnHeadersDecoded(QuicHeaderList headers, + bool header_list_size_limit_exceeded) = 0; // Called when an error has occurred. virtual void OnHeaderDecodingError(QuicStringPiece error_message) = 0; @@ -68,13 +70,6 @@ // Must not be called more that once. void EndHeaderBlock(); - // Returns true if the uncompressed size of the header list, including an - // overhead for each header field, exceeds |max_header_list_size| passed in - // the constructor. - bool header_list_size_limit_exceeded() const { - return header_list_size_limit_exceeded_; - } - private: std::unique_ptr<QpackProgressiveDecoder> decoder_; Visitor* visitor_;
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc index 6616517..2334e89 100644 --- a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc +++ b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
@@ -41,7 +41,9 @@ class MockVisitor : public QpackDecodedHeadersAccumulator::Visitor { public: ~MockVisitor() override = default; - MOCK_METHOD1(OnHeadersDecoded, void(QuicHeaderList headers)); + MOCK_METHOD2(OnHeadersDecoded, + void(QuicHeaderList headers, + bool header_list_size_limit_exceeded)); MOCK_METHOD1(OnHeaderDecodingError, void(QuicStringPiece error_message)); }; @@ -89,9 +91,9 @@ accumulator_.Decode(encoded_data); QuicHeaderList header_list; - EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list)); + EXPECT_CALL(visitor_, OnHeadersDecoded(_, false)) + .WillOnce(SaveArg<0>(&header_list)); accumulator_.EndHeaderBlock(); - EXPECT_FALSE(accumulator_.header_list_size_limit_exceeded()); EXPECT_EQ(0u, header_list.uncompressed_header_bytes()); EXPECT_EQ(encoded_data.size(), header_list.compressed_header_bytes()); @@ -119,9 +121,9 @@ accumulator_.Decode(encoded_data); QuicHeaderList header_list; - EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list)); + EXPECT_CALL(visitor_, OnHeadersDecoded(_, false)) + .WillOnce(SaveArg<0>(&header_list)); accumulator_.EndHeaderBlock(); - EXPECT_FALSE(accumulator_.header_list_size_limit_exceeded()); EXPECT_THAT(header_list, ElementsAre(Pair("foo", "bar"))); EXPECT_EQ(strlen("foo") + strlen("bar"), @@ -145,9 +147,8 @@ "0f" // second byte of a two-byte long Indexed Header Field instruction )); - EXPECT_CALL(visitor_, OnHeadersDecoded(_)); + EXPECT_CALL(visitor_, OnHeadersDecoded(_, true)); accumulator_.EndHeaderBlock(); - EXPECT_TRUE(accumulator_.header_list_size_limit_exceeded()); } // Test that header list limit enforcement works with blocked encoding. @@ -169,9 +170,8 @@ EXPECT_CALL(decoder_stream_sender_delegate_, WriteStreamData(Eq(kHeaderAcknowledgement))); - EXPECT_CALL(visitor_, OnHeadersDecoded(_)); + EXPECT_CALL(visitor_, OnHeadersDecoded(_, true)); qpack_decoder_.OnInsertWithoutNameReference("foo", "bar"); - EXPECT_TRUE(accumulator_.header_list_size_limit_exceeded()); } TEST_F(QpackDecodedHeadersAccumulatorTest, BlockedDecoding) { @@ -187,10 +187,10 @@ WriteStreamData(Eq(kHeaderAcknowledgement))); QuicHeaderList header_list; - EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list)); + EXPECT_CALL(visitor_, OnHeadersDecoded(_, false)) + .WillOnce(SaveArg<0>(&header_list)); qpack_decoder_.OnInsertWithoutNameReference("foo", "bar"); - EXPECT_FALSE(accumulator_.header_list_size_limit_exceeded()); EXPECT_THAT(header_list, ElementsAre(Pair("foo", "bar"))); EXPECT_EQ(strlen("foo") + strlen("bar"), header_list.uncompressed_header_bytes()); @@ -213,9 +213,9 @@ accumulator_.Decode(QuicTextUtils::HexDecode("80")); QuicHeaderList header_list; - EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list)); + EXPECT_CALL(visitor_, OnHeadersDecoded(_, false)) + .WillOnce(SaveArg<0>(&header_list)); accumulator_.EndHeaderBlock(); - EXPECT_FALSE(accumulator_.header_list_size_limit_exceeded()); EXPECT_THAT(header_list, ElementsAre(Pair("foo", "bar"), Pair("foo", "bar"))); }