dcsctp: Avoid bundling FORWARD-TSN and DATA chunks

dcSCTP seems to be able to provoke usrsctp to send ABORT in some
situations, as described in
https://github.com/sctplab/usrsctp/issues/597. Using a packetdrill
script, it seems as a contributing factor to this behavior is when a
FORWARD-TSN chunk is bundled with a DATA chunk. This is a valid and
recommended pattern in RFC3758:

  "F2) The data sender SHOULD always attempt to bundle an outgoing
       FORWARD TSN with outbound DATA chunks for efficiency."

However, that seems to be a rare event in usrsctp, which generally sends
each FORWARD-TSN in a separate packet.

By doing the same, the assumption is that this scenario will generally
be avoided.

With many browsers and other clients using usrsctp, and which will not
be upgraded for a long time, there is an advantage of avoiding the issue
even if it is according to specification.

Before this change, a FORWARD-TSN was bundled with outgoing DATA and due
to this, it wasn't rate limited as the overhead was very little. With
this change, a rate limiting behavior has been added to avoid sending
too many FORWARD-TSN in small packets. It will be sent every RTT, or
200 ms, whichever is smallest. This is also described in the RFC.

Bug: webrtc:12961
Change-Id: I3d8036a34f999f405958982534bfa0e99e330da3
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/229101
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34801}

net/dcsctp/socket/dcsctp_socket_test.cc

@@ -26,6 +26,7 @@
 #include "net/dcsctp/packet/chunk/data_chunk.h"
 #include "net/dcsctp/packet/chunk/data_common.h"
 #include "net/dcsctp/packet/chunk/error_chunk.h"
+#include "net/dcsctp/packet/chunk/forward_tsn_chunk.h"
 #include "net/dcsctp/packet/chunk/heartbeat_ack_chunk.h"
 #include "net/dcsctp/packet/chunk/heartbeat_request_chunk.h"
 #include "net/dcsctp/packet/chunk/idata_chunk.h"
@@ -1764,5 +1765,86 @@ TEST_F(DcSctpSocketTest, SendsOnlyLargePackets) {
   }
 }
 
+TEST_F(DcSctpSocketTest, DoesntBundleForwardTsnWithData) {
+  ConnectSockets();
+
+  // Force an RTT measurement using heartbeats.
+  AdvanceTime(options_.heartbeat_interval);
+  RunTimers();
+
+  // HEARTBEAT
+  std::vector<uint8_t> hb_req_a = cb_a_.ConsumeSentPacket();
+  std::vector<uint8_t> hb_req_z = cb_z_.ConsumeSentPacket();
+
+  constexpr DurationMs kRtt = DurationMs(80);
+  AdvanceTime(kRtt);
+  sock_z_.ReceivePacket(hb_req_a);
+  sock_a_.ReceivePacket(hb_req_z);
+
+  // HEARTBEAT_ACK
+  sock_a_.ReceivePacket(cb_z_.ConsumeSentPacket());
+  sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket());
+
+  SendOptions send_options;
+  send_options.max_retransmissions = 0;
+  std::vector<uint8_t> payload(options_.mtu - 100);
+
+  // Send an initial message that is received, but the SACK was lost
+  sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options);
+  // DATA
+  sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket());
+  // SACK (lost)
+  std::vector<uint8_t> sack = cb_z_.ConsumeSentPacket();
+
+  // Queue enough messages to fill the congestion window.
+  do {
+    sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options);
+  } while (!cb_a_.ConsumeSentPacket().empty());
+
+  // Enqueue at least one more.
+  sock_a_.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options);
+
+  // Let all of them expire by T3-RTX and inspect what's sent.
+  AdvanceTime(options_.rto_initial);
+  RunTimers();
+
+  std::vector<uint8_t> sent1 = cb_a_.ConsumeSentPacket();
+  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet1, SctpPacket::Parse(sent1));
+
+  EXPECT_THAT(packet1.descriptors(), SizeIs(1));
+  EXPECT_EQ(packet1.descriptors()[0].type, ForwardTsnChunk::kType);
+
+  std::vector<uint8_t> sent2 = cb_a_.ConsumeSentPacket();
+  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet2, SctpPacket::Parse(sent2));
+
+  EXPECT_GE(packet2.descriptors().size(), 1u);
+  EXPECT_EQ(packet2.descriptors()[0].type, DataChunk::kType);
+
+  // Drop all remaining packets that A has sent.
+  while (!cb_a_.ConsumeSentPacket().empty()) {
+  }
+
+  // Replay the SACK, and see if a FORWARD-TSN is sent again.
+  sock_a_.ReceivePacket(sack);
+
+  // It shouldn't be sent as not enough time has passed yet. Instead, more
+  // DATA chunks are sent, that are in the queue.
+  std::vector<uint8_t> sent3 = cb_a_.ConsumeSentPacket();
+  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet3, SctpPacket::Parse(sent3));
+
+  EXPECT_GE(packet2.descriptors().size(), 1u);
+  EXPECT_EQ(packet3.descriptors()[0].type, DataChunk::kType);
+
+  // Now let RTT time pass, to allow a FORWARD-TSN to be sent again.
+  AdvanceTime(kRtt);
+  sock_a_.ReceivePacket(sack);
+
+  std::vector<uint8_t> sent4 = cb_a_.ConsumeSentPacket();
+  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet4, SctpPacket::Parse(sent4));
+
+  EXPECT_THAT(packet4.descriptors(), SizeIs(1));
+  EXPECT_EQ(packet4.descriptors()[0].type, ForwardTsnChunk::kType);
+}
+
 }  // namespace
 }  // namespace dcsctp

