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
