diff --git a/net/dcsctp/rx/data_tracker.cc b/net/dcsctp/rx/data_tracker.cc index f31847b524..8faee9e7d2 100644 --- a/net/dcsctp/rx/data_tracker.cc +++ b/net/dcsctp/rx/data_tracker.cc @@ -123,8 +123,9 @@ bool DataTracker::IsTSNValid(TSN tsn) const { return true; } -void DataTracker::Observe(TSN tsn, +bool DataTracker::Observe(TSN tsn, AnyDataChunk::ImmediateAckFlag immediate_ack) { + bool is_duplicate = false; UnwrappedTSN unwrapped_tsn = tsn_unwrapper_.Unwrap(tsn); // IsTSNValid must be called prior to calling this method. @@ -143,6 +144,7 @@ void DataTracker::Observe(TSN tsn, // packet arrives with duplicate DATA chunk(s) bundled with new DATA chunks, // the endpoint MAY immediately send a SACK." UpdateAckState(AckState::kImmediate, "duplicate data"); + is_duplicate = true; } else { if (unwrapped_tsn == last_cumulative_acked_tsn_.next_value()) { last_cumulative_acked_tsn_ = unwrapped_tsn; @@ -167,6 +169,7 @@ void DataTracker::Observe(TSN tsn, // delay. If a packet arrives with duplicate DATA chunk(s) bundled with // new DATA chunks, the endpoint MAY immediately send a SACK." // No need to do this. SACKs are sent immediately on packet loss below. + is_duplicate = true; } } } @@ -208,6 +211,7 @@ void DataTracker::Observe(TSN tsn, } else if (ack_state_ == AckState::kDelayed) { UpdateAckState(AckState::kImmediate, "received DATA when already delayed"); } + return !is_duplicate; } void DataTracker::HandleForwardTsn(TSN new_cumulative_ack) { diff --git a/net/dcsctp/rx/data_tracker.h b/net/dcsctp/rx/data_tracker.h index fb8add82a2..603a237245 100644 --- a/net/dcsctp/rx/data_tracker.h +++ b/net/dcsctp/rx/data_tracker.h @@ -69,8 +69,9 @@ class DataTracker { // means that there is intentional packet loss. bool IsTSNValid(TSN tsn) const; - // Call for every incoming data chunk. - void Observe(TSN tsn, + // Call for every incoming data chunk. Returns `true` if `tsn` was seen for + // the first time, and `false` if it has been seen before (a duplicate `tsn`). + bool Observe(TSN tsn, AnyDataChunk::ImmediateAckFlag immediate_ack = AnyDataChunk::ImmediateAckFlag(false)); // Called at the end of processing an SCTP packet. diff --git a/net/dcsctp/rx/data_tracker_test.cc b/net/dcsctp/rx/data_tracker_test.cc index 3d8380a05c..43494734b6 100644 --- a/net/dcsctp/rx/data_tracker_test.cc +++ b/net/dcsctp/rx/data_tracker_test.cc @@ -48,9 +48,16 @@ class DataTrackerTest : public testing::Test { std::make_unique("log: ", timer_.get(), kInitialTSN)) { } - void Observer(std::initializer_list tsns) { + void Observer(std::initializer_list tsns, + bool expect_as_duplicate = false) { for (const uint32_t tsn : tsns) { - tracker_->Observe(TSN(tsn), AnyDataChunk::ImmediateAckFlag(false)); + if (expect_as_duplicate) { + EXPECT_FALSE( + tracker_->Observe(TSN(tsn), AnyDataChunk::ImmediateAckFlag(false))); + } else { + EXPECT_TRUE( + tracker_->Observe(TSN(tsn), AnyDataChunk::ImmediateAckFlag(false))); + } } } @@ -125,7 +132,7 @@ TEST_F(DataTrackerTest, AckAlreadyReceivedChunk) { EXPECT_THAT(sack1.gap_ack_blocks(), IsEmpty()); // Receive old chunk - Observer({8}); + Observer({8}, /*expect_as_duplicate=*/true); SackChunk sack2 = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack2.cumulative_tsn_ack(), TSN(11)); EXPECT_THAT(sack2.gap_ack_blocks(), IsEmpty()); @@ -145,7 +152,8 @@ TEST_F(DataTrackerTest, DoubleSendRetransmittedChunk) { EXPECT_THAT(sack2.gap_ack_blocks(), IsEmpty()); // Receive chunk 12 again. - Observer({12, 19, 20, 21}); + Observer({12}, /*expect_as_duplicate=*/true); + Observer({19, 20, 21}); SackChunk sack3 = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack3.cumulative_tsn_ack(), TSN(21)); EXPECT_THAT(sack3.gap_ack_blocks(), IsEmpty()); @@ -176,7 +184,8 @@ TEST_F(DataTrackerTest, ForwardTsnSkipsFromGapBlock) { TEST_F(DataTrackerTest, ExampleFromRFC3758) { tracker_->HandleForwardTsn(TSN(102)); - Observer({102, 104, 105, 107}); + Observer({102}, /*expect_as_duplicate=*/true); + Observer({104, 105, 107}); tracker_->HandleForwardTsn(TSN(103)); @@ -246,7 +255,8 @@ TEST_F(DataTrackerTest, WillNotAcceptInvalidTSNs) { } TEST_F(DataTrackerTest, ReportSingleDuplicateTsns) { - Observer({11, 12, 11}); + Observer({11, 12}); + Observer({11}, /*expect_as_duplicate=*/true); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(12)); EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty()); @@ -254,7 +264,9 @@ TEST_F(DataTrackerTest, ReportSingleDuplicateTsns) { } TEST_F(DataTrackerTest, ReportMultipleDuplicateTsns) { - Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16}); + Observer({11, 12, 13, 14}); + Observer({12, 13, 12, 13}, /*expect_as_duplicate=*/true); + Observer({15, 16}); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(16)); EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty()); @@ -262,7 +274,9 @@ TEST_F(DataTrackerTest, ReportMultipleDuplicateTsns) { } TEST_F(DataTrackerTest, ReportDuplicateTsnsInGapAckBlocks) { - Observer({11, /*12,*/ 13, 14, 13, 14, 15, 16}); + Observer({11, /*12,*/ 13, 14}); + Observer({13, 14}, /*expect_as_duplicate=*/true); + Observer({15, 16}); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(11)); EXPECT_THAT(sack.gap_ack_blocks(), ElementsAre(SackChunk::GapAckBlock(2, 5))); @@ -270,7 +284,9 @@ TEST_F(DataTrackerTest, ReportDuplicateTsnsInGapAckBlocks) { } TEST_F(DataTrackerTest, ClearsDuplicateTsnsAfterCreatingSack) { - Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16}); + Observer({11, 12, 13, 14}); + Observer({12, 13, 12, 13}, /*expect_as_duplicate=*/true); + Observer({15, 16}); SackChunk sack1 = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack1.cumulative_tsn_ack(), TSN(16)); EXPECT_THAT(sack1.gap_ack_blocks(), IsEmpty()); @@ -380,7 +396,7 @@ TEST_F(DataTrackerTest, SendsSackOnDuplicateDataChunks) { tracker_->ObservePacketEnd(); EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); - Observer({11}); + Observer({11}, /*expect_as_duplicate=*/true); tracker_->ObservePacketEnd(); EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); @@ -394,7 +410,7 @@ TEST_F(DataTrackerTest, SendsSackOnDuplicateDataChunks) { EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); // Duplicate again - Observer({12}); + Observer({12}, /*expect_as_duplicate=*/true); tracker_->ObservePacketEnd(); EXPECT_TRUE(tracker_->ShouldSendAck()); EXPECT_FALSE(timer_->is_running()); @@ -418,7 +434,7 @@ TEST_F(DataTrackerTest, GapAckBlockAddsAnother) { TEST_F(DataTrackerTest, GapAckBlockAddsDuplicate) { Observer({12}); - Observer({12}); + Observer({12}, /*expect_as_duplicate=*/true); SackChunk sack = tracker_->CreateSelectiveAck(kArwnd); EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(10)); EXPECT_THAT(sack.gap_ack_blocks(), ElementsAre(SackChunk::GapAckBlock(2, 2))); diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc index b93584ed47..21b40d854f 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -1024,11 +1024,12 @@ void DcSctpSocket::HandleDataCommon(AnyDataChunk& chunk) { return; } - tcb_->data_tracker().Observe(tsn, immediate_ack); - tcb_->reassembly_queue().MaybeResetStreamsDeferred( - tcb_->data_tracker().last_cumulative_acked_tsn()); - tcb_->reassembly_queue().Add(tsn, std::move(data)); - DeliverReassembledMessages(); + if (tcb_->data_tracker().Observe(tsn, immediate_ack)) { + tcb_->reassembly_queue().MaybeResetStreamsDeferred( + tcb_->data_tracker().last_cumulative_acked_tsn()); + tcb_->reassembly_queue().Add(tsn, std::move(data)); + DeliverReassembledMessages(); + } } void DcSctpSocket::HandleInit(const CommonHeader& header, diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index d30043d332..848b7d6274 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -2214,5 +2214,53 @@ TEST_P(DcSctpSocketParametrizedTest, CanLoseFirstOrderedMessage) { MaybeHandoverSocketAndSendMessage(a, std::move(z)); } +TEST(DcSctpSocketTest, ReceiveBothUnorderedAndOrderedWithSameTSN) { + /* This issue was found by fuzzing. */ + SocketUnderTest a("A"); + SocketUnderTest z("Z"); + + a.socket.Connect(); + std::vector init_data = a.cb.ConsumeSentPacket(); + ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket init_packet, + SctpPacket::Parse(init_data)); + ASSERT_HAS_VALUE_AND_ASSIGN( + InitChunk init_chunk, + InitChunk::Parse(init_packet.descriptors()[0].data)); + z.socket.ReceivePacket(init_data); + a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); + z.socket.ReceivePacket(a.cb.ConsumeSentPacket()); + a.socket.ReceivePacket(z.cb.ConsumeSentPacket()); + + // Receive a short unordered message with tsn=INITIAL_TSN+1 + TSN tsn = init_chunk.initial_tsn(); + AnyDataChunk::Options opts; + opts.is_beginning = Data::IsBeginning(true); + opts.is_end = Data::IsEnd(true); + opts.is_unordered = IsUnordered(true); + z.socket.ReceivePacket( + SctpPacket::Builder(z.socket.verification_tag(), z.options) + .Add(DataChunk(TSN(*tsn + 1), StreamID(1), SSN(0), PPID(53), + std::vector(10), opts)) + .Build()); + + // Now receive a longer _ordered_ message with [INITIAL_TSN, INITIAL_TSN+1]. + // This isn't allowed as it reuses TSN=53 with different properties, but it + // shouldn't cause any issues. + opts.is_unordered = IsUnordered(false); + opts.is_end = Data::IsEnd(false); + z.socket.ReceivePacket( + SctpPacket::Builder(z.socket.verification_tag(), z.options) + .Add(DataChunk(tsn, StreamID(1), SSN(0), PPID(53), + std::vector(10), opts)) + .Build()); + + opts.is_beginning = Data::IsBeginning(false); + opts.is_end = Data::IsEnd(true); + z.socket.ReceivePacket( + SctpPacket::Builder(z.socket.verification_tag(), z.options) + .Add(DataChunk(TSN(*tsn + 1), StreamID(1), SSN(0), PPID(53), + std::vector(10), opts)) + .Build()); +} } // namespace } // namespace dcsctp