From c54f6722cefb75fdb7be6948d3326a2ad22b23da Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Tue, 13 Apr 2021 11:23:16 +0200 Subject: [PATCH] dcsctp: Fix post-review comments for DataTracker These are some fixes that were added after submission of https://webrtc-review.googlesource.com/c/src/+/213664 Mainly: * Don't accept TSNs that have a too large difference from expected * Renaming of member variable (to confirm to style guidelines) Bug: webrtc:12614 Change-Id: I06e11ab2acf5d307b68c3cbc135fde2c038ee690 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/215070 Commit-Queue: Victor Boivie Reviewed-by: Tommi Cr-Commit-Position: refs/heads/master@{#33721} --- net/dcsctp/rx/data_tracker.cc | 53 +++++++++++++++++++++--------- net/dcsctp/rx/data_tracker.h | 14 +++++++- net/dcsctp/rx/data_tracker_test.cc | 27 ++++++++++++++- 3 files changed, 76 insertions(+), 18 deletions(-) diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc index b95cb44230..3e03dfece2 100644 --- a/net/dcsctp/rx/data_tracker.cc +++ b/net/dcsctp/rx/data_tracker.cc @@ -26,9 +26,30 @@ namespace dcsctp { +bool DataTracker::IsTSNValid(TSN tsn) const { + UnwrappedTSN unwrapped_tsn = tsn_unwrapper_.PeekUnwrap(tsn); + + // Note that this method doesn't return `false` for old DATA chunks, as those + // are actually valid, and receiving those may affect the generated SACK + // response (by setting "duplicate TSNs"). + + uint32_t difference = + UnwrappedTSN::Difference(unwrapped_tsn, last_cumulative_acked_tsn_); + if (difference > kMaxAcceptedOutstandingFragments) { + return false; + } + return true; +} + void DataTracker::Observe(TSN tsn, AnyDataChunk::ImmediateAckFlag immediate_ack) { UnwrappedTSN unwrapped_tsn = tsn_unwrapper_.Unwrap(tsn); + + // IsTSNValid must be called prior to calling this method. + RTC_DCHECK( + UnwrappedTSN::Difference(unwrapped_tsn, last_cumulative_acked_tsn_) <= + kMaxAcceptedOutstandingFragments); + // Old chunk already seen before? if (unwrapped_tsn <= last_cumulative_acked_tsn_) { // TODO(boivie) Set duplicate TSN, even if it's not used in SCTP yet. @@ -38,14 +59,14 @@ void DataTracker::Observe(TSN tsn, if (unwrapped_tsn == last_cumulative_acked_tsn_.next_value()) { last_cumulative_acked_tsn_ = unwrapped_tsn; // The cumulative acked tsn may be moved even further, if a gap was filled. - while (!additional_tsns.empty() && - *additional_tsns.begin() == + while (!additional_tsns_.empty() && + *additional_tsns_.begin() == last_cumulative_acked_tsn_.next_value()) { last_cumulative_acked_tsn_.Increment(); - additional_tsns.erase(additional_tsns.begin()); + additional_tsns_.erase(additional_tsns_.begin()); } } else { - additional_tsns.insert(unwrapped_tsn); + additional_tsns_.insert(unwrapped_tsn); } // https://tools.ietf.org/html/rfc4960#section-6.7 @@ -54,7 +75,7 @@ void DataTracker::Observe(TSN tsn, // the received DATA chunk sequence, it SHOULD send a SACK with Gap Ack // Blocks immediately. The data receiver continues sending a SACK after // receipt of each SCTP packet that doesn't fill the gap." - if (!additional_tsns.empty()) { + if (!additional_tsns_.empty()) { UpdateAckState(AckState::kImmediate, "packet loss"); } @@ -119,15 +140,15 @@ void DataTracker::HandleForwardTsn(TSN new_cumulative_ack) { // now overlapping with the new value, remove them. last_cumulative_acked_tsn_ = unwrapped_tsn; int erased_additional_tsns = std::distance( - additional_tsns.begin(), additional_tsns.upper_bound(unwrapped_tsn)); - additional_tsns.erase(additional_tsns.begin(), - additional_tsns.upper_bound(unwrapped_tsn)); + additional_tsns_.begin(), additional_tsns_.upper_bound(unwrapped_tsn)); + additional_tsns_.erase(additional_tsns_.begin(), + additional_tsns_.upper_bound(unwrapped_tsn)); // See if the `last_cumulative_acked_tsn_` can be moved even further: - while (!additional_tsns.empty() && - *additional_tsns.begin() == last_cumulative_acked_tsn_.next_value()) { + while (!additional_tsns_.empty() && + *additional_tsns_.begin() == last_cumulative_acked_tsn_.next_value()) { last_cumulative_acked_tsn_.Increment(); - additional_tsns.erase(additional_tsns.begin()); + additional_tsns_.erase(additional_tsns_.begin()); ++erased_additional_tsns; } @@ -179,17 +200,17 @@ std::vector DataTracker::CreateGapAckBlocks() const { auto flush = [&]() { if (first_tsn_in_block.has_value()) { - int start_diff = UnwrappedTSN::Difference(*first_tsn_in_block, - last_cumulative_acked_tsn_); - int end_diff = UnwrappedTSN::Difference(*last_tsn_in_block, - last_cumulative_acked_tsn_); + 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_); 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; } }; - for (UnwrappedTSN tsn : additional_tsns) { + for (UnwrappedTSN tsn : additional_tsns_) { if (last_tsn_in_block.has_value() && last_tsn_in_block->next_value() == tsn) { // Continuing the same block. diff --git a/net/dcsctp/rx/data_tracker.h b/net/dcsctp/rx/data_tracker.h index a528967f6e..6146d2a839 100644 --- a/net/dcsctp/rx/data_tracker.h +++ b/net/dcsctp/rx/data_tracker.h @@ -38,6 +38,13 @@ namespace dcsctp { // 200ms, whatever is smallest). class DataTracker { public: + // 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 + // force the transmitter to send data that actually increases the last + // cumulative acked TSN. + static constexpr uint32_t kMaxAcceptedOutstandingFragments = 256; + explicit DataTracker(absl::string_view log_prefix, Timer* delayed_ack_timer, TSN peer_initial_tsn) @@ -46,6 +53,11 @@ class DataTracker { last_cumulative_acked_tsn_( tsn_unwrapper_.Unwrap(TSN(*peer_initial_tsn - 1))) {} + // Indicates if the provided TSN is valid. If this return false, the data + // should be dropped and not added to any other buffers, which essentially + // means that there is intentional packet loss. + bool IsTSNValid(TSN tsn) const; + // Call for every incoming data chunk. void Observe(TSN tsn, AnyDataChunk::ImmediateAckFlag immediate_ack = @@ -113,7 +125,7 @@ class DataTracker { // All TSNs up until (and including) this value have been seen. UnwrappedTSN last_cumulative_acked_tsn_; // Received TSNs that are not directly following `last_cumulative_acked_tsn_`. - std::set additional_tsns; + std::set additional_tsns_; std::set duplicates_; }; } // namespace dcsctp diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc index 1bad807132..d714b0ba9e 100644 --- a/net/dcsctp/rx/data_tracker_test.cc +++ b/net/dcsctp/rx/data_tracker_test.cc @@ -27,6 +27,7 @@ using ::testing::ElementsAre; using ::testing::IsEmpty; constexpr size_t kArwnd = 10000; +constexpr TSN kInitialTSN(11); class DataTrackerTest : public testing::Test { protected: @@ -37,7 +38,7 @@ class DataTrackerTest : public testing::Test { "test/delayed_ack", []() { return absl::nullopt; }, TimerOptions(DurationMs(0)))), - buf_("log: ", timer_.get(), /*peer_initial_tsn=*/TSN(11)) {} + buf_("log: ", timer_.get(), kInitialTSN) {} void Observer(std::initializer_list tsns) { for (const uint32_t tsn : tsns) { @@ -205,5 +206,29 @@ TEST_F(DataTrackerTest, ForceShouldSendSackImmediately) { EXPECT_TRUE(buf_.ShouldSendAck()); } + +TEST_F(DataTrackerTest, WillAcceptValidTSNs) { + // The initial TSN is always one more than the last, which is our base. + TSN last_tsn = TSN(*kInitialTSN - 1); + int limit = static_cast(DataTracker::kMaxAcceptedOutstandingFragments); + + for (int i = -limit; i <= limit; ++i) { + EXPECT_TRUE(buf_.IsTSNValid(TSN(*last_tsn + i))); + } +} + +TEST_F(DataTrackerTest, WillNotAcceptInvalidTSNs) { + // The initial TSN is always one more than the last, which is our base. + TSN last_tsn = TSN(*kInitialTSN - 1); + + size_t limit = DataTracker::kMaxAcceptedOutstandingFragments; + EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn + limit + 1))); + EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - (limit + 1)))); + EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn + 65536))); + EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - 65536))); + EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn + 0x8000000))); + EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - 0x8000000))); +} + } // namespace } // namespace dcsctp