[Backport] Security bug 1184441

Manual backport of patch originally reviewed on
https://webrtc-review.googlesource.com/c/src/+/215060:
[Merge M86] - Reland "Fix race between destroying SctpTransport and receiving notification on timer thread."

This reverts commit 8a38b1cf681cd77f0d59a68fb45d8dedbd7d4cee.

Reason for reland: Problem was identified; has something to do with
the unique_ptr with the custom deleter.

Original change's description:
> Revert "Fix race between destroying SctpTransport and receiving notification on timer thread."
>
> This reverts commit a88fe7be146b9b85575504d4d5193c007f2e3de4.
>
> Reason for revert: Breaks downstream test, still investigating.
>
> Original change's description:
> > Fix race between destroying SctpTransport and receiving notification on timer thread.
> >
> > This gets rid of the SctpTransportMap::Retrieve method and forces
> > everything to go through PostToTransportThread, which behaves safely
> > with relation to the transport's destruction.
> >
> > Bug: webrtc:12467
> > Change-Id: Id4a723c2c985be2a368d2cc5c5e62deb04c509ab
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/208800
> > Reviewed-by: Niels Moller <nisse@webrtc.org>
> > Commit-Queue: Taylor <deadbeef@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#33364}
>
> TBR=nisse@webrtc.org
>
> Bug: webrtc:12467
> Change-Id: Ib5d815a2cbca4feb25f360bff7ed62c02d1910a0
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/209820
> Reviewed-by: Taylor <deadbeef@webrtc.org>
> Commit-Queue: Taylor <deadbeef@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#33386}

TBR=nisse@webrtc.org

