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

dcsctp: Don't reassemble already received chunks

Browse Source 
This is a solution to some problems that have been found locally when
running the fuzzer for a long time. The fuzzer keeps on fuzzing, and has
found a way to trigger a consistency check to fail when a client
intentionally sends different messages - unordered and ordered - using
the same TSNs. As the reassembly queue has different handling of ordered
and unordered chunks due to how they are reassembled, it will not notice
if it receives two different chunks with the same TSN. They will both go
to their respective reassembly streams, as those are separate by design.

The data tracker - which keeps track of all received DATA chunks as it
needs to generate SACKs, has a global understanding of all received
chunks. By having it indicate if this is a duplicate received chunk, the
socket can avoid forwarding both chunks to the reassembly queue; only
one chunk will get there.

Bug: None
Change-Id: I602a8552a9a4c853684fcf105309ec3d8073f2c2
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/256110
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36316}
This commit is contained in:
Victor Boivie 2022-03-20 19:59:03 +01:00 committed by WebRTC LUCI CQ
parent 0c28820dcd
commit 568bc23208
5 changed files with 90 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
net/dcsctp/rx/data_tracker.cc
View File

@ -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) {

5
net/dcsctp/rx/data_tracker.h
View File

@ -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.

40
net/dcsctp/rx/data_tracker_test.cc
View File

@ -48,9 +48,16 @@ class DataTrackerTest : public testing::Test {
std::make_unique<DataTracker>("log: ", timer_.get(), kInitialTSN)) {
}
void Observer(std::initializer_list<uint32_t> tsns) {
void Observer(std::initializer_list<uint32_t> 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)));

11
net/dcsctp/socket/dcsctp_socket.cc
View File

@ -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,

48
net/dcsctp/socket/dcsctp_socket_test.cc
View File

@ -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<uint8_t> 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<uint8_t>(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<uint8_t>(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<uint8_t>(10), opts))
.Build());
}
} // namespace
} // namespace dcsctp