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

dcsctp: Don't sent more packets before COOKIE ACK

Browse Source 
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}
This commit is contained in:
Victor Boivie 2021-06-16 12:52:42 +02:00 committed by WebRTC LUCI CQ
parent 95c30413db
commit c20f1563b6
5 changed files with 120 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -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<DurationMs> 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();

6
net/dcsctp/socket/dcsctp_socket.h
View File

@ -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<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_;

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

@ -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<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();

23
net/dcsctp/socket/transmission_control_block.cc
View File

@ -54,10 +54,16 @@ absl::optional<DurationMs> TransmissionControlBlock::OnRtxTimerExpiry() {
TimeMs now = callbacks_.TimeMillis();
RTC_DLOG(LS_INFO) << log_prefix_ << "Timer " << t3_rtx_->name()
<< " has expired";
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;
}
}

41
net/dcsctp/socket/transmission_control_block.h
View File

@ -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<CookieEchoChunk> cookie_echo_chunk_ = absl::nullopt;
};
} // namespace dcsctp