From 7bd3bc105d8e716f859bdc2246470610e007a095 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Terelius?= Date: Thu, 5 May 2022 14:44:25 +0000 Subject: [PATCH] Revert "dcsctp: Reset send queue when recreating TCB" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 3180a5ad0663900a39adf4b9974052c356c835fe. Reason for revert: Speculative revert due to failures in downstream tests. Original change's description: > dcsctp: Reset send queue when recreating TCB > > This is an issue found in fuzzer, and doesn't really happen in WebRTC > as it never closes the connection and reconnects. > > The issue is that the send queue outlives any connection since you're > allowed to send messages (well, enqueue them) before the association is > fully connected. So the send queue is always present but the TCB > (information about the connection) is torn down when the connection is > closed for example. And the TCB holds the Stream Reset handler, which is > responsible for e.g. keeping track of stream reset sequence numbers and > such - which is tied to the connection. > > So to ensure that the Stream Reset Handler is in charge of deciding > if a stream reset is taking place, make sure that the send queue is in > a known good state when the Stream Reset handler is created. > > Bug: webrtc:13994, chromium:1320194 > Change-Id: I940e4690ac9237ac99dd69a9ffc060cdac61711d > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261260 > Reviewed-by: Harald Alvestrand > Commit-Queue: Victor Boivie > Cr-Commit-Position: refs/heads/main@{#36779} Bug: webrtc:13994, chromium:1320194 Change-Id: I89bb9cae60adc53902c1304e79047d18e72594a5 No-Presubmit: true No-Tree-Checks: true No-Try: true Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261302 Reviewed-by: Harald Alvestrand Auto-Submit: Björn Terelius Bot-Commit: rubber-stamper@appspot.gserviceaccount.com Owners-Override: Björn Terelius Reviewed-by: Victor Boivie Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#36783} --- net/dcsctp/socket/dcsctp_socket.cc | 3 ++ net/dcsctp/socket/dcsctp_socket_test.cc | 28 +------------------ .../socket/transmission_control_block.h | 9 +----- 3 files changed, 5 insertions(+), 35 deletions(-) diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc index 4f445fb6f8..cc9a71eae8 100644 --- a/net/dcsctp/socket/dcsctp_socket.cc +++ b/net/dcsctp/socket/dcsctp_socket.cc @@ -1305,6 +1305,9 @@ bool DcSctpSocket::HandleCookieEchoWithTCB(const CommonHeader& header, RTC_DLOG(LS_VERBOSE) << log_prefix() << "Received COOKIE-ECHO indicating a restarted peer"; + // If a message was partly sent, and the peer restarted, resend it in + // full by resetting the send queue. + send_queue_.Reset(); tcb_ = nullptr; callbacks_.OnConnectionRestarted(); } else if (header.verification_tag == tcb_->my_verification_tag() && diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc index 4c03acab45..914bea34c4 100644 --- a/net/dcsctp/socket/dcsctp_socket_test.cc +++ b/net/dcsctp/socket/dcsctp_socket_test.cc @@ -2341,32 +2341,6 @@ TEST(DcSctpSocketTest, CloseStreamsWithPendingRequest) { absl::optional msg6 = z.cb.ConsumeReceivedMessage(); ASSERT_TRUE(msg6.has_value()); EXPECT_EQ(msg6->stream_id(), StreamID(3)); -} - -TEST(DcSctpSocketTest, ReconnectSocketWithPendingStreamReset) { - // This is an issue found by fuzzing, and doesn't really make sense in WebRTC - // data channels as a SCTP connection is never ever closed and then - // reconnected. SCTP connections are closed when the peer connection is - // deleted, and then it doesn't do more with SCTP. - SocketUnderTest a("A"); - SocketUnderTest z("Z"); - - ConnectSockets(a, z); - - a.socket.ResetStreams(std::vector({StreamID(1)})); - - // EXPECT_CALL(a.cb, OnAborted).Times(1); - EXPECT_CALL(z.cb, OnAborted).Times(1); - a.socket.Close(); - - EXPECT_EQ(a.socket.state(), SocketState::kClosed); - - EXPECT_CALL(a.cb, OnConnected).Times(1); - EXPECT_CALL(z.cb, OnConnected).Times(1); - a.socket.Connect(); - ExchangeMessages(a, z); - a.socket.ResetStreams(std::vector({StreamID(2)})); -} - +} // namespace } // namespace } // namespace dcsctp diff --git a/net/dcsctp/socket/transmission_control_block.h b/net/dcsctp/socket/transmission_control_block.h index 0971c67317..8c240f1043 100644 --- a/net/dcsctp/socket/transmission_control_block.h +++ b/net/dcsctp/socket/transmission_control_block.h @@ -118,14 +118,7 @@ class TransmissionControlBlock : public Context { &reassembly_queue_, &retransmission_queue_, handover_state), - heartbeat_handler_(log_prefix, options, this, &timer_manager_) { - // If the connection is re-established (peer restarted, but re-used old - // connection), make sure that all message identifiers are reset and any - // partly sent message is re-sent in full. The same is true when the socket - // is closed and later re-opened, which never happens in WebRTC, but is a - // valid operation on the SCTP level. - send_queue.Reset(); - } + heartbeat_handler_(log_prefix, options, this, &timer_manager_) {} // Implementation of `Context`. bool is_connection_established() const override {