From 62802a1b0e668a279268b5f2d3d1ea9c10869961 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Tue, 13 Dec 2016 16:38:36 -0800 Subject: [PATCH] Fixing possible crash due to RefCountedChannel assignment operator. We relied on the default destructor of RefCountedChannel to destroy its members in reverse initialization order (deleting the DTLS wrapper before the underlying ICE channel). However, std::vector also may use the default assignment operator, which performs a member-wise copy in initialization order. Which results in deleting the ICE channel before the DTLS one. This CL fixes this by using a vector of pointers instead of structures, and uses RefCountedObject to handle ref-counting. BUG=chromium:672951 Review-Url: https://codereview.webrtc.org/2571683004 Cr-Commit-Position: refs/heads/master@{#15583} --- webrtc/p2p/base/transportcontroller.cc | 107 +++++++++++++++++-------- webrtc/p2p/base/transportcontroller.h | 43 +++------- 2 files changed, 81 insertions(+), 69 deletions(-) diff --git a/webrtc/p2p/base/transportcontroller.cc b/webrtc/p2p/base/transportcontroller.cc index 4b24b1990d..01394a97aa 100644 --- a/webrtc/p2p/base/transportcontroller.cc +++ b/webrtc/p2p/base/transportcontroller.cc @@ -18,7 +18,7 @@ #include "webrtc/base/thread.h" #include "webrtc/p2p/base/port.h" -namespace cricket { +namespace { enum { MSG_ICECONNECTIONSTATE, @@ -29,11 +29,40 @@ enum { struct CandidatesData : public rtc::MessageData { CandidatesData(const std::string& transport_name, - const Candidates& candidates) + const cricket::Candidates& candidates) : transport_name(transport_name), candidates(candidates) {} std::string transport_name; - Candidates candidates; + cricket::Candidates candidates; +}; + +} // namespace { + +namespace cricket { + +// This class groups the DTLS and ICE channels, and helps keep track of +// how many external objects (BaseChannels) reference each channel. +class TransportController::ChannelPair { + public: + // TODO(deadbeef): Change the types of |dtls| and |ice| to + // DtlsTransportChannelWrapper and P2PTransportChannelWrapper, + // once TransportChannelImpl is removed. + ChannelPair(TransportChannelImpl* dtls, TransportChannelImpl* ice) + : ice_(ice), dtls_(dtls) {} + + // Currently, all ICE-related calls still go through this DTLS channel. But + // that will change once we get rid of TransportChannelImpl, and the DTLS + // channel interface no longer includes ICE-specific methods. + const TransportChannelImpl* dtls() const { return dtls_.get(); } + TransportChannelImpl* dtls() { return dtls_.get(); } + const TransportChannelImpl* ice() const { return ice_.get(); } + TransportChannelImpl* ice() { return ice_.get(); } + + private: + std::unique_ptr ice_; + std::unique_ptr dtls_; + + RTC_DISALLOW_COPY_AND_ASSIGN(ChannelPair); }; TransportController::TransportController(rtc::Thread* signaling_thread, @@ -240,7 +269,9 @@ TransportChannel* TransportController::CreateTransportChannel_n( this, &TransportController::OnChannelStateChanged_n); dtls->SignalDtlsHandshakeError.connect( this, &TransportController::OnDtlsHandshakeError); - channels_.insert(channels_.end(), RefCountedChannel(dtls, ice))->AddRef(); + RefCountedChannel* new_pair = new RefCountedChannel(dtls, ice); + new_pair->AddRef(); + channels_.insert(channels_.end(), new_pair); bool channel_added = transport->AddChannel(dtls, component); RTC_DCHECK(channel_added); // Adding a channel could cause aggregate state to change. @@ -260,8 +291,7 @@ void TransportController::DestroyTransportChannel_n( << ", which doesn't exist."; return; } - it->DecRef(); - if (it->ref() > 0) { + if ((*it)->Release() > 0) { return; } channels_.erase(it); @@ -289,8 +319,8 @@ std::vector TransportController::transport_names_for_testing() { std::vector TransportController::channels_for_testing() { std::vector ret; - for (RefCountedChannel& channel : channels_) { - ret.push_back(channel.dtls()); + for (RefCountedChannel* channel : channels_) { + ret.push_back(channel->dtls()); } return ret; } @@ -353,27 +383,27 @@ void TransportController::OnMessage(rtc::Message* pmsg) { } } -std::vector::iterator +std::vector::iterator TransportController::GetChannelIterator_n(const std::string& transport_name, int component) { RTC_DCHECK(network_thread_->IsCurrent()); - return std::find_if( - channels_.begin(), channels_.end(), - [transport_name, component](const RefCountedChannel& channel) { - return channel.dtls()->transport_name() == transport_name && - channel.dtls()->component() == component; - }); + return std::find_if(channels_.begin(), channels_.end(), + [transport_name, component](RefCountedChannel* channel) { + return channel->dtls()->transport_name() == + transport_name && + channel->dtls()->component() == component; + }); } -std::vector::const_iterator +std::vector::const_iterator TransportController::GetChannelIterator_n(const std::string& transport_name, int component) const { RTC_DCHECK(network_thread_->IsCurrent()); return std::find_if( channels_.begin(), channels_.end(), - [transport_name, component](const RefCountedChannel& channel) { - return channel.dtls()->transport_name() == transport_name && - channel.dtls()->component() == component; + [transport_name, component](const RefCountedChannel* channel) { + return channel->dtls()->transport_name() == transport_name && + channel->dtls()->component() == component; }); } @@ -394,7 +424,7 @@ const TransportController::RefCountedChannel* TransportController::GetChannel_n( int component) const { RTC_DCHECK(network_thread_->IsCurrent()); auto it = GetChannelIterator_n(transport_name, component); - return (it == channels_.end()) ? nullptr : &(*it); + return (it == channels_.end()) ? nullptr : *it; } TransportController::RefCountedChannel* TransportController::GetChannel_n( @@ -402,7 +432,7 @@ TransportController::RefCountedChannel* TransportController::GetChannel_n( int component) { RTC_DCHECK(network_thread_->IsCurrent()); auto it = GetChannelIterator_n(transport_name, component); - return (it == channels_.end()) ? nullptr : &(*it); + return (it == channels_.end()) ? nullptr : *it; } JsepTransport* TransportController::GetOrCreateJsepTransport( @@ -422,6 +452,13 @@ JsepTransport* TransportController::GetOrCreateJsepTransport( void TransportController::DestroyAllChannels_n() { RTC_DCHECK(network_thread_->IsCurrent()); transports_.clear(); + for (RefCountedChannel* channel : channels_) { + // Even though these objects are normally ref-counted, if + // TransportController is deleted while they still have references, just + // remove all references. + while (channel->Release() > 0) { + } + } channels_.clear(); } @@ -443,7 +480,7 @@ void TransportController::SetIceConfig_n(const IceConfig& config) { ice_config_ = config; for (auto& channel : channels_) { - channel.dtls()->SetIceConfig(ice_config_); + channel->dtls()->SetIceConfig(ice_config_); } } @@ -452,7 +489,7 @@ void TransportController::SetIceRole_n(IceRole ice_role) { ice_role_ = ice_role; for (auto& channel : channels_) { - channel.dtls()->SetIceRole(ice_role_); + channel->dtls()->SetIceRole(ice_role_); } } @@ -485,7 +522,7 @@ bool TransportController::SetLocalCertificate_n( } // ... and for the DTLS channel, which needs it for the DTLS handshake. for (auto& channel : channels_) { - bool set_cert_success = channel.dtls()->SetLocalCertificate(certificate); + bool set_cert_success = channel->dtls()->SetLocalCertificate(certificate); RTC_DCHECK(set_cert_success); } return true; @@ -584,7 +621,7 @@ bool TransportController::SetRemoteTransportDescription_n( void TransportController::MaybeStartGathering_n() { for (auto& channel : channels_) { - channel.dtls()->MaybeStartGathering(); + channel->dtls()->MaybeStartGathering(); } } @@ -682,7 +719,7 @@ void TransportController::SetMetricsObserver_n( RTC_DCHECK(network_thread_->IsCurrent()); metrics_observer_ = metrics_observer; for (auto& channel : channels_) { - channel.dtls()->SetMetricsObserver(metrics_observer); + channel->dtls()->SetMetricsObserver(metrics_observer); } } @@ -775,21 +812,21 @@ void TransportController::UpdateAggregateStates_n() { bool any_gathering = false; bool all_done_gathering = !channels_.empty(); for (const auto& channel : channels_) { - any_receiving = any_receiving || channel.dtls()->receiving(); + any_receiving = any_receiving || channel->dtls()->receiving(); any_failed = any_failed || - channel.dtls()->GetState() == TransportChannelState::STATE_FAILED; - all_connected = all_connected && channel.dtls()->writable(); + channel->dtls()->GetState() == TransportChannelState::STATE_FAILED; + all_connected = all_connected && channel->dtls()->writable(); all_completed = - all_completed && channel.dtls()->writable() && - channel.dtls()->GetState() == TransportChannelState::STATE_COMPLETED && - channel.dtls()->GetIceRole() == ICEROLE_CONTROLLING && - channel.dtls()->gathering_state() == kIceGatheringComplete; + all_completed && channel->dtls()->writable() && + channel->dtls()->GetState() == TransportChannelState::STATE_COMPLETED && + channel->dtls()->GetIceRole() == ICEROLE_CONTROLLING && + channel->dtls()->gathering_state() == kIceGatheringComplete; any_gathering = - any_gathering || channel.dtls()->gathering_state() != kIceGatheringNew; + any_gathering || channel->dtls()->gathering_state() != kIceGatheringNew; all_done_gathering = all_done_gathering && - channel.dtls()->gathering_state() == kIceGatheringComplete; + channel->dtls()->gathering_state() == kIceGatheringComplete; } if (any_failed) { diff --git a/webrtc/p2p/base/transportcontroller.h b/webrtc/p2p/base/transportcontroller.h index ea5ec80a90..a46ce00dbc 100644 --- a/webrtc/p2p/base/transportcontroller.h +++ b/webrtc/p2p/base/transportcontroller.h @@ -17,6 +17,8 @@ #include #include "webrtc/base/asyncinvoker.h" +#include "webrtc/base/constructormagic.h" +#include "webrtc/base/refcountedobject.h" #include "webrtc/base/sigslot.h" #include "webrtc/base/sslstreamadapter.h" #include "webrtc/p2p/base/candidate.h" @@ -173,44 +175,15 @@ class TransportController : public sigslot::has_slots<>, private: void OnMessage(rtc::Message* pmsg) override; - // This structure groups the DTLS and ICE channels, and helps keep track of - // how many external objects (BaseChannels) reference each channel. - struct RefCountedChannel { - RefCountedChannel() = default; - // TODO(deadbeef): Change the types of |dtls| and |ice| to - // DtlsTransportChannelWrapper and P2PTransportChannelWrapper, - // once TransportChannelImpl is removed. - explicit RefCountedChannel(TransportChannelImpl* dtls, - TransportChannelImpl* ice) - : ice_(ice), dtls_(dtls), ref_(0) {} - - void AddRef() { ++ref_; } - void DecRef() { - ASSERT(ref_ > 0); - --ref_; - } - int ref() const { return ref_; } - - // Currently, all ICE-related calls still go through this DTLS channel. But - // that will change once we get rid of TransportChannelImpl, and the DTLS - // channel interface no longer includes ICE-specific methods. - const TransportChannelImpl* dtls() const { return dtls_.get(); } - TransportChannelImpl* dtls() { return dtls_.get(); } - const TransportChannelImpl* ice() const { return ice_.get(); } - TransportChannelImpl* ice() { return ice_.get(); } - - private: - std::unique_ptr ice_; - std::unique_ptr dtls_; - int ref_ = 0; - }; + class ChannelPair; + typedef rtc::RefCountedObject RefCountedChannel; // Helper functions to get a channel or transport, or iterator to it (in case // it needs to be erased). - std::vector::iterator GetChannelIterator_n( + std::vector::iterator GetChannelIterator_n( const std::string& transport_name, int component); - std::vector::const_iterator GetChannelIterator_n( + std::vector::const_iterator GetChannelIterator_n( const std::string& transport_name, int component) const; const JsepTransport* GetJsepTransport( @@ -274,7 +247,7 @@ class TransportController : public sigslot::has_slots<>, PortAllocator* const port_allocator_ = nullptr; std::map> transports_; - std::vector channels_; + std::vector channels_; // Aggregate state for TransportChannelImpls. IceConnectionState connection_state_ = kIceConnectionConnecting; @@ -292,6 +265,8 @@ class TransportController : public sigslot::has_slots<>, bool quic_ = false; webrtc::MetricsObserverInterface* metrics_observer_ = nullptr; + + RTC_DISALLOW_COPY_AND_ASSIGN(TransportController); }; } // namespace cricket