From fed091edf49c4bc8e4dd04855571f1ccca0d8220 Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Thu, 16 Sep 2021 12:58:20 +0200 Subject: [PATCH] dcsctp: Move last_assembled_tsn_watermark further The ReassemblyQueue will need to track which messages that have already been delivered to the client so that they are not re-delivered on e.g. retransmissions. It does that by tracking which TSNs that those messages were built from. It tracks that in two variables, `last_assembled_tsn_watermark` and `delivered_tsns_`, where the first one represent that all TSNs including and prior this one have been delivered and `delivered_tsns` contain additional ones when there are gaps. When receiving a FORWARD-TSN and asked to forget about some partially received messages, these two variables were updated correctly, but the `delivered_tsns_` were left in a state where it could be adjacent to the `last_assembled_tsn_watermark` - when `last_assembled_tsn_watermark` could actually have been moved further. Added consistency check (that would trigger in existing tests) and fixing the issue. This bug is quite benign, as any received chunk would've corrected the problem, and even at this faulty state, the ReassemblyQueue would function completely fine. Bug: webrtc:13154 Change-Id: Iaa7c612999c9dc609fc6e2fb3be2d0bd04534c90 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/232124 Reviewed-by: Florent Castelli Reviewed-by: Sergey Sukhanov Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#35013} --- net/dcsctp/rx/reassembly_queue.cc | 20 ++++++++++++++++++-- net/dcsctp/rx/reassembly_queue.h | 1 + 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/net/dcsctp/rx/reassembly_queue.cc b/net/dcsctp/rx/reassembly_queue.cc index 7183323642..36ade9230a 100644 --- a/net/dcsctp/rx/reassembly_queue.cc +++ b/net/dcsctp/rx/reassembly_queue.cc @@ -221,14 +221,21 @@ void ReassemblyQueue::AddReassembledMessage( } // With new TSNs in delivered_tsns, gaps might be filled. + MaybeMoveLastAssembledWatermarkFurther(); + + reassembled_messages_.emplace_back(std::move(message)); +} + +void ReassemblyQueue::MaybeMoveLastAssembledWatermarkFurther() { + // `delivered_tsns_` contain TSNS when there is a gap between ranges of + // assembled TSNs. `last_assembled_tsn_watermark_` should not be adjacent to + // that list, because if so, it can be moved. while (!delivered_tsns_.empty() && *delivered_tsns_.begin() == last_assembled_tsn_watermark_.next_value()) { last_assembled_tsn_watermark_.Increment(); delivered_tsns_.erase(delivered_tsns_.begin()); } - - reassembled_messages_.emplace_back(std::move(message)); } void ReassemblyQueue::Handle(const AnyForwardTsnChunk& forward_tsn) { @@ -239,12 +246,21 @@ void ReassemblyQueue::Handle(const AnyForwardTsnChunk& forward_tsn) { delivered_tsns_.erase(delivered_tsns_.begin(), delivered_tsns_.upper_bound(tsn)); + MaybeMoveLastAssembledWatermarkFurther(); + queued_bytes_ -= streams_->HandleForwardTsn(tsn, forward_tsn.skipped_streams()); RTC_DCHECK(IsConsistent()); } bool ReassemblyQueue::IsConsistent() const { + // `delivered_tsns_` and `last_assembled_tsn_watermark_` mustn't overlap or be + // adjacent. + if (!delivered_tsns_.empty() && + last_assembled_tsn_watermark_.next_value() >= *delivered_tsns_.begin()) { + return false; + } + // Allow queued_bytes_ to be larger than max_size_bytes, as it's not actively // enforced in this class. This comparison will still trigger if queued_bytes_ // became "negative". diff --git a/net/dcsctp/rx/reassembly_queue.h b/net/dcsctp/rx/reassembly_queue.h index 2fa11e3f75..9cc0c61eb6 100644 --- a/net/dcsctp/rx/reassembly_queue.h +++ b/net/dcsctp/rx/reassembly_queue.h @@ -128,6 +128,7 @@ class ReassemblyQueue { bool IsConsistent() const; void AddReassembledMessage(rtc::ArrayView tsns, DcSctpMessage message); + void MaybeMoveLastAssembledWatermarkFurther(); struct DeferredResetStreams { explicit DeferredResetStreams(OutgoingSSNResetRequestParameter req)