commit 628f37a6fe24cd7783b3446d40b2e26752cc6ebb
author Harald Alvestrand <hta@webrtc.org> Thu Dec 06 09:55:20 2018
committer Commit Bot <commit-bot@chromium.org> Thu Dec 06 12:06:34 2018
tree 03348edcbbbdb8f0d414e2a6d53b9d8c392de61c
parent fafae11bfc4c7d1f8f02e708b0187fb48f7b8810

Delete a cricket::DtlsTransport when PC is closed

This avoids use-after-free problems that occur when references
to webrtc::DtlsTransport objects are held outside of the PC.

Bug: chromium:907849
Change-Id: Id428c8e616482eff0f4327d2eac17e29bb3f6484
Reviewed-on: https://webrtc-review.googlesource.com/c/113303
Reviewed-by: Fredrik Solenberg <solenberg@webrtc.org>
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25915}

pc/dtlstransport.h

diff --git a/pc/dtlstransport.h b/pc/dtlstransport.h
index dacdba2..03901c3 100644
--- a/pc/dtlstransport.h
+++ b/pc/dtlstransport.h
@@ -27,6 +27,7 @@
   cricket::DtlsTransportInternal* internal() {
     return internal_dtls_transport_.get();
   }
+  void clear() { internal_dtls_transport_.reset(); }
 
  private:
   std::unique_ptr<cricket::DtlsTransportInternal> internal_dtls_transport_;

pc/jseptransport.cc

diff --git a/pc/jseptransport.cc b/pc/jseptransport.cc
index c60c28d..28389d5 100644
--- a/pc/jseptransport.cc
+++ b/pc/jseptransport.cc
@@ -136,6 +136,12 @@
   if (media_transport_) {
     media_transport_->SetMediaTransportStateCallback(nullptr);
   }
+  // Clear all DtlsTransports. There may be pointers to these from
+  // other places, so we can't assume they'll be deleted by the destructor.
+  rtp_dtls_transport_->clear();
+  if (rtcp_dtls_transport_) {
+    rtcp_dtls_transport_->clear();
+  }
 }
 
 webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
@@ -192,9 +198,11 @@
     }
   }
 
+  RTC_DCHECK(rtp_dtls_transport_->internal());
   SetLocalIceParameters(rtp_dtls_transport_->internal()->ice_transport());
 
   if (rtcp_dtls_transport_) {
+    RTC_DCHECK(rtcp_dtls_transport_->internal());
     SetLocalIceParameters(rtcp_dtls_transport_->internal()->ice_transport());
   }
 
@@ -255,9 +263,11 @@
   }
 
   remote_description_.reset(new JsepTransportDescription(jsep_description));
+  RTC_DCHECK(rtp_dtls_transport_->internal());
   SetRemoteIceParameters(rtp_dtls_transport_->internal()->ice_transport());
 
   if (rtcp_dtls_transport_) {
+    RTC_DCHECK(rtcp_dtls_transport_->internal());
     SetRemoteIceParameters(rtcp_dtls_transport_->internal()->ice_transport());
   }
 
@@ -292,6 +302,7 @@
                               "Candidate has an unknown component: " +
                                   candidate.ToString() + " for mid " + mid());
     }
+    RTC_DCHECK(transport->internal() && transport->internal()->ice_transport());
     transport->internal()->ice_transport()->AddRemoteCandidate(candidate);
   }
   return webrtc::RTCError::OK();
@@ -306,6 +317,7 @@
 
 absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const {
   RTC_DCHECK(rtp_dtls_transport_);
+  RTC_DCHECK(rtp_dtls_transport_->internal());
   rtc::SSLRole dtls_role;
   if (!rtp_dtls_transport_->internal()->GetDtlsRole(&dtls_role)) {
     return absl::optional<rtc::SSLRole>();
@@ -317,8 +329,10 @@
 bool JsepTransport::GetStats(TransportStats* stats) {
   stats->transport_name = mid();
   stats->channel_stats.clear();
+  RTC_DCHECK(rtp_dtls_transport_->internal());
   bool ret = GetTransportStats(rtp_dtls_transport_->internal(), stats);
   if (rtcp_dtls_transport_) {
+    RTC_DCHECK(rtcp_dtls_transport_->internal());
     ret &= GetTransportStats(rtcp_dtls_transport_->internal(), stats);
   }
   return ret;
@@ -534,6 +548,7 @@
   // between future SetRemote/SetLocal invocations and new transport
   // creation, we have the negotiation state saved until a new
   // negotiation happens.
+  RTC_DCHECK(rtp_dtls_transport_->internal());
   webrtc::RTCError error = SetNegotiatedDtlsParameters(
       rtp_dtls_transport_->internal(), negotiated_dtls_role,
       remote_fingerprint.get());
@@ -542,6 +557,7 @@
   }
 
   if (rtcp_dtls_transport_) {
+    RTC_DCHECK(rtcp_dtls_transport_->internal());
     error = SetNegotiatedDtlsParameters(rtcp_dtls_transport_->internal(),
                                         negotiated_dtls_role,
                                         remote_fingerprint.get());

pc/jseptransportcontroller_unittest.cc

diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc
index 03ef2ea..fd8a779 100644
--- a/pc/jseptransportcontroller_unittest.cc
+++ b/pc/jseptransportcontroller_unittest.cc
@@ -385,6 +385,15 @@
   EXPECT_EQ(nullptr, transport_controller_->GetRtcpDtlsTransport(kVideoMid2));
   EXPECT_EQ(nullptr,
             transport_controller_->LookupDtlsTransportByMid(kVideoMid2));
+  // Take a pointer to a transport, shut down the transport controller,
+  // and verify that the resulting container is empty.
+  auto dtls_transport =
+      transport_controller_->LookupDtlsTransportByMid(kVideoMid1);
+  webrtc::DtlsTransport* my_transport =
+      static_cast<DtlsTransport*>(dtls_transport.get());
+  EXPECT_NE(nullptr, my_transport->internal());
+  transport_controller_.reset();
+  EXPECT_EQ(nullptr, my_transport->internal());
 }
 
 TEST_F(JsepTransportControllerTest, GetDtlsTransportWithRtcpMux) {