Bug: webrtc:12467
Change-Id: I5f9fcd6df7a211e6edfa64577fc953833f4d9b79
Reviewed-by: Niels Moller <nisse@webrtc.org>
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Taylor <deadbeef@webrtc.org>
Cr-Original-Commit-Position: refs/heads/master@{#33427}
No-Try: True
No-Presubmit: True
Reviewed-by: Taylor <deadbeef@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Cr-Commit-Position: refs/branch-heads/4240@{#19}
Cr-Branched-From: 93a9d19d4eb53b3f4fb4d22e6c54f2e2824437eb-refs/heads/master@{#31969}
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

Author: Taylor Brandstetter

chromium/third_party/webrtc/media/sctp/sctp_transport.cc

@@ -20,6 +20,7 @@ enum PreservedErrno {
 // Successful return value from usrsctp callbacks. Is not actually used by
 // usrsctp, but all example programs for usrsctp use 1 as their return value.
 constexpr int kSctpSuccessReturn = 1;
+constexpr int kSctpErrorReturn = 0;
 
 }  // namespace
 
@@ -83,6 +84,19 @@ enum {
 // Should only be modified by UsrSctpWrapper.
 ABSL_CONST_INIT cricket::SctpTransportMap* g_transport_map_ = nullptr;
 
+// Helper that will call C's free automatically.
+// TODO(b/181900299): Figure out why unique_ptr with a custom deleter is causing
+// issues in a certain build environment.
+class AutoFreedPointer {
+ public:
+  explicit AutoFreedPointer(void* ptr) : ptr_(ptr) {}
+  AutoFreedPointer(AutoFreedPointer&& o) : ptr_(o.ptr_) { o.ptr_ = nullptr; }
+  ~AutoFreedPointer() { free(ptr_); }
+  void* get() const { return ptr_; }
+ private:
+  void* ptr_;
+};
+
 // Helper for logging SCTP messages.
 #if defined(__GNUC__)
 __attribute__((__format__(__printf__, 1, 2)))
@@ -239,32 +253,20 @@ class SctpTransportMap {
     return map_.erase(id) > 0;
   }
 
-  // Must be called on the transport's network thread to protect against
-  // simultaneous deletion/deregistration of the transport; if that's not
-  // guaranteed, use ExecuteWithLock.
-  SctpTransport* Retrieve(uintptr_t id) const {
-    webrtc::MutexLock lock(&lock_);
-    SctpTransport* transport = RetrieveWhileHoldingLock(id);
-    if (transport) {
-      RTC_DCHECK_RUN_ON(transport->network_thread());
-    }
-    return transport;
-  }
-
   // Posts |action| to the network thread of the transport identified by |id|
   // and returns true if found, all while holding a lock to protect against the
   // transport being simultaneously deleted/deregistered, or returns false if
   // not found.
-  bool PostToTransportThread(uintptr_t id,
-                             std::function<void(SctpTransport*)> action) const {
+  template <typename F>
+  bool PostToTransportThread(uintptr_t id, F action) const {
     webrtc::MutexLock lock(&lock_);
     SctpTransport* transport = RetrieveWhileHoldingLock(id);
     if (!transport) {
       return false;
     }
     transport->invoker_.AsyncInvoke<void>(
-        RTC_FROM_HERE, transport->network_thread_, [transport, action]() {
-      action(transport); });
+        RTC_FROM_HERE, transport->network_thread_,
+        [transport, action{std::move(action)}]() { action(transport); });
     return true;
   }
 
@@ -406,7 +408,7 @@ class SctpTransport::UsrSctpWrapper {
     if (!found) {
       RTC_LOG(LS_ERROR)
           << "OnSctpOutboundPacket: Failed to get transport for socket ID "
-          << addr;
+          << addr << "; possibly was already destroyed.";
       return EINVAL;
     }
     return 0;
@@ -423,28 +425,45 @@ class SctpTransport::UsrSctpWrapper {
                                  struct sctp_rcvinfo rcv,
                                  int flags,
                                  void* ulp_info) {
-    SctpTransport* transport = GetTransportFromSocket(sock);
-    if (!transport) {
+    AutoFreedPointer owned_data(data);
+    absl::optional<uintptr_t> id = GetTransportIdFromSocket(sock);
+    if (!id) {
       RTC_LOG(LS_ERROR)
-          << "OnSctpInboundPacket: Failed to get transport for socket " << sock
-          << "; possibly was already destroyed.";
-      free(data);
-      return 0;
+          << "OnSctpInboundPacket: Failed to get transport ID from socket "
+          << sock;
+      return kSctpErrorReturn;
+    }
+
+    if (!g_transport_map_) {
+      RTC_LOG(LS_ERROR)
+          << "OnSctpInboundPacket called after usrsctp uninitialized?";
+      return kSctpErrorReturn;
+    }
+    // PostsToTransportThread protects against the transport being
+    // simultaneously deregistered/deleted, since this callback may come from
+    // the SCTP timer thread and thus race with the network thread.
+    bool found = g_transport_map_->PostToTransportThread(
+        *id, [owned_data{std::move(owned_data)}, length, rcv,
+              flags](SctpTransport* transport) {
+          transport->OnDataOrNotificationFromSctp(owned_data.get(), length, rcv,
+                                                  flags);
+        });
+    if (!found) {
+      RTC_LOG(LS_ERROR)
+          << "OnSctpInboundPacket: Failed to get transport for socket ID "
+          << *id << "; possibly was already destroyed.";
+      return kSctpErrorReturn;
     }
-    // Sanity check that both methods of getting the SctpTransport pointer
-    // yield the same result.
-    RTC_CHECK_EQ(transport, static_cast<SctpTransport*>(ulp_info));
-    int result =
-        transport->OnDataOrNotificationFromSctp(data, length, rcv, flags);
-    free(data);
-    return result;
+    return kSctpSuccessReturn;
   }
 
-  static SctpTransport* GetTransportFromSocket(struct socket* sock) {
+  static absl::optional<uintptr_t> GetTransportIdFromSocket(
+      struct socket* sock) {
+    absl::optional<uintptr_t> ret;
     struct sockaddr* addrs = nullptr;
     int naddrs = usrsctp_getladdrs(sock, 0, &addrs);
     if (naddrs <= 0 || addrs[0].sa_family != AF_CONN) {
-      return nullptr;
+      return ret;
     }
     // usrsctp_getladdrs() returns the addresses bound to this socket, which
     // contains the SctpTransport id as sconn_addr.  Read the id,
@@ -453,17 +472,10 @@ class SctpTransport::UsrSctpWrapper {
     // id of the transport that created them, so [0] is as good as any other.
     struct sockaddr_conn* sconn =
         reinterpret_cast<struct sockaddr_conn*>(&addrs[0]);
-    if (!g_transport_map_) {
-      RTC_LOG(LS_ERROR)
-          << "GetTransportFromSocket called after usrsctp uninitialized?";
-      usrsctp_freeladdrs(addrs);
-      return nullptr;
-    }
-    SctpTransport* transport = g_transport_map_->Retrieve(
-        reinterpret_cast<uintptr_t>(sconn->sconn_addr));
+    ret = reinterpret_cast<uintptr_t>(sconn->sconn_addr);
     usrsctp_freeladdrs(addrs);
 
-    return transport;
+    return ret;
   }
 
   // TODO(crbug.com/webrtc/11899): This is a legacy callback signature, remove
@@ -472,14 +484,26 @@ class SctpTransport::UsrSctpWrapper {
     // Fired on our I/O thread. SctpTransport::OnPacketReceived() gets
     // a packet containing acknowledgments, which goes into usrsctp_conninput,
     // and then back here.
-    SctpTransport* transport = GetTransportFromSocket(sock);
-    if (!transport) {
+    absl::optional<uintptr_t> id = GetTransportIdFromSocket(sock);
+    if (!id) {
       RTC_LOG(LS_ERROR)
-          << "SendThresholdCallback: Failed to get transport for socket "
-          << sock << "; possibly was already destroyed.";
+          << "SendThresholdCallback: Failed to get transport ID from socket "
+          << sock;
       return 0;
     }
-    transport->OnSendThresholdCallback();
+    if (!g_transport_map_) {
+      RTC_LOG(LS_ERROR)
+          << "SendThresholdCallback called after usrsctp uninitialized?";
+      return 0;
+    }
+    bool found = g_transport_map_->PostToTransportThread(
+        *id,
+        [](SctpTransport* transport) { transport->OnSendThresholdCallback(); });
+    if (!found) {
+      RTC_LOG(LS_ERROR)
+          << "SendThresholdCallback: Failed to get transport for socket ID "
+          << *id << "; possibly was already destroyed.";
+    }
     return 0;
   }
 
@@ -489,17 +513,26 @@ class SctpTransport::UsrSctpWrapper {
     // Fired on our I/O thread. SctpTransport::OnPacketReceived() gets
     // a packet containing acknowledgments, which goes into usrsctp_conninput,
     // and then back here.
-    SctpTransport* transport = GetTransportFromSocket(sock);
-    if (!transport) {
+    absl::optional<uintptr_t> id = GetTransportIdFromSocket(sock);
+    if (!id) {
       RTC_LOG(LS_ERROR)
-          << "SendThresholdCallback: Failed to get transport for socket "
-          << sock << "; possibly was already destroyed.";
+          << "SendThresholdCallback: Failed to get transport ID from socket "
+          << sock;
       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();
+    if (!g_transport_map_) {
+      RTC_LOG(LS_ERROR)
+          << "SendThresholdCallback called after usrsctp uninitialized?";
+      return 0;
+    }
+    bool found = g_transport_map_->PostToTransportThread(
+        *id,
+        [](SctpTransport* transport) { transport->OnSendThresholdCallback(); });
+    if (!found) {
+      RTC_LOG(LS_ERROR)
+          << "SendThresholdCallback: Failed to get transport for socket ID "
+          << *id << "; possibly was already destroyed.";
+    }
     return 0;
   }
 };
@@ -1150,24 +1183,25 @@ void SctpTransport::OnPacketFromSctpToNetwork(
                          rtc::PacketOptions(), PF_NORMAL);
 }
 
-int SctpTransport::InjectDataOrNotificationFromSctpForTesting(
+void SctpTransport::InjectDataOrNotificationFromSctpForTesting(
     void* data,
     size_t length,
     struct sctp_rcvinfo rcv,
     int flags) {
-  return OnDataOrNotificationFromSctp(data, length, rcv, flags);
+  OnDataOrNotificationFromSctp(data, length, rcv, flags);
 }
 
-int SctpTransport::OnDataOrNotificationFromSctp(void* data,
-                                                size_t length,
-                                                struct sctp_rcvinfo rcv,
-                                                int flags) {
+void SctpTransport::OnDataOrNotificationFromSctp(void* data,
+                                                 size_t length,
+                                                 struct sctp_rcvinfo rcv,
+                                                 int flags) {
+  RTC_DCHECK_RUN_ON(network_thread_);
   // If data is NULL, the SCTP association has been closed.
   if (!data) {
     RTC_LOG(LS_INFO) << debug_name_
                      << "->OnDataOrNotificationFromSctp(...): "
                         "No data; association closed.";
-    return kSctpSuccessReturn;
+    return;
   }
 
   // Handle notifications early.
@@ -1180,13 +1214,10 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data,
         << "->OnDataOrNotificationFromSctp(...): SCTP notification"
         << " length=" << length;
 
-    // Copy and dispatch asynchronously
     rtc::CopyOnWriteBuffer notification(reinterpret_cast<uint8_t*>(data),
                                         length);
-    invoker_.AsyncInvoke<void>(
-        RTC_FROM_HERE, network_thread_,
-        rtc::Bind(&SctpTransport::OnNotificationFromSctp, this, notification));
-    return kSctpSuccessReturn;
+    OnNotificationFromSctp(notification);
+    return;
   }
 
   // Log data chunk
@@ -1204,7 +1235,7 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data,
     // Unexpected PPID, dropping
     RTC_LOG(LS_ERROR) << "Received an unknown PPID " << ppid
                       << " on an SCTP packet.  Dropping.";
-    return kSctpSuccessReturn;
+    return;
   }
 
   // Expect only continuation messages belonging to the same SID. The SCTP
@@ -1240,7 +1271,7 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data,
     if (partial_incoming_message_.size() < kSctpSendBufferSize) {
       // We still have space in the buffer. Continue buffering chunks until
       // the message is complete before handing it out.
-      return kSctpSuccessReturn;
+      return;
     } else {
       // The sender is exceeding the maximum message size that we announced.
       // Spit out a warning but still hand out the partial message. Note that
@@ -1254,17 +1285,11 @@ int SctpTransport::OnDataOrNotificationFromSctp(void* data,
     }
   }
 
-  // Dispatch the complete message.
-  // The ownership of the packet transfers to |invoker_|. Using
-  // CopyOnWriteBuffer is the most convenient way to do this.
-  invoker_.AsyncInvoke<void>(
-      RTC_FROM_HERE, network_thread_,
-      rtc::Bind(&SctpTransport::OnDataFromSctpToTransport, this, params,
-                partial_incoming_message_));
+  // Dispatch the complete message and reset the message buffer.
 
-  // Reset the message buffer
+  // Dispatch the complete message and reset the message buffer.
+  OnDataFromSctpToTransport(params, partial_incoming_message_);
   partial_incoming_message_.Clear();
-  return kSctpSuccessReturn;
 }
 
 void SctpTransport::OnDataFromSctpToTransport(

chromium/third_party/webrtc/media/sctp/sctp_transport.h

@@ -96,10 +96,10 @@ class SctpTransport : public SctpTransportInternal,
   void set_debug_name_for_testing(const char* debug_name) override {
     debug_name_ = debug_name;
   }
-  int InjectDataOrNotificationFromSctpForTesting(void* data,
-                                                 size_t length,
-                                                 struct sctp_rcvinfo rcv,
-                                                 int flags);
+  void InjectDataOrNotificationFromSctpForTesting(void* data,
+                                                  size_t length,
+                                                  struct sctp_rcvinfo rcv,
+                                                  int flags);
 
   // Exposed to allow Post call from c-callbacks.
   // TODO(deadbeef): Remove this or at least make it return a const pointer.
@@ -180,12 +180,12 @@ class SctpTransport : public SctpTransportInternal,
   // Called using |invoker_| to send packet on the network.
   void OnPacketFromSctpToNetwork(const rtc::CopyOnWriteBuffer& buffer);
 
-  // Called on the SCTP thread.
+  // Called on the network thread.
   // Flags are standard socket API flags (RFC 6458).
-  int OnDataOrNotificationFromSctp(void* data,
-                                   size_t length,
-                                   struct sctp_rcvinfo rcv,
-                                   int flags);
+  void OnDataOrNotificationFromSctp(void* data,
+                                    size_t length,
+                                    struct sctp_rcvinfo rcv,
+                                    int flags);
   // Called using |invoker_| to decide what to do with the data.
   void OnDataFromSctpToTransport(const ReceiveDataParams& params,
                                  const rtc::CopyOnWriteBuffer& buffer);