Enforce limit on number of blocked streams in QPACK decoder. gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 264688336 Change-Id: Iab1e97a9f31ef95d8dfa05ca8439bf066913b5ba
diff --git a/quic/core/http/http_constants.h b/quic/core/http/http_constants.h index 84d0abd..9c2c6b5 100644 --- a/quic/core/http/http_constants.h +++ b/quic/core/http/http_constants.h
@@ -36,6 +36,10 @@ // SETTINGS_QPACK_MAX_TABLE_CAPACITY. const QuicByteCount kDefaultQpackMaxDynamicTableCapacity = 64 * 1024; // 64 KB +// Default limit on number of blocked streams, communicated via +// SETTINGS_QPACK_BLOCKED_STREAMS. +const uint64_t kDefaultMaximumBlockedStreams = 100; + } // namespace quic #endif // QUICHE_QUIC_CORE_HTTP_HTTP_CONSTANTS_H_
diff --git a/quic/core/http/quic_spdy_session.cc b/quic/core/http/quic_spdy_session.cc index 619c881..29c6d9d 100644 --- a/quic/core/http/quic_spdy_session.cc +++ b/quic/core/http/quic_spdy_session.cc
@@ -380,13 +380,11 @@ qpack_encoder_ = QuicMakeUnique<QpackEncoder>(this); qpack_decoder_ = QuicMakeUnique<QpackDecoder>(kDefaultQpackMaxDynamicTableCapacity, - /* maximum_blocked_streams = */ 0, this); + kDefaultMaximumBlockedStreams, this); MaybeInitializeHttp3UnidirectionalStreams(); - // TODO(b/112770235): Set sensible limit on maximum number of blocked - // streams. // TODO(b/112770235): Send SETTINGS_QPACK_MAX_TABLE_CAPACITY with value // kDefaultQpackMaxDynamicTableCapacity, and SETTINGS_QPACK_BLOCKED_STREAMS - // with limit on maximum number of blocked streams. + // with value kDefaultMaximumBlockedStreams. } spdy_framer_visitor_->set_max_header_list_size(max_inbound_header_list_size_);
diff --git a/quic/core/qpack/qpack_decoder.cc b/quic/core/qpack/qpack_decoder.cc index 7fac1f2..b20cbdf 100644 --- a/quic/core/qpack/qpack_decoder.cc +++ b/quic/core/qpack/qpack_decoder.cc
@@ -12,10 +12,11 @@ QpackDecoder::QpackDecoder( uint64_t maximum_dynamic_table_capacity, - uint64_t /* maximum_blocked_streams */, + uint64_t maximum_blocked_streams, EncoderStreamErrorDelegate* encoder_stream_error_delegate) : encoder_stream_error_delegate_(encoder_stream_error_delegate), - encoder_stream_receiver_(this) { + encoder_stream_receiver_(this), + maximum_blocked_streams_(maximum_blocked_streams) { DCHECK(encoder_stream_error_delegate_); header_table_.SetMaximumDynamicTableCapacity(maximum_dynamic_table_capacity); @@ -29,6 +30,17 @@ decoder_stream_sender_.SendStreamCancellation(stream_id); } +bool QpackDecoder::OnStreamBlocked(QuicStreamId stream_id) { + auto result = blocked_streams_.insert(stream_id); + DCHECK(result.second); + return blocked_streams_.size() <= maximum_blocked_streams_; +} + +void QpackDecoder::OnStreamUnblocked(QuicStreamId stream_id) { + size_t result = blocked_streams_.erase(stream_id); + DCHECK_EQ(1u, result); +} + void QpackDecoder::OnInsertWithNameReference(bool is_static, uint64_t name_index, QuicStringPiece value) { @@ -117,7 +129,7 @@ QuicStreamId stream_id, QpackProgressiveDecoder::HeadersHandlerInterface* handler) { return QuicMakeUnique<QpackProgressiveDecoder>( - stream_id, &header_table_, &decoder_stream_sender_, handler); + stream_id, this, &header_table_, &decoder_stream_sender_, handler); } } // namespace quic
diff --git a/quic/core/qpack/qpack_decoder.h b/quic/core/qpack/qpack_decoder.h index 1ec2d39..87d650f 100644 --- a/quic/core/qpack/qpack_decoder.h +++ b/quic/core/qpack/qpack_decoder.h
@@ -22,7 +22,8 @@ // This class vends a new QpackProgressiveDecoder instance for each new header // list to be encoded. class QUIC_EXPORT_PRIVATE QpackDecoder - : public QpackEncoderStreamReceiver::Delegate { + : public QpackEncoderStreamReceiver::Delegate, + public QpackProgressiveDecoder::BlockedStreamLimitEnforcer { public: // Interface for receiving notification that an error has occurred on the // encoder stream. This MUST be treated as a connection error of type @@ -57,6 +58,10 @@ // using the FIN bit. void OnStreamReset(QuicStreamId stream_id); + // QpackProgressiveDecoder::BlockedStreamLimitEnforcer implementation. + bool OnStreamBlocked(QuicStreamId stream_id) override; + void OnStreamUnblocked(QuicStreamId stream_id) override; + // Factory method to create a QpackProgressiveDecoder for decoding a header // block. |handler| must remain valid until the returned // QpackProgressiveDecoder instance is destroyed or the decoder calls @@ -89,6 +94,8 @@ QpackEncoderStreamReceiver encoder_stream_receiver_; QpackDecoderStreamSender decoder_stream_sender_; QpackHeaderTable header_table_; + std::set<QuicStreamId> blocked_streams_; + const uint64_t maximum_blocked_streams_; }; } // namespace quic
diff --git a/quic/core/qpack/qpack_decoder_test.cc b/quic/core/qpack/qpack_decoder_test.cc index 556053b..0a4e5e2 100644 --- a/quic/core/qpack/qpack_decoder_test.cc +++ b/quic/core/qpack/qpack_decoder_test.cc
@@ -26,16 +26,15 @@ // Header Acknowledgement decoder stream instruction with stream_id = 1. const char* const kHeaderAcknowledgement = "\x81"; -// TODO(b/112770235) Change this constant, enforce the limit and add tests. -const uint64_t kMaximumBlockedStreams = 0; +const uint64_t kMaximumDynamicTableCapacity = 1024; +const uint64_t kMaximumBlockedStreams = 1; class QpackDecoderTest : public QuicTestWithParam<FragmentMode> { protected: QpackDecoderTest() - : qpack_decoder_( - /* maximum_dynamic_table_capacity = */ 1024, - kMaximumBlockedStreams, - &encoder_stream_error_delegate_), + : qpack_decoder_(kMaximumDynamicTableCapacity, + kMaximumBlockedStreams, + &encoder_stream_error_delegate_), fragment_mode_(GetParam()) { qpack_decoder_.set_qpack_stream_sender_delegate( &decoder_stream_sender_delegate_); @@ -47,10 +46,14 @@ qpack_decoder_.encoder_stream_receiver()->Decode(data); } + std::unique_ptr<QpackProgressiveDecoder> CreateProgressiveDecoder( + QuicStreamId stream_id) { + return qpack_decoder_.CreateProgressiveDecoder(stream_id, &handler_); + } + // Set up |progressive_decoder_|. void StartDecoding() { - progressive_decoder_ = - qpack_decoder_.CreateProgressiveDecoder(/* stream_id = */ 1, &handler_); + progressive_decoder_ = CreateProgressiveDecoder(/* stream_id = */ 1); } // Pass header block data to QpackProgressiveDecoder::Decode() @@ -813,6 +816,21 @@ DecodeEncoderStreamData(QuicTextUtils::HexDecode("6294e70362617a")); } +TEST_P(QpackDecoderTest, TooManyBlockedStreams) { + // Required Insert Count 1 and Delta Base 0. + // Without any dynamic table entries received, decoding is blocked. + std::string data = QuicTextUtils::HexDecode("0200"); + + auto progressive_decoder1 = CreateProgressiveDecoder(/* stream_id = */ 1); + progressive_decoder1->Decode(data); + + EXPECT_CALL(handler_, OnDecodingErrorDetected(Eq( + "Limit on number of blocked streams exceeded."))); + + auto progressive_decoder2 = CreateProgressiveDecoder(/* stream_id = */ 2); + progressive_decoder2->Decode(data); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/qpack/qpack_progressive_decoder.cc b/quic/core/qpack/qpack_progressive_decoder.cc index e5f7f09..51d98ff 100644 --- a/quic/core/qpack/qpack_progressive_decoder.cc +++ b/quic/core/qpack/qpack_progressive_decoder.cc
@@ -17,6 +17,7 @@ QpackProgressiveDecoder::QpackProgressiveDecoder( QuicStreamId stream_id, + BlockedStreamLimitEnforcer* enforcer, QpackHeaderTable* header_table, QpackDecoderStreamSender* decoder_stream_sender, HeadersHandlerInterface* handler) @@ -24,6 +25,7 @@ prefix_decoder_( QuicMakeUnique<QpackInstructionDecoder>(QpackPrefixLanguage(), this)), instruction_decoder_(QpackRequestStreamLanguage(), this), + enforcer_(enforcer), header_table_(header_table), decoder_stream_sender_(decoder_stream_sender), handler_(handler), @@ -117,6 +119,7 @@ } blocked_ = false; + enforcer_->OnStreamUnblocked(stream_id_); if (!decoding_) { FinishDecoding(); @@ -287,6 +290,10 @@ if (required_insert_count_ > header_table_->inserted_entry_count()) { blocked_ = true; + if (!enforcer_->OnStreamBlocked(stream_id_)) { + OnError("Limit on number of blocked streams exceeded."); + return false; + } header_table_->RegisterObserver(this, required_insert_count_); }
diff --git a/quic/core/qpack/qpack_progressive_decoder.h b/quic/core/qpack/qpack_progressive_decoder.h index 6d2e992..5c116b1 100644 --- a/quic/core/qpack/qpack_progressive_decoder.h +++ b/quic/core/qpack/qpack_progressive_decoder.h
@@ -49,8 +49,26 @@ virtual void OnDecodingErrorDetected(QuicStringPiece error_message) = 0; }; + // Interface for keeping track of blocked streams for the purpose of enforcing + // the limit communicated to peer via QPACK_BLOCKED_STREAMS settings. + class QUIC_EXPORT_PRIVATE BlockedStreamLimitEnforcer { + public: + virtual ~BlockedStreamLimitEnforcer() {} + + // Called when the stream becomes blocked. Returns true if allowed. Returns + // false if limit is violated, in which case QpackProgressiveDecoder signals + // an error. + // Stream must not be already blocked. + virtual bool OnStreamBlocked(QuicStreamId stream_id) = 0; + + // Called when the stream becomes unblocked. + // Stream must be blocked. + virtual void OnStreamUnblocked(QuicStreamId stream_id) = 0; + }; + QpackProgressiveDecoder() = delete; QpackProgressiveDecoder(QuicStreamId stream_id, + BlockedStreamLimitEnforcer* enforcer, QpackHeaderTable* header_table, QpackDecoderStreamSender* decoder_stream_sender, HeadersHandlerInterface* handler); @@ -97,6 +115,7 @@ std::unique_ptr<QpackInstructionDecoder> prefix_decoder_; QpackInstructionDecoder instruction_decoder_; + BlockedStreamLimitEnforcer* const enforcer_; QpackHeaderTable* const header_table_; QpackDecoderStreamSender* const decoder_stream_sender_; HeadersHandlerInterface* const handler_;
diff --git a/quic/core/qpack/qpack_progressive_decoder_test.cc b/quic/core/qpack/qpack_progressive_decoder_test.cc new file mode 100644 index 0000000..ccc7436 --- /dev/null +++ b/quic/core/qpack/qpack_progressive_decoder_test.cc
@@ -0,0 +1,109 @@ +// Copyright (c) 2019 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "net/third_party/quiche/src/quic/core/qpack/qpack_progressive_decoder.h" + +#include "net/third_party/quiche/src/quic/core/qpack/qpack_decoder_test_utils.h" +#include "net/third_party/quiche/src/quic/core/qpack/qpack_test_utils.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_test.h" +#include "net/third_party/quiche/src/quic/platform/api/quic_text_utils.h" + +using ::testing::Eq; +using ::testing::Return; +using ::testing::StrictMock; + +namespace quic { +namespace test { +namespace { + +const uint64_t kMaximumDynamicTableCapacityForTesting = 1024 * 1024; +const QuicStreamId kStreamId = 0; +// Header Acknowledgement decoder stream instruction with stream_id = 0. +const char* const kHeaderAcknowledgement = "\x80"; + +class MockEnforcer + : public QpackProgressiveDecoder::BlockedStreamLimitEnforcer { + public: + ~MockEnforcer() override = default; + + MOCK_METHOD(bool, OnStreamBlocked, (QuicStreamId stream_id)); + MOCK_METHOD(void, OnStreamUnblocked, (QuicStreamId stream_id)); +}; + +class QpackProgressiveDecoderTest : public QuicTest { + protected: + QpackProgressiveDecoderTest() + : progressive_decoder_(kStreamId, + &enforcer_, + &header_table_, + &decoder_stream_sender_, + &headers_handler_) { + decoder_stream_sender_.set_qpack_stream_sender_delegate( + &decoder_stream_sender_delegate_); + header_table_.SetMaximumDynamicTableCapacity( + kMaximumDynamicTableCapacityForTesting); + } + ~QpackProgressiveDecoderTest() override = default; + + QpackProgressiveDecoder progressive_decoder_; + StrictMock<MockEnforcer> enforcer_; + QpackHeaderTable header_table_; + QpackDecoderStreamSender decoder_stream_sender_; + StrictMock<MockQpackStreamSenderDelegate> decoder_stream_sender_delegate_; + StrictMock<MockHeadersHandler> headers_handler_; +}; + +TEST_F(QpackProgressiveDecoderTest, Literal) { + EXPECT_CALL(headers_handler_, OnHeaderDecoded(Eq("foo"), Eq("bar"))); + progressive_decoder_.Decode(QuicTextUtils::HexDecode("000023666f6f03626172")); + + EXPECT_CALL(headers_handler_, OnDecodingCompleted()); + progressive_decoder_.EndHeaderBlock(); +} + +TEST_F(QpackProgressiveDecoderTest, DynamicTableSynchronous) { + EXPECT_TRUE(header_table_.InsertEntry("foo", "bar")); + + EXPECT_CALL(headers_handler_, OnHeaderDecoded(Eq("foo"), Eq("bar"))); + progressive_decoder_.Decode(QuicTextUtils::HexDecode( + "0200" // Required Insert Count 1 and Delta Base 0. + "80")); // Dynamic table entry with relative index 0, + // absolute index 0. + + EXPECT_CALL(headers_handler_, OnDecodingCompleted()); + EXPECT_CALL(decoder_stream_sender_delegate_, + WriteStreamData(Eq(kHeaderAcknowledgement))); + progressive_decoder_.EndHeaderBlock(); +} + +TEST_F(QpackProgressiveDecoderTest, DynamicTableBlocked) { + EXPECT_CALL(enforcer_, OnStreamBlocked(kStreamId)).WillOnce(Return(true)); + progressive_decoder_.Decode(QuicTextUtils::HexDecode( + "0200" // Required Insert Count 1 and Delta Base 0. + "80")); // Dynamic table entry with relative index 0, + // absolute index 0. + progressive_decoder_.EndHeaderBlock(); + + EXPECT_CALL(enforcer_, OnStreamUnblocked(kStreamId)); + EXPECT_CALL(headers_handler_, OnHeaderDecoded(Eq("foo"), Eq("bar"))); + EXPECT_CALL(headers_handler_, OnDecodingCompleted()); + EXPECT_CALL(decoder_stream_sender_delegate_, + WriteStreamData(Eq(kHeaderAcknowledgement))); + + EXPECT_TRUE(header_table_.InsertEntry("foo", "bar")); +} + +TEST_F(QpackProgressiveDecoderTest, TooManyBlockedStreams) { + EXPECT_CALL(enforcer_, OnStreamBlocked(kStreamId)).WillOnce(Return(false)); + EXPECT_CALL( + headers_handler_, + OnDecodingErrorDetected("Limit on number of blocked streams exceeded.")); + + // Required Insert Count 1. + progressive_decoder_.Decode(QuicTextUtils::HexDecode("0200")); +} + +} // namespace +} // namespace test +} // namespace quic