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

dcsctp: Move last_assembled_tsn_watermark further

Browse Source 
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}
This commit is contained in:
Victor Boivie 2021-09-16 12:58:20 +02:00 committed by WebRTC LUCI CQ
parent 3b08fe3dcd
commit fed091edf4
2 changed files with 19 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

20
net/dcsctp/rx/reassembly_queue.cc
View File

@ -221,14 +221,21 @@ void ReassemblyQueue::AddReassembledMessage(
} }
// With new TSNs in delivered_tsns, gaps might be filled. // 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() && while (!delivered_tsns_.empty() &&
*delivered_tsns_.begin() == *delivered_tsns_.begin() ==
last_assembled_tsn_watermark_.next_value()) { last_assembled_tsn_watermark_.next_value()) {
last_assembled_tsn_watermark_.Increment(); last_assembled_tsn_watermark_.Increment();
delivered_tsns_.erase(delivered_tsns_.begin()); delivered_tsns_.erase(delivered_tsns_.begin());
} }
reassembled_messages_.emplace_back(std::move(message));
} }
void ReassemblyQueue::Handle(const AnyForwardTsnChunk& forward_tsn) { 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_.erase(delivered_tsns_.begin(),
delivered_tsns_.upper_bound(tsn)); delivered_tsns_.upper_bound(tsn));
MaybeMoveLastAssembledWatermarkFurther();
queued_bytes_ -= queued_bytes_ -=
streams_->HandleForwardTsn(tsn, forward_tsn.skipped_streams()); streams_->HandleForwardTsn(tsn, forward_tsn.skipped_streams());
RTC_DCHECK(IsConsistent()); RTC_DCHECK(IsConsistent());
} }
bool ReassemblyQueue::IsConsistent() const { 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 // 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_ // enforced in this class. This comparison will still trigger if queued_bytes_
// became "negative". // became "negative".

1
net/dcsctp/rx/reassembly_queue.h
View File

@ -128,6 +128,7 @@ class ReassemblyQueue {
bool IsConsistent() const; bool IsConsistent() const;
void AddReassembledMessage(rtc::ArrayView<const UnwrappedTSN> tsns, void AddReassembledMessage(rtc::ArrayView<const UnwrappedTSN> tsns,
DcSctpMessage message); DcSctpMessage message);
void MaybeMoveLastAssembledWatermarkFurther();
struct DeferredResetStreams { struct DeferredResetStreams {
explicit DeferredResetStreams(OutgoingSSNResetRequestParameter req) explicit DeferredResetStreams(OutgoingSSNResetRequestParameter req)