From 161d2c84528ec9eb0c19bfb51024bca54353abc4 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Wed, 13 Dec 2023 12:41:55 +0100 Subject: [PATCH] dcsctp: Fix not using iteraters after NackItem 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 Commit-Queue: Victor Boivie Reviewed-by: Sam Zackrisson Cr-Commit-Position: refs/heads/main@{#41374} --- net/dcsctp/tx/outstanding_data.cc | 16 +++++++++++----- net/dcsctp/tx/outstanding_data.h | 9 +++++---- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/net/dcsctp/tx/outstanding_data.cc b/net/dcsctp/tx/outstanding_data.cc index 4972fc5f10..ca639abc54 100644 --- a/net/dcsctp/tx/outstanding_data.cc +++ b/net/dcsctp/tx/outstanding_data.cc @@ -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 OutstandingData::Insert( void OutstandingData::NackAll() { UnwrappedTSN tsn = last_cumulative_tsn_ack_; + // A two-pass algorithm is needed, as NackItem will invalidate iterators. + std::vector 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()); } diff --git a/net/dcsctp/tx/outstanding_data.h b/net/dcsctp/tx/outstanding_data.h index 82e78337b8..2a214975e6 100644 --- a/net/dcsctp/tx/outstanding_data.h +++ b/net/dcsctp/tx/outstanding_data.h @@ -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