diff --git a/net/dcsctp/packet/chunk/forward_tsn_chunk.cc b/net/dcsctp/packet/chunk/forward_tsn_chunk.cc index f01505094d..e432114c50 100644 --- a/net/dcsctp/packet/chunk/forward_tsn_chunk.cc +++ b/net/dcsctp/packet/chunk/forward_tsn_chunk.cc @@ -87,6 +87,9 @@ void ForwardTsnChunk::SerializeTo(std::vector& out) const { std::string ForwardTsnChunk::ToString() const { rtc::StringBuilder sb; sb << "FORWARD-TSN, new_cumulative_tsn=" << *new_cumulative_tsn(); + for (const auto& skipped : skipped_streams()) { + sb << ", skip " << skipped.stream_id.value() << ":" << *skipped.ssn; + } return sb.str(); } } // namespace dcsctp diff --git a/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc b/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc index 9420c1f2ef..51f97f2396 100644 --- a/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc +++ b/net/dcsctp/packet/chunk/forward_tsn_chunk_test.cc @@ -56,7 +56,8 @@ TEST(ForwardTsnChunkTest, SerializeAndDeserialize) { ElementsAre(ForwardTsnChunk::SkippedStream(StreamID(1), SSN(23)), ForwardTsnChunk::SkippedStream(StreamID(42), SSN(99)))); - EXPECT_EQ(deserialized.ToString(), "FORWARD-TSN, new_cumulative_tsn=123"); + EXPECT_EQ(deserialized.ToString(), + "FORWARD-TSN, new_cumulative_tsn=123, skip 1:23, skip 42:99"); } } // namespace diff --git a/net/dcsctp/rx/reassembly_queue.cc b/net/dcsctp/rx/reassembly_queue.cc index 5791d6805c..cbf198b136 100644 --- a/net/dcsctp/rx/reassembly_queue.cc +++ b/net/dcsctp/rx/reassembly_queue.cc @@ -210,8 +210,16 @@ void ReassemblyQueue::AddReassembledMessage( << ", payload=" << message.payload().size() << " bytes"; for (const UnwrappedTSN tsn : tsns) { - // Update watermark, or insert into delivered_tsns_ - if (tsn == last_assembled_tsn_watermark_.next_value()) { + if (tsn <= last_assembled_tsn_watermark_) { + // This can be provoked by a misbehaving peer by sending FORWARD-TSN with + // invalid SSNs, allowing ordered messages to stay in the queue that + // should've been discarded. + RTC_DLOG(LS_VERBOSE) + << log_prefix_ + << "Message is built from fragments already seen - skipping"; + return; + } else if (tsn == last_assembled_tsn_watermark_.next_value()) { + // Update watermark, or insert into delivered_tsns_ last_assembled_tsn_watermark_.Increment(); } else { delivered_tsns_.insert(tsn); diff --git a/net/dcsctp/rx/reassembly_queue_test.cc b/net/dcsctp/rx/reassembly_queue_test.cc index d1e3bf6413..bc1b776837 100644 --- a/net/dcsctp/rx/reassembly_queue_test.cc +++ b/net/dcsctp/rx/reassembly_queue_test.cc @@ -389,5 +389,21 @@ TEST_F(ReassemblyQueueTest, HandoverAfterHavingAssembedOneMessage) { reasm2.Add(TSN(11), gen_.Ordered({1, 2, 3, 4}, "BE")); EXPECT_THAT(reasm2.FlushMessages(), SizeIs(1)); } + +TEST_F(ReassemblyQueueTest, HandleInconsistentForwardTSN) { + // Found when fuzzing. + ReassemblyQueue reasm("log: ", TSN(10), kBufferSize); + // Add TSN=43, SSN=7. Can't be reassembled as previous SSNs aren't known. + reasm.Add(TSN(43), Data(kStreamID, SSN(7), MID(0), FSN(0), kPPID, + std::vector(10), Data::IsBeginning(true), + Data::IsEnd(true), IsUnordered(false))); + + // Invalid, as TSN=44 have to have SSN>=7, but peer says 6. + reasm.Handle(ForwardTsnChunk( + TSN(44), {ForwardTsnChunk::SkippedStream(kStreamID, SSN(6))})); + + // Don't assemble SSN=7, as that TSN is skipped. + EXPECT_FALSE(reasm.HasMessages()); +} } // namespace } // namespace dcsctp