admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

dcsctp: Fix not using iteraters after NackItem

Browse Source 
OutstandingData::NackItem nacks a chunk, and if that chunk reaches its
partial reliability critera, it will abandon the entire message. If that
message hasn't been sent in full, a placeholder "end" message is
inserted (see https://crbug.com/webrtc/12812). And when the message is
inserted, any iterators may be invalidated (if e.g. std::deque would
want to grow the deque).

So ensure that there are no iterators used after having called NackItem.
By changing the interface of NackItem, and not passing an Item, but just
the TSN, this is encouraged. NackAll was rewritten as a two-pass
algorithm to first collect TSNs, then iterating that list, looking up
the items in the second pass (constant complexity).

Bug: chromium:1510364
Change-Id: I5156b6d6a683184f290e71c98f16bc68ea2a562f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/331320
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Sam Zackrisson <saza@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41374}
This commit is contained in:
Victor Boivie 2023-12-13 12:41:55 +01:00 committed by WebRTC LUCI CQ
parent d76e0898a9
commit 161d2c8452
2 changed files with 16 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
net/dcsctp/tx/outstanding_data.cc
View File

@ -240,9 +240,8 @@ void OutstandingData::NackBetweenAckBlocks(
for (UnwrappedTSN tsn = prev_block_last_acked.next_value();
tsn < cur_block_first_acked && tsn <= max_tsn_to_nack;
tsn = tsn.next_value()) {
Item& item = GetItem(tsn);
ack_info.has_packet_loss |=
NackItem(tsn, item, /*retransmit_now=*/false,
NackItem(tsn, /*retransmit_now=*/false,
/*do_fast_retransmit=*/!is_in_fast_recovery);
}
prev_block_last_acked = UnwrappedTSN::AddTo(cumulative_tsn_ack, block.end);
@ -255,9 +254,9 @@ void OutstandingData::NackBetweenAckBlocks(
}
bool OutstandingData::NackItem(UnwrappedTSN tsn,
Item& item,
bool retransmit_now,
bool do_fast_retransmit) {
Item& item = GetItem(tsn);
if (item.is_outstanding()) {
unacked_bytes_ -= GetSerializedChunkSize(item.data());
--unacked_items_;
@ -446,13 +445,20 @@ absl::optional<UnwrappedTSN> OutstandingData::Insert(
void OutstandingData::NackAll() {
UnwrappedTSN tsn = last_cumulative_tsn_ack_;
// A two-pass algorithm is needed, as NackItem will invalidate iterators.
std::vector<UnwrappedTSN> tsns_to_nack;
for (Item& item : outstanding_data_) {
tsn.Increment();
if (!item.is_acked()) {
NackItem(tsn, item, /*retransmit_now=*/true,
/*do_fast_retransmit=*/false);
tsns_to_nack.push_back(tsn);
}
}
for (UnwrappedTSN tsn : tsns_to_nack) {
NackItem(tsn, /*retransmit_now=*/true,
/*do_fast_retransmit=*/false);
}
RTC_DCHECK(IsConsistent());
}

9
net/dcsctp/tx/outstanding_data.h
View File

@ -330,10 +330,11 @@ class OutstandingData {
// many times so that it should be retransmitted, this will schedule it to be
// "fast retransmitted". This is only done just before going into fast
// recovery.
bool NackItem(UnwrappedTSN tsn,
Item& item,
bool retransmit_now,
bool do_fast_retransmit);
//
// Note that since nacking an item may result in it becoming abandoned, which
// in turn could alter `outstanding_data_`, any iterators are invalidated
// after having called this method.
bool NackItem(UnwrappedTSN tsn, bool retransmit_now, bool do_fast_retransmit);
// Given that a message fragment, `item` has been abandoned, abandon all other
// fragments that share the same message - both never-before-sent fragments