From a04b8b504393d7d36ae1e173dcb223371c2e197b Mon Sep 17 00:00:00 2001 From: Victor Boivie Date: Tue, 8 Mar 2022 09:36:55 +0100 Subject: [PATCH] dcsctp: Handle losing first DATA on ordered stream When a FORWARD-TSN is received as the first chunk on an ordered stream, it will fail to set the new "next expected SSN" that is present in the FORWARD-TSN as that stream hasn't been allocated yet. It's allocated when the first DATA is received on that stream. This is a non-issue for ordinary data channels as the first message on any stream will be the "Data Channel Establishment Protocol" messages, which are always sent reliably. But if prenegotiated channels are used, and the very first packet received on an ordered data channel is lost _and_ signaled to the receiver as lost _before_ the receiver has received any other fragments on that data channel, future messages will not be delivered on that channel. Bug: webrtc:13799 Change-Id: Ide5c656243b3a51a2ed9d76615cfc3631cfe900c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/253902 Reviewed-by: Harald Alvestrand Commit-Queue: Victor Boivie Cr-Commit-Position: refs/heads/main@{#36155} --- .../rx/traditional_reassembly_streams.cc | 11 +++--- .../rx/traditional_reassembly_streams_test.cc | 21 +++++++++++ net/dcsctp/socket/dcsctp_socket_test.cc | 35 +++++++++++++++++++ 3 files changed, 61 insertions(+), 6 deletions(-) diff --git a/net/dcsctp/rx/traditional_reassembly_streams.cc b/net/dcsctp/rx/traditional_reassembly_streams.cc index 4ccc2e5278..f5dc8cacc8 100644 --- a/net/dcsctp/rx/traditional_reassembly_streams.cc +++ b/net/dcsctp/rx/traditional_reassembly_streams.cc @@ -261,11 +261,11 @@ size_t TraditionalReassemblyStreams::OrderedStream::EraseTo(SSN ssn) { int TraditionalReassemblyStreams::Add(UnwrappedTSN tsn, Data data) { if (data.is_unordered) { - auto it = unordered_streams_.emplace(data.stream_id, this).first; + auto it = unordered_streams_.try_emplace(data.stream_id, this).first; return it->second.Add(tsn, std::move(data)); } - auto it = ordered_streams_.emplace(data.stream_id, this).first; + auto it = ordered_streams_.try_emplace(data.stream_id, this).first; return it->second.Add(tsn, std::move(data)); } @@ -280,10 +280,9 @@ size_t TraditionalReassemblyStreams::HandleForwardTsn( } for (const auto& skipped_stream : skipped_streams) { - auto it = ordered_streams_.find(skipped_stream.stream_id); - if (it != ordered_streams_.end()) { - bytes_removed += it->second.EraseTo(skipped_stream.ssn); - } + auto it = + ordered_streams_.try_emplace(skipped_stream.stream_id, this).first; + bytes_removed += it->second.EraseTo(skipped_stream.ssn); } return bytes_removed; diff --git a/net/dcsctp/rx/traditional_reassembly_streams_test.cc b/net/dcsctp/rx/traditional_reassembly_streams_test.cc index 3e6f560aba..759962473d 100644 --- a/net/dcsctp/rx/traditional_reassembly_streams_test.cc +++ b/net/dcsctp/rx/traditional_reassembly_streams_test.cc @@ -25,8 +25,10 @@ namespace dcsctp { namespace { +using ::testing::ElementsAre; using ::testing::MockFunction; using ::testing::NiceMock; +using ::testing::Property; class TraditionalReassemblyStreamsTest : public testing::Test { protected: @@ -232,5 +234,24 @@ TEST_F(TraditionalReassemblyStreamsTest, EXPECT_EQ(streams2.Add(tsn(4), gen_.Unordered({7})), 1); } +TEST_F(TraditionalReassemblyStreamsTest, CanDeleteFirstOrderedMessage) { + NiceMock> on_assembled; + EXPECT_CALL(on_assembled, + Call(ElementsAre(tsn(2)), + Property(&DcSctpMessage::payload, ElementsAre(2, 3, 4)))); + + TraditionalReassemblyStreams streams("", on_assembled.AsStdFunction()); + + // Not received, SID=1. TSN=1, SSN=0 + gen_.Ordered({1}, "BE"); + // And deleted (SID=1, TSN=1, SSN=0) + ForwardTsnChunk::SkippedStream skipped[] = { + ForwardTsnChunk::SkippedStream(StreamID(1), SSN(0))}; + EXPECT_EQ(streams.HandleForwardTsn(tsn(1), skipped), 0u); + + // Receive SID=1, TSN=2, SSN=1 + EXPECT_EQ(streams.Add(tsn(2), gen_.Ordered({2, 3, 4}, "BE")), 0); +} + } // namespace } // namespace dcsctp diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index f45773baba..d30043d332 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -2179,5 +2179,40 @@ TEST(DcSctpSocketTest, BothCanDetectDcsctpImplementation) { EXPECT_EQ(a.socket.peer_implementation(), SctpImplementation::kDcsctp); EXPECT_EQ(z.socket.peer_implementation(), SctpImplementation::kDcsctp); } + +TEST_P(DcSctpSocketParametrizedTest, CanLoseFirstOrderedMessage) { + SocketUnderTest a("A"); + auto z = std::make_unique("Z"); + + ConnectSockets(a, *z); + z = MaybeHandoverSocket(std::move(z)); + + SendOptions send_options; + send_options.unordered = IsUnordered(false); + send_options.max_retransmissions = 0; + std::vector payload(a.options.mtu - 100); + + // Send a first message (SID=1, SSN=0) + a.socket.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options); + + // First DATA is lost, and retransmission timer will delete it. + a.cb.ConsumeSentPacket(); + AdvanceTime(a, *z, a.options.rto_initial); + ExchangeMessages(a, *z); + + // Send a second message (SID=0, SSN=1). + a.socket.Send(DcSctpMessage(StreamID(1), PPID(52), payload), send_options); + ExchangeMessages(a, *z); + + // The Z socket should receive the second message, but not the first. + absl::optional msg = z->cb.ConsumeReceivedMessage(); + ASSERT_TRUE(msg.has_value()); + EXPECT_EQ(msg->ppid(), PPID(52)); + + EXPECT_FALSE(z->cb.ConsumeReceivedMessage().has_value()); + + MaybeHandoverSocketAndSendMessage(a, std::move(z)); +} + } // namespace } // namespace dcsctp