diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc index 0c181996a2..71bc98c70d 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -191,7 +191,7 @@ bool DcSctpSocket::IsConsistent() const { 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 @@ void DcSctpSocket::InternalClose(ErrorKind error, absl::string_view message) { t1_cookie_->Stop(); t2_shutdown_->Stop(); tcb_ = nullptr; - cookie_echo_chunk_ = absl::nullopt; if (error == ErrorKind::kNoError) { callbacks_.OnClosed(); @@ -776,7 +775,7 @@ absl::optional DcSctpSocket::OnCookieTimerExpiry() { 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 @@ void DcSctpSocket::HandleInit(const CommonHeader& header, 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 @@ void DcSctpSocket::HandleInitAck( 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 @@ void DcSctpSocket::HandleCookieEcho( 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 @@ void DcSctpSocket::HandleCookieAck( // 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 592e2794c1..32e89b50d1 100644 --- a/net/dcsctp/socket/dcsctp_socket.h +++ b/net/dcsctp/socket/dcsctp_socket.h @@ -148,8 +148,6 @@ class DcSctpSocket : public DcSctpSocketInterface { 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 @@ class DcSctpSocket : public DcSctpSocketInterface { // 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 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 942085f68b..e5db12cd5a 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -494,6 +494,65 @@ TEST_F(DcSctpSocketTest, ResendingCookieEchoTooManyTimesAborts) { EXPECT_EQ(sock_a_.state(), SocketState::kClosed); } +TEST_F(DcSctpSocketTest, DoesntSendMorePacketsUntilCookieAckHasBeenReceived) { + sock_a_.Send(DcSctpMessage(StreamID(1), PPID(53), + std::vector(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 6e0be6a316..4fde40cee9 100644 --- a/net/dcsctp/socket/transmission_control_block.cc +++ b/net/dcsctp/socket/transmission_control_block.cc @@ -54,9 +54,15 @@ absl::optional TransmissionControlBlock::OnRtxTimerExpiry() { 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::MaybeSendSack() { } 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 @@ void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder, 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 5d3bfb884f..172f7c0c08 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 @@ class TransmissionControlBlock : public Context { // 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 @@ class TransmissionControlBlock : public Context { 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 cookie_echo_chunk_ = absl::nullopt; }; } // namespace dcsctp