From 628f37a6fe24cd7783b3446d40b2e26752cc6ebb Mon Sep 17 00:00:00 2001
From: Harald Alvestrand <hta@webrtc.org>
Date: Thu, 6 Dec 2018 10:55:20 +0100
Subject: [PATCH] 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                     |  1 +
 pc/jseptransport.cc                    | 16 ++++++++++++++++
 pc/jseptransportcontroller_unittest.cc |  9 +++++++++
 3 files changed, 26 insertions(+)

diff --git a/pc/dtlstransport.h b/pc/dtlstransport.h
index dacdba2b68..03901c33ff 100644
--- a/pc/dtlstransport.h
+++ b/pc/dtlstransport.h
@@ -27,6 +27,7 @@ class DtlsTransport : public DtlsTransportInterface {
   cricket::DtlsTransportInternal* internal() {
     return internal_dtls_transport_.get();
   }
+  void clear() { internal_dtls_transport_.reset(); }
 
  private:
   std::unique_ptr<cricket::DtlsTransportInternal> internal_dtls_transport_;
diff --git a/pc/jseptransport.cc b/pc/jseptransport.cc
index c60c28d738..28389d562e 100644
--- a/pc/jseptransport.cc
+++ b/pc/jseptransport.cc
@@ -136,6 +136,12 @@ JsepTransport::~JsepTransport() {
   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 @@ webrtc::RTCError JsepTransport::SetLocalJsepTransportDescription(
     }
   }
 
+  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 @@ webrtc::RTCError JsepTransport::SetRemoteJsepTransportDescription(
   }
 
   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 @@ webrtc::RTCError JsepTransport::AddRemoteCandidates(
                               "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 @@ void JsepTransport::SetNeedsIceRestartFlag() {
 
 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 @@ absl::optional<rtc::SSLRole> JsepTransport::GetDtlsRole() const {
 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 @@ webrtc::RTCError JsepTransport::NegotiateAndSetDtlsParameters(
   // 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 @@ webrtc::RTCError JsepTransport::NegotiateAndSetDtlsParameters(
   }
 
   if (rtcp_dtls_transport_) {
+    RTC_DCHECK(rtcp_dtls_transport_->internal());
     error = SetNegotiatedDtlsParameters(rtcp_dtls_transport_->internal(),
                                         negotiated_dtls_role,
                                         remote_fingerprint.get());
diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc
index 03ef2eae0f..fd8a779c78 100644
--- a/pc/jseptransportcontroller_unittest.cc
+++ b/pc/jseptransportcontroller_unittest.cc
@@ -385,6 +385,15 @@ TEST_F(JsepTransportControllerTest, GetDtlsTransport) {
   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) {