From c09c58134a46038c0dc37b4252a60a2756beb9dc Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Wed, 19 May 2021 14:03:22 +0200 Subject: [PATCH] dcsctp: Limit the size of generated SACK chunks Today, there is no actual limit on how large a SACK chunk can be. And having limits is good to be able to stay within the MTU. This commit adds a limit to the number of reported duplicate TSNs as well as the number of reported gap-ack-blocks in a SACK chunk. These limits are never expected to be reached in a real-life situation. Bug: webrtc:12614 Change-Id: Ib2c143714a214cd3d961e8a52dac26a04b909b80 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219464 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/master@{#34108} --- net/dcsctp/rx/data_tracker.cc | 23 ++++++++++++++++------- net/dcsctp/rx/data_tracker.h | 5 +++++ net/dcsctp/rx/data_tracker_test.cc | 26 ++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 7 deletions(-) diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc index 68a4895ec8..5083e0981b 100644 --- a/net/dcsctp/rx/data_tracker.cc +++ b/net/dcsctp/rx/data_tracker.cc @@ -26,6 +26,9 @@ namespace dcsctp { +constexpr size_t DataTracker::kMaxDuplicateTsnReported; +constexpr size_t DataTracker::kMaxGapAckBlocksReported; + bool DataTracker::IsTSNValid(TSN tsn) const { UnwrappedTSN unwrapped_tsn = tsn_unwrapper_.PeekUnwrap(tsn); @@ -52,7 +55,9 @@ void DataTracker::Observe(TSN tsn, // Old chunk already seen before? if (unwrapped_tsn <= last_cumulative_acked_tsn_) { - duplicate_tsns_.insert(unwrapped_tsn.Wrap()); + if (duplicate_tsns_.size() < kMaxDuplicateTsnReported) { + duplicate_tsns_.insert(unwrapped_tsn.Wrap()); + } return; } @@ -69,7 +74,9 @@ void DataTracker::Observe(TSN tsn, bool inserted = additional_tsns_.insert(unwrapped_tsn).second; if (!inserted) { // Already seen before. - duplicate_tsns_.insert(unwrapped_tsn.Wrap()); + if (duplicate_tsns_.size() < kMaxDuplicateTsnReported) { + duplicate_tsns_.insert(unwrapped_tsn.Wrap()); + } } } @@ -200,12 +207,14 @@ std::vector DataTracker::CreateGapAckBlocks() const { auto flush = [&]() { if (first_tsn_in_block.has_value()) { - auto start_diff = UnwrappedTSN::Difference(*first_tsn_in_block, + if (gap_ack_blocks.size() < kMaxGapAckBlocksReported) { + auto start_diff = UnwrappedTSN::Difference(*first_tsn_in_block, + last_cumulative_acked_tsn_); + auto end_diff = UnwrappedTSN::Difference(*last_tsn_in_block, last_cumulative_acked_tsn_); - auto end_diff = UnwrappedTSN::Difference(*last_tsn_in_block, - last_cumulative_acked_tsn_); - gap_ack_blocks.emplace_back(static_cast(start_diff), - static_cast(end_diff)); + gap_ack_blocks.emplace_back(static_cast(start_diff), + static_cast(end_diff)); + } first_tsn_in_block = absl::nullopt; last_tsn_in_block = absl::nullopt; } diff --git a/net/dcsctp/rx/data_tracker.h b/net/dcsctp/rx/data_tracker.h index f5deaf147d..63b7d8fe36 100644 --- a/net/dcsctp/rx/data_tracker.h +++ b/net/dcsctp/rx/data_tracker.h @@ -38,6 +38,11 @@ namespace dcsctp { // 200ms, whatever is smallest). class DataTracker { public: + // The maximum number of duplicate TSNs that will be reported in a SACK. + static constexpr size_t kMaxDuplicateTsnReported = 20; + // The maximum number of gap-ack-blocks that will be reported in a SACK. + static constexpr size_t kMaxGapAckBlocksReported = 20; + // The maximum number of accepted in-flight DATA chunks. This indicates the // maximum difference from this buffer's last cumulative ack TSN, and any // received data. Data received beyond this limit will be dropped, which will diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc index 7518d6dc91..a19ac8ba4c 100644 --- a/net/dcsctp/rx/data_tracker_test.cc +++ b/net/dcsctp/rx/data_tracker_test.cc @@ -25,6 +25,7 @@ namespace dcsctp { namespace { using ::testing::ElementsAre; using ::testing::IsEmpty; +using ::testing::SizeIs; using ::testing::UnorderedElementsAre; constexpr size_t kArwnd = 10000; @@ -269,5 +270,30 @@ TEST_F(DataTrackerTest, ClearsDuplicateTsnsAfterCreatingSack) { EXPECT_THAT(sack2.duplicate_tsns(), IsEmpty()); } +TEST_F(DataTrackerTest, LimitsNumberOfDuplicatesReported) { + for (size_t i = 0; i < DataTracker::kMaxDuplicateTsnReported + 10; ++i) { + TSN tsn(11 + i); + buf_.Observe(tsn, AnyDataChunk::ImmediateAckFlag(false)); + buf_.Observe(tsn, AnyDataChunk::ImmediateAckFlag(false)); + } + + SackChunk sack = buf_.CreateSelectiveAck(kArwnd); + EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty()); + EXPECT_THAT(sack.duplicate_tsns(), + SizeIs(DataTracker::kMaxDuplicateTsnReported)); +} + +TEST_F(DataTrackerTest, LimitsNumberOfGapAckBlocksReported) { + for (size_t i = 0; i < DataTracker::kMaxGapAckBlocksReported + 10; ++i) { + TSN tsn(11 + i * 2); + buf_.Observe(tsn, AnyDataChunk::ImmediateAckFlag(false)); + } + + SackChunk sack = buf_.CreateSelectiveAck(kArwnd); + EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(11)); + EXPECT_THAT(sack.gap_ack_blocks(), + SizeIs(DataTracker::kMaxGapAckBlocksReported)); +} + } // namespace } // namespace dcsctp