From 7726b7d067789f57b18072b928fcada337115d1c Mon Sep 17 00:00:00 2001
From: Victor Boivie <boivie@webrtc.org>
Date: Thu, 2 Jun 2022 21:49:12 +0200
Subject: [PATCH] dcsctp: Abandon chunks marked for fast retransmit

The current code assumed that chunks that were scheduled for fast
retransmission would never be abandoned, as chunks marked for fast
retransmission would be immediately sent after the SACK has been
processed, giving no time for them to be abandoned.

But fuzzers keep on fuzzing, and can craft a sequence of chunks that
result in a SACK that both marks the chunks for fast retransmission and
later (while processing the same SACK) abandons them.

Bug: chromium:1331087
Change-Id: Id218607e18a6f3a9d6d51044dccb920e1e77372a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264960
Commit-Queue: Florent Castelli <orphis@webrtc.org>
Auto-Submit: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37108}
---
 net/dcsctp/tx/outstanding_data.cc      |  3 +-
 net/dcsctp/tx/outstanding_data_test.cc | 45 ++++++++++++++++++++++++++
 2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/net/dcsctp/tx/outstanding_data.cc b/net/dcsctp/tx/outstanding_data.cc
index 37bba69253..c013ac5bdd 100644
--- a/net/dcsctp/tx/outstanding_data.cc
+++ b/net/dcsctp/tx/outstanding_data.cc
@@ -284,8 +284,7 @@ void OutstandingData::AbandonAllFor(const Item& item) {
       RTC_DLOG(LS_VERBOSE) << "Marking chunk " << *tsn.Wrap()
                            << " as abandoned";
       if (other.should_be_retransmitted()) {
-        RTC_DCHECK(to_be_fast_retransmitted_.find(tsn) ==
-                   to_be_fast_retransmitted_.end());
+        to_be_fast_retransmitted_.erase(tsn);
         to_be_retransmitted_.erase(tsn);
       }
       other.Abandon();
diff --git a/net/dcsctp/tx/outstanding_data_test.cc b/net/dcsctp/tx/outstanding_data_test.cc
index 5b88c6ddad..113f5f44e4 100644
--- a/net/dcsctp/tx/outstanding_data_test.cc
+++ b/net/dcsctp/tx/outstanding_data_test.cc
@@ -474,5 +474,50 @@ TEST_F(OutstandingDataTest, MustRetransmitBeforeGettingNackedAgain) {
   EXPECT_FALSE(buf_.has_data_to_be_retransmitted());
 }
 
+TEST_F(OutstandingDataTest, CanAbandonChunksMarkedForFastRetransmit) {
+  // This test is a bit convoluted, and can't really happen with a well behaving
+  // client, but this was found by fuzzers. This test will verify that a message
+  // that was both marked as "to be fast retransmitted" and "abandoned" at the
+  // same time doesn't cause any consistency issues.
+
+  // Add chunks 10-14, but chunk 11 has zero retransmissions. When chunk 10 and
+  // 11 are NACKed three times, chunk 10 will be marked for retransmission, but
+  // chunk 11 will be abandoned, which also abandons chunk 10, as it's part of
+  // the same message.
+  buf_.Insert(gen_.Ordered({1}, "B"), MaxRetransmits::NoLimit(), kNow,
+              TimeMs::InfiniteFuture());  // 10
+  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits(0), kNow,
+              TimeMs::InfiniteFuture());  // 11
+  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
+              TimeMs::InfiniteFuture());  // 12
+  buf_.Insert(gen_.Ordered({1}, ""), MaxRetransmits::NoLimit(), kNow,
+              TimeMs::InfiniteFuture());  // 13
+  buf_.Insert(gen_.Ordered({1}, "E"), MaxRetransmits::NoLimit(), kNow,
+              TimeMs::InfiniteFuture());  // 13
+
+  // ACK 9, 12
+  std::vector<SackChunk::GapAckBlock> gab1 = {SackChunk::GapAckBlock(3, 3)};
+  EXPECT_FALSE(
+      buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab1, false).has_packet_loss);
+  EXPECT_FALSE(buf_.has_data_to_be_retransmitted());
+
+  // ACK 9, 12, 13
+  std::vector<SackChunk::GapAckBlock> gab2 = {SackChunk::GapAckBlock(3, 4)};
+  EXPECT_FALSE(
+      buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab2, false).has_packet_loss);
+  EXPECT_FALSE(buf_.has_data_to_be_retransmitted());
+
+  EXPECT_CALL(on_discard_, Call(IsUnordered(false), StreamID(1), MID(42)))
+      .WillOnce(Return(false));
+
+  // ACK 9, 12, 13, 14
+  std::vector<SackChunk::GapAckBlock> gab3 = {SackChunk::GapAckBlock(3, 5)};
+  OutstandingData::AckInfo ack =
+      buf_.HandleSack(unwrapper_.Unwrap(TSN(9)), gab3, false);
+  EXPECT_TRUE(ack.has_packet_loss);
+  EXPECT_FALSE(buf_.has_data_to_be_retransmitted());
+  EXPECT_THAT(buf_.GetChunksToBeFastRetransmitted(1000), IsEmpty());
+  EXPECT_THAT(buf_.GetChunksToBeRetransmitted(1000), IsEmpty());
+}
 }  // namespace
 }  // namespace dcsctp