From 49b20f919abe9a21227b0f6f86d223a3fb1e5ee4 Mon Sep 17 00:00:00 2001 From: Taylor Brandstetter Date: Tue, 19 Jan 2021 19:05:17 -0800 Subject: [PATCH] Fix race with SctpTransport destruction and usrsctp timer thread. The race occurs if the transport is being destroyed at the same time as a callback occurs on the usrsctp timer thread (for example, for a retransmission). Fixed by slightly extending the scope of mutex acquisition to include posting a task to the network thread, where it's safe to do further work. Bug: chromium:1162424 Change-Id: Ia25c96fa51cd4ba2d8690ba03de8af9e9f1605ea Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/202560 Reviewed-by: Harald Alvestrand Commit-Queue: Taylor Cr-Commit-Position: refs/heads/master@{#33048} --- media/sctp/sctp_transport.cc | 153 +++++++++++++++++++++-------------- media/sctp/sctp_transport.h | 4 + 2 files changed, 95 insertions(+), 62 deletions(-) diff --git a/media/sctp/sctp_transport.cc b/media/sctp/sctp_transport.cc index 6bb4a8fdf2..4a99144752 100644 --- a/media/sctp/sctp_transport.cc +++ b/media/sctp/sctp_transport.cc @@ -27,6 +27,7 @@ constexpr int kSctpSuccessReturn = 1; #include #include +#include #include #include @@ -81,58 +82,8 @@ enum { 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) { - webrtc::MutexLock lock(&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) { - webrtc::MutexLock lock(&lock_); - return map_.erase(id) > 0; - } - - cricket::SctpTransport* Retrieve(uintptr_t id) const { - webrtc::MutexLock lock(&lock_); - auto it = map_.find(id); - if (it == map_.end()) { - return nullptr; - } - return it->second; - } - - private: - mutable webrtc::Mutex lock_; - - uintptr_t next_id_ RTC_GUARDED_BY(lock_) = 0; - std::unordered_map map_ - RTC_GUARDED_BY(lock_); -}; - // Should only be modified by UsrSctpWrapper. -ABSL_CONST_INIT SctpTransportMap* g_transport_map_ = nullptr; +ABSL_CONST_INIT cricket::SctpTransportMap* g_transport_map_ = nullptr; // Helper for logging SCTP messages. #if defined(__GNUC__) @@ -258,6 +209,82 @@ sctp_sendv_spa CreateSctpSendParams(const cricket::SendDataParams& params) { namespace cricket { +// Maps SCTP transport ID to SctpTransport object, necessary in send threshold +// callback and outgoing packet callback. It also provides a facility to +// safely post a task to an SctpTransport's network thread from another thread. +class SctpTransportMap { + public: + SctpTransportMap() = default; + + // Assigns a new unused ID to the following transport. + uintptr_t Register(cricket::SctpTransport* transport) { + webrtc::MutexLock lock(&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) { + webrtc::MutexLock lock(&lock_); + 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 action) const { + webrtc::MutexLock lock(&lock_); + SctpTransport* transport = RetrieveWhileHoldingLock(id); + if (!transport) { + return false; + } + transport->network_thread_->PostTask(ToQueuedTask( + transport->task_safety_, [transport, action]() { action(transport); })); + return true; + } + + private: + SctpTransport* RetrieveWhileHoldingLock(uintptr_t id) const + RTC_EXCLUSIVE_LOCKS_REQUIRED(lock_) { + auto it = map_.find(id); + if (it == map_.end()) { + return nullptr; + } + return it->second; + } + + mutable webrtc::Mutex lock_; + + uintptr_t next_id_ RTC_GUARDED_BY(lock_) = 0; + std::unordered_map map_ RTC_GUARDED_BY(lock_); +}; + // Handles global init/deinit, and mapping from usrsctp callbacks to // SctpTransport calls. class SctpTransport::UsrSctpWrapper { @@ -370,14 +397,6 @@ class SctpTransport::UsrSctpWrapper { << "OnSctpOutboundPacket called after usrsctp uninitialized?"; return EINVAL; } - SctpTransport* transport = - g_transport_map_->Retrieve(reinterpret_cast(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 @@ -385,13 +404,23 @@ class SctpTransport::UsrSctpWrapper { << "; set_df: " << rtc::ToHex(set_df); VerboseLogPacket(data, length, SCTP_DUMP_OUTBOUND); + // Note: We have to copy the data; the caller will delete it. rtc::CopyOnWriteBuffer buf(reinterpret_cast(data), length); - transport->network_thread_->PostTask(ToQueuedTask( - transport->task_safety_, [transport, buf = std::move(buf)]() { + // 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( + reinterpret_cast(addr), [buf](SctpTransport* transport) { transport->OnPacketFromSctpToNetwork(buf); - })); + }); + if (!found) { + RTC_LOG(LS_ERROR) + << "OnSctpOutboundPacket: Failed to get transport for socket ID " + << addr; + return EINVAL; + } return 0; } diff --git a/media/sctp/sctp_transport.h b/media/sctp/sctp_transport.h index 38a89fcb61..bd166ef332 100644 --- a/media/sctp/sctp_transport.h +++ b/media/sctp/sctp_transport.h @@ -281,6 +281,8 @@ class SctpTransport : public SctpTransportInternal, // various callbacks. uintptr_t id_ = 0; + friend class SctpTransportMap; + RTC_DISALLOW_COPY_AND_ASSIGN(SctpTransport); }; @@ -299,6 +301,8 @@ class SctpTransportFactory : public webrtc::SctpTransportFactoryInterface { rtc::Thread* network_thread_; }; +class SctpTransportMap; + } // namespace cricket #endif // MEDIA_SCTP_SCTP_TRANSPORT_H_