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 <orphis@webrtc.org>
Reviewed-by: Sergey Sukhanov <sergeysu@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#35013}
diff --git a/net/dcsctp/rx/reassembly_queue.cc b/net/dcsctp/rx/reassembly_queue.cc
index 7183323..36ade92 100644
--- a/net/dcsctp/rx/reassembly_queue.cc
+++ b/net/dcsctp/rx/reassembly_queue.cc
@@ -221,14 +221,21 @@
}
// 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 @@
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 2fa11e3..9cc0c61 100644
--- a/net/dcsctp/rx/reassembly_queue.h
+++ b/net/dcsctp/rx/reassembly_queue.h
@@ -128,6 +128,7 @@
bool IsConsistent() const;
void AddReassembledMessage(rtc::ArrayView<const UnwrappedTSN> tsns,
DcSctpMessage message);
+ void MaybeMoveLastAssembledWatermarkFurther();
struct DeferredResetStreams {
explicit DeferredResetStreams(OutgoingSSNResetRequestParameter req)
