Improve SCTP cookie generation
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Jun 2020 16:24:07 +0000 (16:24 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 17 Jun 2020 16:24:07 +0000 (16:24 +0000)
https://bugs.webkit.org/show_bug.cgi?id=213284
<rdar://problem/64438133>

Reviewed by Eric Carlson.

* Source/webrtc/media/sctp/sctp_transport.cc:
* Source/webrtc/media/sctp/sctp_transport.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@263154 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/ThirdParty/libwebrtc/ChangeLog
Source/ThirdParty/libwebrtc/Source/webrtc/media/sctp/sctp_transport.cc
Source/ThirdParty/libwebrtc/Source/webrtc/media/sctp/sctp_transport.h

index 62094af..7444c12 100644 (file)
@@ -1,3 +1,14 @@
+2020-06-17  Youenn Fablet  <youenn@apple.com>
+
+        Improve SCTP cookie generation
+        https://bugs.webkit.org/show_bug.cgi?id=213284
+        <rdar://problem/64438133>
+
+        Reviewed by Eric Carlson.
+
+        * Source/webrtc/media/sctp/sctp_transport.cc:
+        * Source/webrtc/media/sctp/sctp_transport.h:
+
 2020-06-12  David Kilzer  <ddkilzer@apple.com>
 
         [IPC hardening] Check enum values in IPC::Decoder::decodeEnum() an IPC::Encoder::encodeEnum()
index 40061a6..6be9461 100644 (file)
@@ -22,6 +22,7 @@ enum PreservedErrno {
 #include <stdio.h>
 
 #include <memory>
+#include <unordered_map>
 
 #include "absl/algorithm/container.h"
 #include "absl/base/attributes.h"
@@ -39,6 +40,7 @@ enum PreservedErrno {
 #include "rtc_base/logging.h"
 #include "rtc_base/numerics/safe_conversions.h"
 #include "rtc_base/string_utils.h"
+#include "rtc_base/thread_annotations.h"
 #include "rtc_base/thread_checker.h"
 #include "rtc_base/trace_event.h"
 #include "usrsctplib/usrsctp.h"
@@ -72,6 +74,59 @@ enum PayloadProtocolIdentifier {
   PPID_TEXT_LAST = 51
 };
 
+// Maps SCTP transport ID to SctpTransport object, necessary in send threshold
+// callback and outgoing packet callback.
+// TODO(crbug.com/1076703): Remove once the underlying problem is fixed or
+// workaround is provided in usrsctp.
+class SctpTransportMap {
+ public:
+  SctpTransportMap() = default;
+
+  // Assigns a new unused ID to the following transport.
+  uintptr_t Register(cricket::SctpTransport* transport) {
+    rtc::CritScope cs(&lock_);
+    // usrsctp_connect fails with a value of 0...
+    if (next_id_ == 0) {
+      ++next_id_;
+    }
+    // In case we've wrapped around and need to find an empty spot from a
+    // removed transport. Assumes we'll never be full.
+    while (map_.find(next_id_) != map_.end()) {
+      ++next_id_;
+      if (next_id_ == 0) {
+        ++next_id_;
+      }
+    };
+    map_[next_id_] = transport;
+    return next_id_++;
+  }
+
+  // Returns true if found.
+  bool Deregister(uintptr_t id) {
+    rtc::CritScope cs(&lock_);
+    return map_.erase(id) > 0;
+  }
+
+  cricket::SctpTransport* Retrieve(uintptr_t id) const {
+    rtc::CritScope cs(&lock_);
+    auto it = map_.find(id);
+    if (it == map_.end()) {
+      return nullptr;
+    }
+    return it->second;
+  }
+
+ private:
+  rtc::CriticalSection lock_;
+
+  uintptr_t next_id_ RTC_GUARDED_BY(lock_) = 0;
+  std::unordered_map<uintptr_t, cricket::SctpTransport*> map_
+      RTC_GUARDED_BY(lock_);
+};
+
+// Should only be modified by UsrSctpWrapper.
+ABSL_CONST_INIT SctpTransportMap* g_transport_map_ = nullptr;
+
 // Helper for logging SCTP messages.
 #if defined(__GNUC__)
 __attribute__((__format__(__printf__, 1, 2)))
@@ -242,9 +297,12 @@ class SctpTransport::UsrSctpWrapper {
     // Set the number of default outgoing streams. This is the number we'll
     // send in the SCTP INIT message.
     usrsctp_sysctl_set_sctp_nr_outgoing_streams_default(kMaxSctpStreams);
+
+    g_transport_map_ = new SctpTransportMap();
   }
 
   static void UninitializeUsrSctp() {
+    delete g_transport_map_;
     RTC_LOG(LS_INFO) << __FUNCTION__;
     // usrsctp_finish() may fail if it's called too soon after the transports
     // are
@@ -282,7 +340,14 @@ class SctpTransport::UsrSctpWrapper {
                                   size_t length,
                                   uint8_t tos,
                                   uint8_t set_df) {
-    SctpTransport* transport = static_cast<SctpTransport*>(addr);
+    SctpTransport* transport =
+        g_transport_map_->Retrieve(reinterpret_cast<uintptr_t>(addr));
+    if (!transport) {
+      RTC_LOG(LS_ERROR)
+          << "OnSctpOutboundPacket: Failed to get transport for socket ID "
+          << addr;
+      return EINVAL;
+    }
     RTC_LOG(LS_VERBOSE) << "global OnSctpOutboundPacket():"
                            "addr: "
                         << addr << "; length: " << length
@@ -392,14 +457,14 @@ class SctpTransport::UsrSctpWrapper {
       return nullptr;
     }
     // usrsctp_getladdrs() returns the addresses bound to this socket, which
-    // contains the SctpTransport* as sconn_addr.  Read the pointer,
+    // contains the SctpTransport id as sconn_addr.  Read the id,
     // then free the list of addresses once we have the pointer.  We only open
     // AF_CONN sockets, and they should all have the sconn_addr set to the
-    // pointer that created them, so [0] is as good as any other.
+    // 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]);
-    SctpTransport* transport =
-        reinterpret_cast<SctpTransport*>(sconn->sconn_addr);
+    SctpTransport* transport = g_transport_map_->Retrieve(
+        reinterpret_cast<uintptr_t>(sconn->sconn_addr));
     usrsctp_freeladdrs(addrs);
 
     return transport;
@@ -779,9 +844,10 @@ bool SctpTransport::OpenSctpSocket() {
     UsrSctpWrapper::DecrementUsrSctpUsageCount();
     return false;
   }
-  // Register this class as an address for usrsctp. This is used by SCTP to
+  id_ = g_transport_map_->Register(this);
+  // Register our id as an address for usrsctp. This is used by SCTP to
   // direct the packets received (by the created socket) to this class.
-  usrsctp_register_address(this);
+  usrsctp_register_address(reinterpret_cast<void*>(id_));
   return true;
 }
 
@@ -872,7 +938,8 @@ void SctpTransport::CloseSctpSocket() {
     // discarded instead of being sent.
     usrsctp_close(sock_);
     sock_ = nullptr;
-    usrsctp_deregister_address(this);
+    usrsctp_deregister_address(reinterpret_cast<void*>(id_));
+    RTC_CHECK(g_transport_map_->Deregister(id_));
     UsrSctpWrapper::DecrementUsrSctpUsageCount();
     ready_to_send_data_ = false;
   }
@@ -1003,7 +1070,7 @@ void SctpTransport::OnPacketRead(rtc::PacketTransportInternal* transport,
     // will be will be given to the global OnSctpInboundData, and then,
     // marshalled by the AsyncInvoker.
     VerboseLogPacket(data, len, SCTP_DUMP_INBOUND);
-    usrsctp_conninput(this, data, len, 0);
+    usrsctp_conninput(reinterpret_cast<void*>(id_), data, len, 0);
   } else {
     // TODO(ldixon): Consider caching the packet for very slightly better
     // reliability.
@@ -1033,7 +1100,7 @@ sockaddr_conn SctpTransport::GetSctpSockAddr(int port) {
 #endif
   // Note: conversion from int to uint16_t happens here.
   sconn.sconn_port = rtc::HostToNetwork16(port);
-  sconn.sconn_addr = this;
+  sconn.sconn_addr = reinterpret_cast<void*>(id_);
   return sconn;
 }
 
@@ -1182,6 +1249,9 @@ void SctpTransport::OnNotificationAssocChange(const sctp_assoc_change& change) {
       max_outbound_streams_ = change.sac_outbound_streams;
       max_inbound_streams_ = change.sac_inbound_streams;
       SignalAssociationChangeCommunicationUp();
+      // In case someone tried to close a stream before communication
+      // came up, send any queued resets.
+      SendQueuedStreamResets();
       break;
     case SCTP_COMM_LOST:
       RTC_LOG(LS_INFO) << "Association change SCTP_COMM_LOST";
index d346cfc..758503b 100644 (file)
@@ -13,6 +13,7 @@
 
 #include <errno.h>
 
+#include <cstdint>
 #include <map>
 #include <memory>
 #include <set>
@@ -267,6 +268,10 @@ class SctpTransport : public SctpTransportInternal,
   absl::optional<int> max_outbound_streams_;
   absl::optional<int> max_inbound_streams_;
 
+  // Used for associating this transport with the underlying sctp socket in
+  // various callbacks.
+  uintptr_t id_ = 0;
+
   RTC_DISALLOW_COPY_AND_ASSIGN(SctpTransport);
 };