commit 914925f51e01f7cf3113db7153d582a0fa91977e
author Victor Boivie <boivie@webrtc.org> Fri May 07 09:22:50 2021
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri May 07 19:04:49 2021
tree 743e708583df61e640b0437ba11053f67b368586
parent cf2aeffdc2f91e868dca461cc9ccd65ece959331

dcsctp: Don't access TCB when the socket is closed

When the shutdown timer has expired, the socket will abort/close and the
TCB is not valid after InternalClose.

Bug: webrtc:12614
Change-Id: I09a94a049f0cda4577225dd9c80a92a8ec7e0423
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/217767
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33956}

net/dcsctp/socket/dcsctp_socket.cc

diff --git a/net/dcsctp/socket/dcsctp_socket.cc b/net/dcsctp/socket/dcsctp_socket.cc
index 622f6ec..bcff5be 100644
--- a/net/dcsctp/socket/dcsctp_socket.cc
+++ b/net/dcsctp/socket/dcsctp_socket.cc
@@ -763,12 +763,7 @@
                        << " has expired: " << t2_shutdown_->expiration_count()
                        << "/" << t2_shutdown_->options().max_restarts;
 
-  // https://tools.ietf.org/html/rfc4960#section-9.2
-  // "If the timer expires, the endpoint must resend the SHUTDOWN with the
-  // updated last sequential TSN received from its peer."
-  if (t2_shutdown_->is_running()) {
-    SendShutdown();
-  } else {
+  if (!t2_shutdown_->is_running()) {
     // https://tools.ietf.org/html/rfc4960#section-9.2
     // "An endpoint should limit the number of retransmissions of the SHUTDOWN
     // chunk to the protocol parameter 'Association.Max.Retrans'. If this
@@ -781,7 +776,14 @@
                              .Build())));
 
     InternalClose(ErrorKind::kTooManyRetries, "No SHUTDOWN_ACK received");
+    RTC_DCHECK(IsConsistent());
+    return absl::nullopt;
   }
+
+  // https://tools.ietf.org/html/rfc4960#section-9.2
+  // "If the timer expires, the endpoint must resend the SHUTDOWN with the
+  // updated last sequential TSN received from its peer."
+  SendShutdown();
   RTC_DCHECK(IsConsistent());
   return tcb_->current_rto();
 }

net/dcsctp/socket/dcsctp_socket_test.cc

diff --git a/net/dcsctp/socket/dcsctp_socket_test.cc b/net/dcsctp/socket/dcsctp_socket_test.cc
index a9eec60..b662b3e 100644
--- a/net/dcsctp/socket/dcsctp_socket_test.cc
+++ b/net/dcsctp/socket/dcsctp_socket_test.cc
@@ -30,6 +30,7 @@
 #include "net/dcsctp/packet/chunk/idata_chunk.h"
 #include "net/dcsctp/packet/chunk/init_chunk.h"
 #include "net/dcsctp/packet/chunk/sack_chunk.h"
+#include "net/dcsctp/packet/chunk/shutdown_chunk.h"
 #include "net/dcsctp/packet/error_cause/error_cause.h"
 #include "net/dcsctp/packet/error_cause/unrecognized_chunk_type_cause.h"
 #include "net/dcsctp/packet/parameter/heartbeat_info_parameter.h"
@@ -507,6 +508,37 @@
   EXPECT_EQ(sock_z_.state(), SocketState::kClosed);
 }
 
+TEST_F(DcSctpSocketTest, ShutdownTimerExpiresTooManyTimeClosesConnection) {
+  ConnectSockets();
+
+  sock_a_.Shutdown();
+  // Drop first SHUTDOWN packet.
+  cb_a_.ConsumeSentPacket();
+
+  EXPECT_EQ(sock_a_.state(), SocketState::kShuttingDown);
+
+  for (int i = 0; i < options_.max_retransmissions; ++i) {
+    AdvanceTime(DurationMs(options_.rto_initial * (1 << i)));
+    RunTimers();
+
+    // Dropping every shutdown chunk.
+    ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
+                                SctpPacket::Parse(cb_a_.ConsumeSentPacket()));
+    EXPECT_EQ(packet.descriptors()[0].type, ShutdownChunk::kType);
+    EXPECT_TRUE(cb_a_.ConsumeSentPacket().empty());
+  }
+  // The last expiry, makes it abort the connection.
+  AdvanceTime(options_.rto_initial * (1 << options_.max_retransmissions));
+  EXPECT_CALL(cb_a_, OnAborted).Times(1);
+  RunTimers();
+
+  EXPECT_EQ(sock_a_.state(), SocketState::kClosed);
+  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet,
+                              SctpPacket::Parse(cb_a_.ConsumeSentPacket()));
+  EXPECT_EQ(packet.descriptors()[0].type, AbortChunk::kType);
+  EXPECT_TRUE(cb_a_.ConsumeSentPacket().empty());
+}
+
 TEST_F(DcSctpSocketTest, EstablishConnectionWhileSendingData) {
   sock_a_.Connect();