From 9def3ff4a3112a21ff707f03bb677e2916c500d3 Mon Sep 17 00:00:00 2001
From: Taylor Brandstetter <deadbeef@webrtc.org>
Date: Thu, 24 Sep 2020 15:32:25 -0700
Subject: [PATCH] Fix for OnSctpInboundPacket being called after transport
 destruction.

OnSctpInboundPacket is called not only for incoming packets, but for
notifications, which can be delivered on the usrsctp timer thread.

I suspect that these notifications can be delivered after we attempt to
close the socket, because if we attempt to close it while the timer
thread holds a reference, it isn't actually destroyed until the timer
thread finishes its operation.

Bug: chromium:1127774
Change-Id: Id6a883b14796e8f5bf1c2990f3d9d389d72c8a46
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/184240
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Taylor <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32204}
---
 media/sctp/sctp_transport.cc | 41 +++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc
index 6dbce1369f..23c87debf8 100644
--- a/media/sctp/sctp_transport.cc
+++ b/media/sctp/sctp_transport.cc
@@ -393,11 +393,16 @@ class SctpTransport::UsrSctpWrapper {
                                  struct sctp_rcvinfo rcv,
                                  int flags,
                                  void* ulp_info) {
+    SctpTransport* transport = GetTransportFromSocket(sock);
+    if (!transport) {
+      RTC_LOG(LS_ERROR)
+          << "OnSctpInboundPacket: Failed to get transport for socket " << sock
+          << "; possibly was already destroyed.";
+      return 0;
+    }
     // Sanity check that both methods of getting the SctpTransport pointer
     // yield the same result.
-    RTC_CHECK_EQ(static_cast<SctpTransport*>(ulp_info),
-                 GetTransportFromSocket(sock));
-    SctpTransport* transport = static_cast<SctpTransport*>(ulp_info);
+    RTC_CHECK_EQ(transport, static_cast<SctpTransport*>(ulp_info));
     int result =
         transport->OnDataOrNotificationFromSctp(data, length, rcv, flags);
     free(data);
@@ -440,7 +445,7 @@ class SctpTransport::UsrSctpWrapper {
     if (!transport) {
       RTC_LOG(LS_ERROR)
           << "SendThresholdCallback: Failed to get transport for socket "
-          << sock;
+          << sock << "; possibly was already destroyed.";
       return 0;
     }
     transport->OnSendThresholdCallback();
@@ -450,14 +455,19 @@ class SctpTransport::UsrSctpWrapper {
   static int SendThresholdCallback(struct socket* sock,
                                    uint32_t sb_free,
                                    void* ulp_info) {
-    // Sanity check that both methods of getting the SctpTransport pointer
-    // yield the same result.
-    RTC_CHECK_EQ(static_cast<SctpTransport*>(ulp_info),
-                 GetTransportFromSocket(sock));
     // Fired on our I/O thread. SctpTransport::OnPacketReceived() gets
     // a packet containing acknowledgments, which goes into usrsctp_conninput,
     // and then back here.
-    SctpTransport* transport = static_cast<SctpTransport*>(ulp_info);
+    SctpTransport* transport = GetTransportFromSocket(sock);
+    if (!transport) {
+      RTC_LOG(LS_ERROR)
+          << "SendThresholdCallback: Failed to get transport for socket "
+          << sock << "; possibly was already destroyed.";
+      return 0;
+    }
+    // Sanity check that both methods of getting the SctpTransport pointer
+    // yield the same result.
+    RTC_CHECK_EQ(transport, static_cast<SctpTransport*>(ulp_info));
     transport->OnSendThresholdCallback();
     return 0;
   }
@@ -1122,8 +1132,8 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data,
   // If data is NULL, the SCTP association has been closed.
   if (!data) {
     RTC_LOG(LS_INFO) << debug_name_
-                     << "->OnSctpInboundPacket(...): "
-                        "No data, closing.";
+                     << "->OnDataOrNotificationFromSctp(...): "
+                        "No data; association closed.";
     return kSctpSuccessReturn;
   }
 
@@ -1132,9 +1142,10 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data,
   //       be handled early and entirely separate from the reassembly
   //       process.
   if (flags & MSG_NOTIFICATION) {
-    RTC_LOG(LS_VERBOSE) << debug_name_
-                        << "->OnSctpInboundPacket(...): SCTP notification"
-                        << " length=" << length;
+    RTC_LOG(LS_VERBOSE)
+        << debug_name_
+        << "->OnDataOrNotificationFromSctp(...): SCTP notification"
+        << " length=" << length;
 
     // Copy and dispatch asynchronously
     rtc::CopyOnWriteBuffer notification(reinterpret_cast<uint8_t*>(data),
@@ -1148,7 +1159,7 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data,
   // Log data chunk
   const uint32_t ppid = rtc::NetworkToHost32(rcv.rcv_ppid);
   RTC_LOG(LS_VERBOSE) << debug_name_
-                      << "->OnSctpInboundPacket(...): SCTP data chunk"
+                      << "->OnDataOrNotificationFromSctp(...): SCTP data chunk"
                       << " length=" << length << ", sid=" << rcv.rcv_sid
                       << ", ppid=" << ppid << ", ssn=" << rcv.rcv_ssn
                       << ", cum-tsn=" << rcv.rcv_cumtsn