Enforce header list size limit in QpackDecodedHeadersAccumulator.
Move header list size limit enforcement from QuicHeaderList to
QpackDecodedHeadersAccumulator when using IETF QUIC. This provides an explicit
signal, instead of having to rely on QuicHeaderList being empty.
Also change limit counting to account for 32 bytes of overhead per header field
as prescribed by the specification. Keep |uncompressed_size| passed in to
QuicHeaderList without this overhead, just like it is when called from
HpackDecoder::ListenerAdapter (used for Google QUIC) and from AsHeaderList (used
for tests).
gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99.
PiperOrigin-RevId: 282822239
Change-Id: If9b27ed3189cad5bfdb6be415196e7c0f2267a74
diff --git a/quic/core/http/quic_spdy_stream.cc b/quic/core/http/quic_spdy_stream.cc
index b0c4343..4c5e642 100644
--- a/quic/core/http/quic_spdy_stream.cc
+++ b/quic/core/http/quic_spdy_stream.cc
@@ -193,6 +193,7 @@
visitor_(nullptr),
blocked_on_decoding_headers_(false),
headers_decompressed_(false),
+ header_list_size_limit_exceeded_(false),
headers_payload_length_(0),
trailers_payload_length_(0),
trailers_decompressed_(false),
@@ -227,6 +228,7 @@
visitor_(nullptr),
blocked_on_decoding_headers_(false),
headers_decompressed_(false),
+ header_list_size_limit_exceeded_(false),
headers_payload_length_(0),
trailers_payload_length_(0),
trailers_decompressed_(false),
@@ -517,13 +519,13 @@
size_t frame_len,
const QuicHeaderList& header_list) {
// TODO(b/134706391): remove |fin| argument.
- // The headers list avoid infinite buffering by clearing the headers list
- // if the current headers are too large. So if the list is empty here
- // then the headers list must have been too large, and the stream should
- // be reset.
- // TODO(rch): Use an explicit "headers too large" signal. An empty header list
- // might be acceptable if it corresponds to a trailing header frame.
- if (header_list.empty()) {
+ // When using Google QUIC, an empty header list indicates that the size limit
+ // has been exceeded.
+ // When using IETF QUIC, there is an explicit signal from
+ // QpackDecodedHeadersAccumulator.
+ if ((VersionUsesHttp3(transport_version()) &&
+ header_list_size_limit_exceeded_) ||
+ (!VersionUsesHttp3(transport_version()) && header_list.empty())) {
OnHeadersTooLarge();
if (IsDoneReading()) {
return;
@@ -537,6 +539,8 @@
}
void QuicSpdyStream::OnHeadersDecoded(QuicHeaderList headers) {
+ header_list_size_limit_exceeded_ =
+ qpack_decoded_headers_accumulator_->header_list_size_limit_exceeded();
qpack_decoded_headers_accumulator_.reset();
QuicSpdySession::LogHeaderCompressionRatioHistogram(
@@ -564,8 +568,6 @@
void QuicSpdyStream::OnHeaderDecodingError(QuicStringPiece error_message) {
qpack_decoded_headers_accumulator_.reset();
- // TODO(b/124216424): Use HTTP_EXCESSIVE_LOAD instead if indicated by
- // |qpack_decoded_headers_accumulator_|.
std::string connection_close_error_message = QuicStrCat(
"Error decoding ", headers_decompressed_ ? "trailers" : "headers",
" on stream ", id(), ": ", error_message);
@@ -575,7 +577,8 @@
void QuicSpdyStream::OnHeadersTooLarge() {
if (VersionUsesHttp3(transport_version())) {
- // TODO(124216424): Use HTTP_EXCESSIVE_LOAD error code.
+ // TODO(b/124216424): Reset stream with H3_REQUEST_CANCELLED (if client)
+ // or with H3_REQUEST_REJECTED (if server).
std::string error_message =
QuicStrCat("Too large headers received on stream ", id());
CloseConnectionWithDetails(QUIC_HEADERS_STREAM_DATA_DECOMPRESS_FAILURE,
diff --git a/quic/core/http/quic_spdy_stream.h b/quic/core/http/quic_spdy_stream.h
index a7a0939..e315dcc 100644
--- a/quic/core/http/quic_spdy_stream.h
+++ b/quic/core/http/quic_spdy_stream.h
@@ -281,6 +281,9 @@
bool blocked_on_decoding_headers_;
// True if the headers have been completely decompressed.
bool headers_decompressed_;
+ // True if uncompressed headers or trailers exceed maximum allowed size
+ // advertised to peer via SETTINGS_MAX_HEADER_LIST_SIZE.
+ bool header_list_size_limit_exceeded_;
// Contains a copy of the decompressed header (name, value) pairs until they
// are consumed via Readv.
QuicHeaderList header_list_;
diff --git a/quic/core/http/quic_spdy_stream_test.cc b/quic/core/http/quic_spdy_stream_test.cc
index 39bd6b0..74ad6ef 100644
--- a/quic/core/http/quic_spdy_stream_test.cc
+++ b/quic/core/http/quic_spdy_stream_test.cc
@@ -346,26 +346,36 @@
TEST_P(QuicSpdyStreamTest, ProcessTooLargeHeaderList) {
Initialize(kShouldProcessData);
- QuicHeaderList headers;
- stream_->OnStreamHeadersPriority(
- spdy::SpdyStreamPrecedence(kV3HighestPriority));
+ if (!UsesHttp3()) {
+ QuicHeaderList headers;
+ stream_->OnStreamHeadersPriority(
+ spdy::SpdyStreamPrecedence(kV3HighestPriority));
- if (UsesHttp3()) {
- EXPECT_CALL(
- *connection_,
- CloseConnection(
- QUIC_HEADERS_STREAM_DATA_DECOMPRESS_FAILURE,
- MatchesRegex("Too large headers received on stream \\d+"), _));
- } else {
EXPECT_CALL(*session_,
SendRstStream(stream_->id(), QUIC_HEADERS_TOO_LARGE, 0));
- }
+ stream_->OnStreamHeaderList(false, 1 << 20, headers);
- stream_->OnStreamHeaderList(false, 1 << 20, headers);
-
- if (!UsesHttp3()) {
EXPECT_THAT(stream_->stream_error(), IsStreamError(QUIC_HEADERS_TOO_LARGE));
+
+ return;
}
+
+ // Header list size includes 32 bytes for overhead per header field.
+ session_->set_max_inbound_header_list_size(40);
+ std::string headers =
+ HeadersFrame({std::make_pair("foo", "too long headers")});
+
+ QuicStreamFrame frame(stream_->id(), false, 0, headers);
+
+ EXPECT_CALL(
+ *connection_,
+ CloseConnection(QUIC_HEADERS_STREAM_DATA_DECOMPRESS_FAILURE,
+ MatchesRegex("Too large headers received on stream \\d+"),
+ _));
+
+ stream_->OnStreamFrame(frame);
+
+ EXPECT_TRUE(stream_->header_list().empty());
}
TEST_P(QuicSpdyStreamTest, ProcessHeaderListWithFin) {
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator.cc b/quic/core/qpack/qpack_decoded_headers_accumulator.cc
index 7c305ed..e16aa5a 100644
--- a/quic/core/qpack/qpack_decoded_headers_accumulator.cc
+++ b/quic/core/qpack/qpack_decoded_headers_accumulator.cc
@@ -8,6 +8,12 @@
namespace quic {
+namespace {
+
+size_t kHeaderFieldSizeOverhead = 32;
+
+}
+
QpackDecodedHeadersAccumulator::QpackDecodedHeadersAccumulator(
QuicStreamId id,
QpackDecoder* qpack_decoder,
@@ -15,11 +21,13 @@
size_t max_header_list_size)
: decoder_(qpack_decoder->CreateProgressiveDecoder(id, this)),
visitor_(visitor),
- uncompressed_header_bytes_(0),
+ max_header_list_size_(max_header_list_size),
+ uncompressed_header_bytes_including_overhead_(0),
+ uncompressed_header_bytes_without_overhead_(0),
compressed_header_bytes_(0),
+ header_list_size_limit_exceeded_(false),
headers_decoded_(false),
error_detected_(false) {
- quic_header_list_.set_max_header_list_size(max_header_list_size);
quic_header_list_.OnHeaderBlockStart();
}
@@ -27,8 +35,21 @@
QuicStringPiece value) {
DCHECK(!error_detected_);
- uncompressed_header_bytes_ += name.size() + value.size();
- quic_header_list_.OnHeader(name, value);
+ uncompressed_header_bytes_without_overhead_ += name.size() + value.size();
+
+ if (header_list_size_limit_exceeded_) {
+ return;
+ }
+
+ uncompressed_header_bytes_including_overhead_ +=
+ name.size() + value.size() + kHeaderFieldSizeOverhead;
+
+ if (uncompressed_header_bytes_including_overhead_ > max_header_list_size_) {
+ header_list_size_limit_exceeded_ = true;
+ quic_header_list_.Clear();
+ } else {
+ quic_header_list_.OnHeader(name, value);
+ }
}
void QpackDecodedHeadersAccumulator::OnDecodingCompleted() {
@@ -36,8 +57,9 @@
DCHECK(!error_detected_);
headers_decoded_ = true;
- quic_header_list_.OnHeaderBlockEnd(uncompressed_header_bytes_,
- compressed_header_bytes_);
+
+ quic_header_list_.OnHeaderBlockEnd(
+ uncompressed_header_bytes_without_overhead_, compressed_header_bytes_);
// Might destroy |this|.
visitor_->OnHeadersDecoded(std::move(quic_header_list_));
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator.h b/quic/core/qpack/qpack_decoded_headers_accumulator.h
index 460c189..0a2db6a 100644
--- a/quic/core/qpack/qpack_decoded_headers_accumulator.h
+++ b/quic/core/qpack/qpack_decoded_headers_accumulator.h
@@ -34,7 +34,12 @@
public:
virtual ~Visitor() = default;
- // Called when headers are successfully decoded.
+ // 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 an error has occurred.
@@ -63,15 +68,37 @@
// 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_;
+ // Maximum header list size including overhead.
+ size_t max_header_list_size_;
+ // Uncompressed header list size including overhead, for enforcing the limit.
+ size_t uncompressed_header_bytes_including_overhead_;
QuicHeaderList quic_header_list_;
- size_t uncompressed_header_bytes_;
+ // Uncompressed header list size with overhead,
+ // for passing in to QuicHeaderList::OnHeaderBlockEnd().
+ size_t uncompressed_header_bytes_without_overhead_;
+ // Compressed header list size
+ // for passing in to QuicHeaderList::OnHeaderBlockEnd().
size_t compressed_header_bytes_;
+
+ // True if the header size limit has been exceeded.
+ // Input data is still fed to QpackProgressiveDecoder.
+ bool header_list_size_limit_exceeded_;
+
+ // The following two members are only used for DCHECKs.
+
// True if headers have been completedly and successfully decoded.
bool headers_decoded_;
- // An error is detected during decoding.
+ // True if an error has been detected during decoding.
bool error_detected_;
};
diff --git a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
index a4f7f14..6616517 100644
--- a/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
+++ b/quic/core/qpack/qpack_decoded_headers_accumulator_test.cc
@@ -91,6 +91,7 @@
QuicHeaderList header_list;
EXPECT_CALL(visitor_, OnHeadersDecoded(_)).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());
@@ -120,6 +121,7 @@
QuicHeaderList header_list;
EXPECT_CALL(visitor_, OnHeadersDecoded(_)).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"),
@@ -127,25 +129,49 @@
EXPECT_EQ(encoded_data.size(), header_list.compressed_header_bytes());
}
-TEST_F(QpackDecodedHeadersAccumulatorTest, ExceedingLimit) {
+// Test that Decode() calls are not ignored after header list limit is exceeded,
+// otherwise decoding could fail with "incomplete header block" error.
+TEST_F(QpackDecodedHeadersAccumulatorTest, ExceedLimitThenSplitInstruction) {
// Total length of header list exceeds kMaxHeaderListSize.
- std::string encoded_data(QuicTextUtils::HexDecode(
+ accumulator_.Decode(QuicTextUtils::HexDecode(
"0000" // header block prefix
"26666f6f626172" // header key: "foobar"
"7d61616161616161616161616161616161616161" // header value: 'a' 125 times
"616161616161616161616161616161616161616161616161616161616161616161616161"
"616161616161616161616161616161616161616161616161616161616161616161616161"
- "61616161616161616161616161616161616161616161616161616161616161616161"));
- accumulator_.Decode(encoded_data);
+ "61616161616161616161616161616161616161616161616161616161616161616161"
+ "ff")); // first byte of a two-byte long Indexed Header Field instruction
+ accumulator_.Decode(QuicTextUtils::HexDecode(
+ "0f" // second byte of a two-byte long Indexed Header Field instruction
+ ));
- QuicHeaderList header_list;
- EXPECT_CALL(visitor_, OnHeadersDecoded(_)).WillOnce(SaveArg<0>(&header_list));
+ EXPECT_CALL(visitor_, OnHeadersDecoded(_));
+ accumulator_.EndHeaderBlock();
+ EXPECT_TRUE(accumulator_.header_list_size_limit_exceeded());
+}
+
+// Test that header list limit enforcement works with blocked encoding.
+TEST_F(QpackDecodedHeadersAccumulatorTest, ExceedLimitBlocked) {
+ // Total length of header list exceeds kMaxHeaderListSize.
+ accumulator_.Decode(QuicTextUtils::HexDecode(
+ "0200" // header block prefix
+ "80" // reference to dynamic table entry not yet received
+ "26666f6f626172" // header key: "foobar"
+ "7d61616161616161616161616161616161616161" // header value: 'a' 125 times
+ "616161616161616161616161616161616161616161616161616161616161616161616161"
+ "616161616161616161616161616161616161616161616161616161616161616161616161"
+ "61616161616161616161616161616161616161616161616161616161616161616161"));
accumulator_.EndHeaderBlock();
- // QuicHeaderList signals header list over limit by clearing it.
- EXPECT_EQ(0u, header_list.uncompressed_header_bytes());
- EXPECT_EQ(0u, header_list.compressed_header_bytes());
- EXPECT_TRUE(header_list.empty());
+ // Set dynamic table capacity.
+ qpack_decoder_.OnSetDynamicTableCapacity(kMaxDynamicTableCapacity);
+ // Adding dynamic table entry unblocks decoding.
+ EXPECT_CALL(decoder_stream_sender_delegate_,
+ WriteStreamData(Eq(kHeaderAcknowledgement)));
+
+ EXPECT_CALL(visitor_, OnHeadersDecoded(_));
+ qpack_decoder_.OnInsertWithoutNameReference("foo", "bar");
+ EXPECT_TRUE(accumulator_.header_list_size_limit_exceeded());
}
TEST_F(QpackDecodedHeadersAccumulatorTest, BlockedDecoding) {
@@ -164,6 +190,7 @@
EXPECT_CALL(visitor_, OnHeadersDecoded(_)).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());
@@ -188,6 +215,7 @@
QuicHeaderList header_list;
EXPECT_CALL(visitor_, OnHeadersDecoded(_)).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")));
}