net/dcsctp/socket/transmission_control_block.cc

@@ -82,8 +82,31 @@ void TransmissionControlBlock::MaybeSendSack() {
   }
 }
 
+void TransmissionControlBlock::MaybeSendForwardTsn(SctpPacket::Builder& builder,
+                                                   TimeMs now) {
+  if (now >= limit_forward_tsn_until_ &&
+      retransmission_queue_.ShouldSendForwardTsn(now)) {
+    if (capabilities_.message_interleaving) {
+      builder.Add(retransmission_queue_.CreateIForwardTsn());
+    } else {
+      builder.Add(retransmission_queue_.CreateForwardTsn());
+    }
+    packet_sender_.Send(builder);
+    // https://datatracker.ietf.org/doc/html/rfc3758
+    // "IMPLEMENTATION NOTE: An implementation may wish to limit the number of
+    // duplicate FORWARD TSN chunks it sends by ... waiting a full RTT before
+    // sending a duplicate FORWARD TSN."
+    // "Any delay applied to the sending of FORWARD TSN chunk SHOULD NOT exceed
+    // 200ms and MUST NOT exceed 500ms".
+    limit_forward_tsn_until_ = now + std::min(DurationMs(200), rto_.srtt());
+  }
+}
+
 void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder,
                                                    TimeMs now) {
+  // FORWARD-TSNs are sent as separate packets to avoid bugs.webrtc.org/12961.
+  MaybeSendForwardTsn(builder, now);
+
   for (int packet_idx = 0;
        packet_idx < options_.max_burst && retransmission_queue_.can_send_data();
        ++packet_idx) {
@@ -108,13 +131,6 @@ void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder,
         builder.Add(data_tracker_.CreateSelectiveAck(
             reassembly_queue_.remaining_bytes()));
       }
-      if (retransmission_queue_.ShouldSendForwardTsn(now)) {
-        if (capabilities_.message_interleaving) {
-          builder.Add(retransmission_queue_.CreateIForwardTsn());
-        } else {
-          builder.Add(retransmission_queue_.CreateForwardTsn());
-        }
-      }
       absl::optional<ReConfigChunk> reconfig =
           stream_reset_handler_.MakeStreamResetRequest();
       if (reconfig.has_value()) {

net/dcsctp/socket/transmission_control_block.h

@@ -152,6 +152,9 @@ class TransmissionControlBlock : public Context {
   // Sends a SACK, if there is a need to.
   void MaybeSendSack();
 
+  // Sends a FORWARD-TSN, if it is needed and allowed (rate-limited).
+  void MaybeSendForwardTsn(SctpPacket::Builder& builder, TimeMs now);
+
   // Will be set while the socket is in kCookieEcho state. In this state, there
   // can only be a single packet outstanding, and it must contain the COOKIE
   // ECHO chunk as the first chunk in that packet, until the COOKIE ACK has been
@@ -206,6 +209,8 @@ class TransmissionControlBlock : public Context {
   const TieTag tie_tag_;
   const std::function<bool()> is_connection_established_;
   PacketSender& packet_sender_;
+  // Rate limiting of FORWARD-TSN. Next can be sent at or after this timestamp.
+  TimeMs limit_forward_tsn_until_ = TimeMs(0);
 
   RetransmissionTimeout rto_;
   RetransmissionErrorCounter tx_error_counter_;