dcsctp: Don't sent more packets before COOKIE ACK
While in the COOKIE ECHO state, there is a TCB and there might be data
in the send buffer, and RFC4960 allows the COOKIE ECHO chunk to bundle
additional DATA chunks in the same packet, but there mustn't be more
than one such packet sent, and that packet must have a COOKIE ECHO chunk
as the first chunk in it.
When the COOKIE ACK chunk has been received, the socket is allowed to
send multiple packets.
Previously, this was state managed by the socket and not the TCB, as
the socket is responsible for moving between the different states. And
when the COOKIE ECHO chunk was sent, the TCB was instructed to only send
a single packet by the socket.
However, if there were retransmissions or anything else that could
result in calling TransmissionControlBlock::SendBufferedChunks, it would
do as instructed and send those, even if the socket was in a state where
that wasn't allowed.
When the peer was dcSCTP, this didn't cause any issues as dcSCTP tries
to be tolerant in what it receives (but strict in what it sends, except
for when there are bugs). When the peer was usrsctp, it would send an
ABORT for each received packet that didn't have a COOKIE ECHO as the
first chunk, and then restart the handshake (sending an INIT). So this
resulted in a longer handshake, but the connection would eventually be
correctly established and any DATA chunks that resulted in the ABORTs
would've been retransmitted.
By making the TCB aware of that particular state, and to make it
responsible for creating the SCTP packet with the COOKIE ECHO chunk
first, and also to only send a single packet when it is in that state,
there will not be any way to bypass this limitation.
Also, while not explicitly mentioned in the RFC, the retransmission
timer will not affect resending any outstanding DATA chunks that were
bundled together with the COOKIE ECHO chunk, as then there would be two
timers that both would drive resending COOKIE ECHO and DATA chunks.
Bug: webrtc:12880
Change-Id: I76f215a03cceab5bafe9f16eb4775f3dc68a6f05
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222645
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34329}
diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc
index 0c18199..71bc98c 100644
--- a/net/dcsctp/socket/dcsctp_socket.cc
+++ b/net/dcsctp/socket/dcsctp_socket.cc
@@ -191,7 +191,7 @@
case State::kCookieEchoed:
return (tcb_ != nullptr && !t1_init_->is_running() &&
t1_cookie_->is_running() && !t2_shutdown_->is_running() &&
- cookie_echo_chunk_.has_value());
+ tcb_->has_cookie_echo_chunk());
case State::kEstablished:
return (tcb_ != nullptr && !t1_init_->is_running() &&
!t1_cookie_->is_running() && !t2_shutdown_->is_running());
@@ -339,7 +339,6 @@
t1_cookie_->Stop();
t2_shutdown_->Stop();
tcb_ = nullptr;
- cookie_echo_chunk_ = absl::nullopt;
if (error == ErrorKind::kNoError) {
callbacks_.OnClosed();
@@ -776,7 +775,7 @@
RTC_DCHECK(state_ == State::kCookieEchoed);
if (t1_cookie_->is_running()) {
- SendCookieEcho();
+ tcb_->SendBufferedPackets(callbacks_.TimeMillis());
} else {
InternalClose(ErrorKind::kTooManyRetries, "No COOKIE_ACK received");
}
@@ -1048,19 +1047,6 @@
SendPacket(b);
}
-void DcSctpSocket::SendCookieEcho() {
- RTC_DCHECK(tcb_ != nullptr);
- TimeMs now = callbacks_.TimeMillis();
- SctpPacket::Builder b = tcb_->PacketBuilder();
- b.Add(*cookie_echo_chunk_);
-
- // https://tools.ietf.org/html/rfc4960#section-5.1
- // "The COOKIE ECHO chunk can be bundled with any pending outbound DATA
- // chunks, but it MUST be the first chunk in the packet and until the COOKIE
- // ACK is returned the sender MUST NOT send any other packets to the peer."
- tcb_->SendBufferedPackets(b, now, /*only_one_packet=*/true);
-}
-
void DcSctpSocket::HandleInitAck(
const CommonHeader& header,
const SctpPacket::ChunkDescriptor& descriptor) {
@@ -1106,8 +1092,8 @@
SetState(State::kCookieEchoed, "INIT_ACK received");
// The connection isn't fully established just yet.
- cookie_echo_chunk_ = CookieEchoChunk(cookie->data());
- SendCookieEcho();
+ tcb_->SetCookieEchoChunk(CookieEchoChunk(cookie->data()));
+ tcb_->SendBufferedPackets(callbacks_.TimeMillis());
t1_cookie_->Start();
}
@@ -1147,7 +1133,9 @@
t1_init_->Stop();
t1_cookie_->Stop();
if (state_ != State::kEstablished) {
- cookie_echo_chunk_ = absl::nullopt;
+ if (tcb_ != nullptr) {
+ tcb_->ClearCookieEchoChunk();
+ }
SetState(State::kEstablished, "COOKIE_ECHO received");
callbacks_.OnConnected();
}
@@ -1270,7 +1258,7 @@
// RFC 4960, Errata ID: 4400
t1_cookie_->Stop();
- cookie_echo_chunk_ = absl::nullopt;
+ tcb_->ClearCookieEchoChunk();
SetState(State::kEstablished, "COOKIE_ACK received");
tcb_->SendBufferedPackets(callbacks_.TimeMillis());
callbacks_.OnConnected();
diff --git a/net/dcsctp/socket/dcsctp_socket.h b/net/dcsctp/socket/dcsctp_socket.h
index 592e279..32e89b5 100644
--- a/net/dcsctp/socket/dcsctp_socket.h
+++ b/net/dcsctp/socket/dcsctp_socket.h
@@ -148,8 +148,6 @@
void MaybeSendShutdownOnPacketReceived(const SctpPacket& packet);
// Sends a INIT chunk.
void SendInit();
- // Sends a CookieEcho chunk.
- void SendCookieEcho();
// Sends a SHUTDOWN chunk.
void SendShutdown();
// Sends a SHUTDOWN-ACK chunk.
@@ -261,10 +259,6 @@
// the connection is established, this component is not in the TCB.
RRSendQueue send_queue_;
- // Only valid when state == State::kCookieEchoed
- // A cached Cookie Echo Chunk, to be re-sent on timer expiry.
- absl::optional<CookieEchoChunk> cookie_echo_chunk_ = absl::nullopt;
-
// Contains verification tag and initial TSN between having sent the INIT
// until the connection is established (there is no TCB at this point).
ConnectParameters connect_params_;
diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc
index 942085f..e5db12c 100644
--- a/net/dcsctp/socket/dcsctp_socket_test.cc
+++ b/net/dcsctp/socket/dcsctp_socket_test.cc
@@ -494,6 +494,65 @@
EXPECT_EQ(sock_a_.state(), SocketState::kClosed);
}
+TEST_F(DcSctpSocketTest, DoesntSendMorePacketsUntilCookieAckHasBeenReceived) {
+ sock_a_.Send(DcSctpMessage(StreamID(1), PPID(53),
+ std::vector<uint8_t>(kLargeMessageSize)),
+ kSendOptions);
+ sock_a_.Connect();
+
+ // Z reads INIT, produces INIT_ACK
+ sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket());
+ // A reads INIT_ACK, produces COOKIE_ECHO
+ sock_a_.ReceivePacket(cb_z_.ConsumeSentPacket());
+
+ // COOKIE_ECHO is never received by Z.
+ ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket cookie_echo_packet1,
+ SctpPacket::Parse(cb_a_.ConsumeSentPacket()));
+ EXPECT_THAT(cookie_echo_packet1.descriptors(), SizeIs(2));
+ EXPECT_EQ(cookie_echo_packet1.descriptors()[0].type, CookieEchoChunk::kType);
+ EXPECT_EQ(cookie_echo_packet1.descriptors()[1].type, DataChunk::kType);
+
+ EXPECT_THAT(cb_a_.ConsumeSentPacket(), IsEmpty());
+
+ // There are DATA chunks in the sent packet (that was lost), which means that
+ // the T3-RTX timer is running, but as the socket is in kCookieEcho state, it
+ // will be T1-COOKIE that drives retransmissions, so when the T3-RTX expires,
+ // nothing should be retransmitted.
+ ASSERT_TRUE(options_.rto_initial < options_.t1_cookie_timeout);
+ AdvanceTime(options_.rto_initial);
+ RunTimers();
+ EXPECT_THAT(cb_a_.ConsumeSentPacket(), IsEmpty());
+
+ // When T1-COOKIE expires, both the COOKIE-ECHO and DATA should be present.
+ AdvanceTime(options_.t1_cookie_timeout - options_.rto_initial);
+ RunTimers();
+
+ // And this COOKIE-ECHO and DATA is also lost - never received by Z.
+ ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket cookie_echo_packet2,
+ SctpPacket::Parse(cb_a_.ConsumeSentPacket()));
+ EXPECT_THAT(cookie_echo_packet2.descriptors(), SizeIs(2));
+ EXPECT_EQ(cookie_echo_packet2.descriptors()[0].type, CookieEchoChunk::kType);
+ EXPECT_EQ(cookie_echo_packet2.descriptors()[1].type, DataChunk::kType);
+
+ EXPECT_THAT(cb_a_.ConsumeSentPacket(), IsEmpty());
+
+ // COOKIE_ECHO has exponential backoff.
+ AdvanceTime(options_.t1_cookie_timeout * 2);
+ RunTimers();
+
+ // Z reads COOKIE_ECHO, produces COOKIE_ACK
+ sock_z_.ReceivePacket(cb_a_.ConsumeSentPacket());
+ // A reads COOKIE_ACK.
+ sock_a_.ReceivePacket(cb_z_.ConsumeSentPacket());
+
+ EXPECT_EQ(sock_a_.state(), SocketState::kConnected);
+ EXPECT_EQ(sock_z_.state(), SocketState::kConnected);
+
+ ExchangeMessages(sock_a_, cb_a_, sock_z_, cb_z_);
+ EXPECT_THAT(cb_z_.ConsumeReceivedMessage()->payload(),
+ SizeIs(kLargeMessageSize));
+}
+
TEST_F(DcSctpSocketTest, ShutdownConnection) {
ConnectSockets();
diff --git a/net/dcsctp/socket/transmission_control_block.cc b/net/dcsctp/socket/transmission_control_block.cc
index 6e0be6a..4fde40c 100644
--- a/net/dcsctp/socket/transmission_control_block.cc
+++ b/net/dcsctp/socket/transmission_control_block.cc
@@ -54,9 +54,15 @@
TimeMs now = callbacks_.TimeMillis();
RTC_DLOG(LS_INFO) << log_prefix_ << "Timer " << t3_rtx_->name()
<< " has expired";
- if (IncrementTxErrorCounter("t3-rtx expired")) {
- retransmission_queue_.HandleT3RtxTimerExpiry();
- SendBufferedPackets(now);
+ if (cookie_echo_chunk_.has_value()) {
+ // In the COOKIE_ECHO state, let the T1-COOKIE timer trigger
+ // retransmissions, to avoid having two timers doing that.
+ RTC_DLOG(LS_VERBOSE) << "Not retransmitting as T1-cookie is active.";
+ } else {
+ if (IncrementTxErrorCounter("t3-rtx expired")) {
+ retransmission_queue_.HandleT3RtxTimerExpiry();
+ SendBufferedPackets(now);
+ }
}
return absl::nullopt;
}
@@ -77,12 +83,19 @@
}
void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder,
- TimeMs now,
- bool only_one_packet) {
+ TimeMs now) {
for (int packet_idx = 0;; ++packet_idx) {
// Only add control chunks to the first packet that is sent, if sending
// multiple packets in one go (as allowed by the congestion window).
if (packet_idx == 0) {
+ if (cookie_echo_chunk_.has_value()) {
+ // https://tools.ietf.org/html/rfc4960#section-5.1
+ // "The COOKIE ECHO chunk can be bundled with any pending outbound DATA
+ // chunks, but it MUST be the first chunk in the packet..."
+ RTC_DCHECK(builder.empty());
+ builder.Add(*cookie_echo_chunk_);
+ }
+
// https://tools.ietf.org/html/rfc4960#section-6
// "Before an endpoint transmits a DATA chunk, if any received DATA
// chunks have not been acknowledged (e.g., due to delayed ack), the
@@ -122,7 +135,11 @@
break;
}
Send(builder);
- if (only_one_packet) {
+
+ if (cookie_echo_chunk_.has_value()) {
+ // https://tools.ietf.org/html/rfc4960#section-5.1
+ // "... until the COOKIE ACK is returned the sender MUST NOT send any
+ // other packets to the peer."
break;
}
}
diff --git a/net/dcsctp/socket/transmission_control_block.h b/net/dcsctp/socket/transmission_control_block.h
index 5d3bfb8..172f7c0 100644
--- a/net/dcsctp/socket/transmission_control_block.h
+++ b/net/dcsctp/socket/transmission_control_block.h
@@ -19,6 +19,7 @@
#include "absl/strings/string_view.h"
#include "net/dcsctp/common/sequence_numbers.h"
+#include "net/dcsctp/packet/chunk/cookie_echo_chunk.h"
#include "net/dcsctp/packet/sctp_packet.h"
#include "net/dcsctp/public/dcsctp_options.h"
#include "net/dcsctp/public/dcsctp_socket.h"
@@ -144,20 +145,31 @@
// Sends a SACK, if there is a need to.
void MaybeSendSack();
- // Fills `builder` (which may already be filled with control chunks) with
- // with other control and data chunks, and sends packets as much as can be
- // allowed by the congestion control algorithm. If `only_one_packet` is true,
- // only a single packet will be sent. Otherwise, zero, one or multiple may be
- // sent.
- void SendBufferedPackets(SctpPacket::Builder& builder,
- TimeMs now,
- bool only_one_packet = false);
+ // 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
+ // received, which will make the socket call `ClearCookieEchoChunk`.
+ void SetCookieEchoChunk(CookieEchoChunk chunk) {
+ cookie_echo_chunk_ = std::move(chunk);
+ }
- // As above, but without passing in a builder and allowing sending many
- // packets.
+ // Called when the COOKIE ACK chunk has been received, to allow further
+ // packets to be sent.
+ void ClearCookieEchoChunk() { cookie_echo_chunk_ = absl::nullopt; }
+
+ bool has_cookie_echo_chunk() const { return cookie_echo_chunk_.has_value(); }
+
+ // Fills `builder` (which may already be filled with control chunks) with
+ // other control and data chunks, and sends packets as much as can be
+ // allowed by the congestion control algorithm.
+ void SendBufferedPackets(SctpPacket::Builder& builder, TimeMs now);
+
+ // As above, but without passing in a builder. If `cookie_echo_chunk_` is
+ // present, then only one packet will be sent, with this chunk as the first
+ // chunk.
void SendBufferedPackets(TimeMs now) {
SctpPacket::Builder builder(peer_verification_tag_, options_);
- SendBufferedPackets(builder, now, /*only_one_packet=*/false);
+ SendBufferedPackets(builder, now);
}
// Returns a textual representation of this object, for logging.
@@ -195,6 +207,13 @@
RetransmissionQueue retransmission_queue_;
StreamResetHandler stream_reset_handler_;
HeartbeatHandler heartbeat_handler_;
+
+ // Only valid when the socket state == State::kCookieEchoed. In this state,
+ // the socket must wait for COOKIE ACK to continue sending any packets (not
+ // including a COOKIE ECHO). So if `cookie_echo_chunk_` is present, the
+ // SendBufferedChunks will always only just send one packet, with this chunk
+ // as the first chunk in the packet.
+ absl::optional<CookieEchoChunk> cookie_echo_chunk_ = absl::nullopt;
};
} // namespace dcsctp
