Fixing possible crash due to RefCountedChannel assignment operator.

webrtc/p2p/base/transportcontroller.cc

 /*
  *  Copyright 2015 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
 #include "webrtc/p2p/base/transportcontroller.h"
 
 #include <algorithm>
 #include <memory>
 
 #include "webrtc/base/bind.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/thread.h"
 #include "webrtc/p2p/base/port.h"
 
-namespace cricket {
+namespace {
 
 enum {
   MSG_ICECONNECTIONSTATE,
   MSG_RECEIVING,
   MSG_ICEGATHERINGSTATE,
   MSG_CANDIDATESGATHERED,
 };
 
 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<TransportChannelImpl> ice_;
+  std::unique_ptr<TransportChannelImpl> dtls_;
+
+  RTC_DISALLOW_COPY_AND_ASSIGN(ChannelPair);
 };
 
 TransportController::TransportController(rtc::Thread* signaling_thread,
                                          rtc::Thread* network_thread,
                                          PortAllocator* port_allocator,
                                          bool redetermine_role_on_ice_restart)
     : signaling_thread_(signaling_thread),
       network_thread_(network_thread),
       port_allocator_(port_allocator),
       redetermine_role_on_ice_restart_(redetermine_role_on_ice_restart) {}
@@ skipping 171 matching lines @@
   dtls->SignalCandidateGathered.connect(
       this, &TransportController::OnChannelCandidateGathered_n);
   dtls->SignalCandidatesRemoved.connect(
       this, &TransportController::OnChannelCandidatesRemoved_n);
   dtls->SignalRoleConflict.connect(
       this, &TransportController::OnChannelRoleConflict_n);
   dtls->SignalStateChanged.connect(
       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.
   UpdateAggregateStates_n();
   return dtls;
 }
 
 void TransportController::DestroyTransportChannel_n(
     const std::string& transport_name,
     int component) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   auto it = GetChannelIterator_n(transport_name, component);
   if (it == channels_.end()) {
     LOG(LS_WARNING) << "Attempting to delete " << transport_name
                     << " TransportChannel " << component
                     << ", which doesn't exist.";
     return;
   }
-  it->DecRef();
-  if (it->ref() > 0) {
+  if ((*it)->Release() > 0) {
     return;
   }
   channels_.erase(it);
 
   JsepTransport* t = GetJsepTransport_n(transport_name);
   bool channel_removed = t->RemoveChannel(component);
   RTC_DCHECK(channel_removed);
   // Just as we create a Transport when its first channel is created,
   // we delete it when its last channel is deleted.
   if (!t->HasChannels()) {
     transports_.erase(transport_name);
   }
 
   // Removing a channel could cause aggregate state to change.
   UpdateAggregateStates_n();
 }
 
 std::vector<std::string> TransportController::transport_names_for_testing() {
   std::vector<std::string> ret;
   for (const auto& kv : transports_) {
     ret.push_back(kv.first);
   }
   return ret;
 }
 
 std::vector<TransportChannelImpl*> TransportController::channels_for_testing() {
   std::vector<TransportChannelImpl*> ret;
-  for (RefCountedChannel& channel : channels_) {
-    ret.push_back(channel.dtls());
+  for (RefCountedChannel* channel : channels_) {
+    ret.push_back(channel->dtls());
   }
   return ret;
 }
 
 TransportChannelImpl* TransportController::get_channel_for_testing(
     const std::string& transport_name,
     int component) {
   RefCountedChannel* ch = GetChannel_n(transport_name, component);
   return ch ? ch->dtls() : nullptr;
 }
@@ skipping 42 matching lines @@
       CandidatesData* data = static_cast<CandidatesData*>(pmsg->pdata);
       SignalCandidatesGathered(data->transport_name, data->candidates);
       delete data;
       break;
     }
     default:
       ASSERT(false);
   }
 }
 
-std::vector<TransportController::RefCountedChannel>::iterator
+std::vector<TransportController::RefCountedChannel*>::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<TransportController::RefCountedChannel>::const_iterator
+std::vector<TransportController::RefCountedChannel*>::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;
       });
 }
 
 const JsepTransport* TransportController::GetJsepTransport_n(
     const std::string& transport_name) const {
   RTC_DCHECK(network_thread_->IsCurrent());
   auto it = transports_.find(transport_name);
   return (it == transports_.end()) ? nullptr : it->second.get();
 }
 
 JsepTransport* TransportController::GetJsepTransport_n(
     const std::string& transport_name) {
   RTC_DCHECK(network_thread_->IsCurrent());
   auto it = transports_.find(transport_name);
   return (it == transports_.end()) ? nullptr : it->second.get();
 }
 
 const TransportController::RefCountedChannel* TransportController::GetChannel_n(
     const std::string& transport_name,
     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(
     const std::string& transport_name,
     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_n(
     const std::string& transport_name) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   JsepTransport* transport = GetJsepTransport_n(transport_name);
   if (transport) {
     return transport;
   }
 
   transport = new JsepTransport(transport_name, certificate_);
   transports_[transport_name] = std::unique_ptr<JsepTransport>(transport);
   return transport;
 }
 
 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) {
+    }

[pthatcher1, 2016/12/13 23:36:56]

Look at at refcountedobject.h, that does that exact same thing as "delete
channel", doesn't it?  

[Taylor Brandstetter, 2016/12/13 23:50:50]

The destructor isn't public, it's expected to only be destroyed by the ref count
going to 0. We probably can remove this code later and just assert that
channels_ is empty when the destructor is called.

+  }
   channels_.clear();
 }
 
 bool TransportController::SetSslMaxProtocolVersion_n(
     rtc::SSLProtocolVersion version) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   // Max SSL version can only be set before transports are created.
   if (!transports_.empty()) {
     return false;
   }
 
   ssl_max_version_ = version;
   return true;
 }
 
 void TransportController::SetIceConfig_n(const IceConfig& config) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   ice_config_ = config;
   for (auto& channel : channels_) {
-    channel.dtls()->SetIceConfig(ice_config_);
+    channel->dtls()->SetIceConfig(ice_config_);
   }
 }
 
 void TransportController::SetIceRole_n(IceRole ice_role) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   ice_role_ = ice_role;
   for (auto& channel : channels_) {
-    channel.dtls()->SetIceRole(ice_role_);
+    channel->dtls()->SetIceRole(ice_role_);
   }
 }
 
 bool TransportController::GetSslRole_n(const std::string& transport_name,
                                        rtc::SSLRole* role) const {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   const JsepTransport* t = GetJsepTransport_n(transport_name);
   if (!t) {
     return false;
@@ skipping 12 matching lines @@
   }
   certificate_ = certificate;
 
   // Set certificate both for Transport, which verifies it matches the
   // fingerprint in SDP...
   for (auto& kv : transports_) {
     kv.second->SetLocalCertificate(certificate_);
   }
   // ... 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;
 }
 
 bool TransportController::GetLocalCertificate_n(
     const std::string& transport_name,
     rtc::scoped_refptr<rtc::RTCCertificate>* certificate) const {
   RTC_DCHECK(network_thread_->IsCurrent());
 
@@ skipping 78 matching lines @@
     // description on a deleted transport.
     return true;
   }
 
   LOG(LS_INFO) << "Set remote transport description on " << transport_name;
   return transport->SetRemoteTransportDescription(tdesc, action, err);
 }
 
 void TransportController::MaybeStartGathering_n() {
   for (auto& channel : channels_) {
-    channel.dtls()->MaybeStartGathering();
+    channel->dtls()->MaybeStartGathering();
   }
 }
 
 bool TransportController::AddRemoteCandidates_n(
     const std::string& transport_name,
     const Candidates& candidates,
     std::string* err) {
   RTC_DCHECK(network_thread_->IsCurrent());
 
   // Verify each candidate before passing down to the transport layer.
@@ skipping 77 matching lines @@
     return false;
   }
   return transport->GetStats(stats);
 }
 
 void TransportController::SetMetricsObserver_n(
     webrtc::MetricsObserverInterface* metrics_observer) {
   RTC_DCHECK(network_thread_->IsCurrent());
   metrics_observer_ = metrics_observer;
   for (auto& channel : channels_) {
-    channel.dtls()->SetMetricsObserver(metrics_observer);
+    channel->dtls()->SetMetricsObserver(metrics_observer);
   }
 }
 
 void TransportController::OnChannelWritableState_n(
     rtc::PacketTransportInterface* transport) {
   RTC_DCHECK(network_thread_->IsCurrent());
   LOG(LS_INFO) << " TransportChannel " << transport->debug_name()
                << " writability changed to " << transport->writable() << ".";
   UpdateAggregateStates_n();
 }
@@ skipping 72 matching lines @@
 
   IceConnectionState new_connection_state = kIceConnectionConnecting;
   IceGatheringState new_gathering_state = kIceGatheringNew;
   bool any_receiving = false;
   bool any_failed = false;
   bool all_connected = !channels_.empty();
   bool all_completed = !channels_.empty();
   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) {
     new_connection_state = kIceConnectionFailed;
   } else if (all_completed) {
     new_connection_state = kIceConnectionCompleted;
   } else if (all_connected) {
     new_connection_state = kIceConnectionConnected;
   }
   if (connection_state_ != new_connection_state) {
@@ skipping 20 matching lines @@
         RTC_FROM_HERE, this, MSG_ICEGATHERINGSTATE,
         new rtc::TypedMessageData<IceGatheringState>(new_gathering_state));
   }
 }
 
 void TransportController::OnDtlsHandshakeError(rtc::SSLHandshakeError error) {
   SignalDtlsHandshakeError(error);
 }
 
 }  // namespace cricket