Signal error if Insert Count Increment causes overflow. gfe-relnote: n/a, change to QUIC v99-only code. Protected by existing disabled gfe2_reloadable_flag_quic_enable_version_99. PiperOrigin-RevId: 274830568 Change-Id: Ic9e7fa84e04808d2c7bade54ed3670b53b5bb0f6
diff --git a/quic/core/qpack/qpack_blocking_manager.cc b/quic/core/qpack/qpack_blocking_manager.cc index 2243c90..ffec172 100644 --- a/quic/core/qpack/qpack_blocking_manager.cc +++ b/quic/core/qpack/qpack_blocking_manager.cc
@@ -4,6 +4,7 @@ #include "net/third_party/quiche/src/quic/core/qpack/qpack_blocking_manager.h" +#include <limits> #include <utility> namespace quic { @@ -49,8 +50,14 @@ header_blocks_.erase(it); } -void QpackBlockingManager::OnInsertCountIncrement(uint64_t increment) { +bool QpackBlockingManager::OnInsertCountIncrement(uint64_t increment) { + if (increment > + std::numeric_limits<uint64_t>::max() - known_received_count_) { + return false; + } + IncreaseKnownReceivedCountTo(known_received_count_ + increment); + return true; } void QpackBlockingManager::OnHeaderBlockSent(QuicStreamId stream_id,
diff --git a/quic/core/qpack/qpack_blocking_manager.h b/quic/core/qpack/qpack_blocking_manager.h index 6bebf5f..60f7db7 100644 --- a/quic/core/qpack/qpack_blocking_manager.h +++ b/quic/core/qpack/qpack_blocking_manager.h
@@ -40,8 +40,9 @@ void OnStreamCancellation(QuicStreamId stream_id); // Called when an Insert Count Increment instruction is received on the - // decoder stream. - void OnInsertCountIncrement(uint64_t increment); + // decoder stream. Returns true if Known Received Count is successfully + // updated. Returns false on overflow. + bool OnInsertCountIncrement(uint64_t increment); // Called when sending a header block containing references to dynamic table // entries with |indices|. |indices| must not be empty.
diff --git a/quic/core/qpack/qpack_blocking_manager_test.cc b/quic/core/qpack/qpack_blocking_manager_test.cc index 8257363..64bfb97 100644 --- a/quic/core/qpack/qpack_blocking_manager_test.cc +++ b/quic/core/qpack/qpack_blocking_manager_test.cc
@@ -4,6 +4,8 @@ #include "net/third_party/quiche/src/quic/core/qpack/qpack_blocking_manager.h" +#include <limits> + #include "net/third_party/quiche/src/quic/platform/api/quic_test.h" namespace quic { @@ -53,7 +55,7 @@ } TEST_F(QpackBlockingManagerTest, NotBlockedByInsertCountIncrement) { - manager_.OnInsertCountIncrement(2); + EXPECT_TRUE(manager_.OnInsertCountIncrement(2)); // Stream 0 is not blocked, because it only references entries that are // already acknowledged by an Insert Count Increment instruction. @@ -65,7 +67,7 @@ manager_.OnHeaderBlockSent(0, {1, 0}); EXPECT_TRUE(stream_is_blocked(0)); - manager_.OnInsertCountIncrement(2); + EXPECT_TRUE(manager_.OnInsertCountIncrement(2)); EXPECT_FALSE(stream_is_blocked(0)); } @@ -114,7 +116,7 @@ EXPECT_EQ(2u, manager_.known_received_count()); // Insert Count Increment increases Known Received Count. - manager_.OnInsertCountIncrement(2); + EXPECT_TRUE(manager_.OnInsertCountIncrement(2)); EXPECT_EQ(4u, manager_.known_received_count()); EXPECT_TRUE(manager_.OnHeaderAcknowledgement(2)); @@ -161,7 +163,7 @@ EXPECT_EQ(1u, manager_.smallest_blocking_index()); // Insert Count Increment does not change smallest blocking index. - manager_.OnInsertCountIncrement(2); + EXPECT_TRUE(manager_.OnInsertCountIncrement(2)); EXPECT_EQ(1u, manager_.smallest_blocking_index()); manager_.OnStreamCancellation(1); @@ -249,7 +251,7 @@ EXPECT_EQ(0u, manager_.smallest_blocking_index()); // Acknowledging entry 1 still leaves one unacknowledged reference to entry 0. - manager_.OnInsertCountIncrement(2); + EXPECT_TRUE(manager_.OnInsertCountIncrement(2)); EXPECT_EQ(2u, manager_.known_received_count()); EXPECT_EQ(0u, manager_.smallest_blocking_index()); @@ -261,13 +263,13 @@ EXPECT_EQ(0u, manager_.smallest_blocking_index()); // Acknowledging entry 2 removes last reference to entry 0. - manager_.OnInsertCountIncrement(1); + EXPECT_TRUE(manager_.OnInsertCountIncrement(1)); EXPECT_EQ(3u, manager_.known_received_count()); EXPECT_EQ(2u, manager_.smallest_blocking_index()); // Acknowledging entry 4 (and implicitly 3) removes reference to entry 2. - manager_.OnInsertCountIncrement(2); + EXPECT_TRUE(manager_.OnInsertCountIncrement(2)); EXPECT_EQ(5u, manager_.known_received_count()); EXPECT_EQ(std::numeric_limits<uint64_t>::max(), @@ -386,6 +388,14 @@ EXPECT_TRUE(manager_.blocking_allowed_on_stream(kStreamId2, 1)); } +TEST_F(QpackBlockingManagerTest, InsertCountIncrementOverflow) { + EXPECT_TRUE(manager_.OnInsertCountIncrement(10)); + EXPECT_EQ(10u, manager_.known_received_count()); + + EXPECT_FALSE(manager_.OnInsertCountIncrement( + std::numeric_limits<uint64_t>::max() - 5)); +} + } // namespace } // namespace test } // namespace quic
diff --git a/quic/core/qpack/qpack_encoder.cc b/quic/core/qpack/qpack_encoder.cc index 418af8c..319af82 100644 --- a/quic/core/qpack/qpack_encoder.cc +++ b/quic/core/qpack/qpack_encoder.cc
@@ -406,7 +406,10 @@ return; } - blocking_manager_.OnInsertCountIncrement(increment); + if (!blocking_manager_.OnInsertCountIncrement(increment)) { + decoder_stream_error_delegate_->OnDecoderStreamError( + "Insert Count Increment instruction causes overflow."); + } if (blocking_manager_.known_received_count() > header_table_.inserted_entry_count()) {
diff --git a/quic/core/qpack/qpack_encoder_test.cc b/quic/core/qpack/qpack_encoder_test.cc index 3bc5859..6f2efe3 100644 --- a/quic/core/qpack/qpack_encoder_test.cc +++ b/quic/core/qpack/qpack_encoder_test.cc
@@ -4,6 +4,7 @@ #include "net/third_party/quiche/src/quic/core/qpack/qpack_encoder.h" +#include <limits> #include <string> #include "net/third_party/quiche/src/quic/core/qpack/qpack_encoder_test_utils.h" @@ -176,6 +177,27 @@ encoder_.OnInsertCountIncrement(1); } +// Regression test for https://crbug.com/1014372. +TEST_F(QpackEncoderTest, InsertCountIncrementOverflow) { + QpackHeaderTable* header_table = QpackEncoderPeer::header_table(&encoder_); + + // Set dynamic table capacity large enough to hold one entry. + header_table->SetMaximumDynamicTableCapacity(4096); + header_table->SetDynamicTableCapacity(4096); + // Insert one entry into the header table. + header_table->InsertEntry("foo", "bar"); + + // Receive Insert Count Increment instruction with increment value 1. + encoder_.OnInsertCountIncrement(1); + + // Receive Insert Count Increment instruction that overflows the known + // received count. This must result in an error instead of a crash. + EXPECT_CALL(decoder_stream_error_delegate_, + OnDecoderStreamError( + Eq("Insert Count Increment instruction causes overflow."))); + encoder_.OnInsertCountIncrement(std::numeric_limits<uint64_t>::max()); +} + TEST_F(QpackEncoderTest, InvalidHeaderAcknowledgement) { // Encoder receives header acknowledgement for a stream on which no header // block with dynamic table entries was ever sent.