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 <boivie@webrtc.org>
Reviewed-by: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33721}
diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc
index b95cb44..3e03dfe 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 @@
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 @@
// 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 @@
// 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 @@
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<uint16_t>(start_diff),
static_cast<uint16_t>(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 a528967..6146d2a 100644
--- a/net/dcsctp/rx/data_tracker.h
+++ b/net/dcsctp/rx/data_tracker.h
@@ -38,6 +38,13 @@
// 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 @@
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 @@
// 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<UnwrappedTSN> additional_tsns;
+ std::set<UnwrappedTSN> additional_tsns_;
std::set<UnwrappedTSN> duplicates_;
};
} // namespace dcsctp
diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc
index 1bad807..d714b0b 100644
--- a/net/dcsctp/rx/data_tracker_test.cc
+++ b/net/dcsctp/rx/data_tracker_test.cc
@@ -27,6 +27,7 @@
using ::testing::IsEmpty;
constexpr size_t kArwnd = 10000;
+constexpr TSN kInitialTSN(11);
class DataTrackerTest : public testing::Test {
protected:
@@ -37,7 +38,7 @@
"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<uint32_t> tsns) {
for (const uint32_t tsn : tsns) {
@@ -205,5 +206,29 @@
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<int>(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
