diff --git a/p2p/base/datagram_dtls_adaptor.cc b/p2p/base/datagram_dtls_adaptor.cc index 344f1c1e07..f62cfbf91c 100644 --- a/p2p/base/datagram_dtls_adaptor.cc +++ b/p2p/base/datagram_dtls_adaptor.cc @@ -44,12 +44,12 @@ constexpr bool kBypassDatagramDtlsTestOnly = false; namespace cricket { DatagramDtlsAdaptor::DatagramDtlsAdaptor( - std::unique_ptr ice_transport, + IceTransportInternal* ice_transport, std::unique_ptr datagram_transport, const webrtc::CryptoOptions& crypto_options, webrtc::RtcEventLog* event_log) : crypto_options_(crypto_options), - ice_transport_(std::move(ice_transport)), + ice_transport_(ice_transport), datagram_transport_(std::move(datagram_transport)), event_log_(event_log) { RTC_DCHECK(ice_transport_); @@ -91,7 +91,6 @@ DatagramDtlsAdaptor::~DatagramDtlsAdaptor() { // Make sure datagram transport is destroyed before ICE. datagram_transport_.reset(); - ice_transport_.reset(); } const webrtc::CryptoOptions& DatagramDtlsAdaptor::crypto_options() const { @@ -127,7 +126,7 @@ void DatagramDtlsAdaptor::OnReadPacket(rtc::PacketTransportInternal* transport, RTC_DCHECK(kBypassDatagramDtlsTestOnly); RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DCHECK_EQ(transport, ice_transport_.get()); + RTC_DCHECK_EQ(transport, ice_transport_); RTC_DCHECK(flags == 0); PropagateReadPacket( @@ -245,7 +244,7 @@ bool DatagramDtlsAdaptor::SetSslMaxProtocolVersion( } IceTransportInternal* DatagramDtlsAdaptor::ice_transport() { - return ice_transport_.get(); + return ice_transport_; } webrtc::DatagramTransportInterface* DatagramDtlsAdaptor::datagram_transport() { @@ -264,7 +263,7 @@ void DatagramDtlsAdaptor::OnReadyToSend( void DatagramDtlsAdaptor::OnWritableState( rtc::PacketTransportInternal* transport) { RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DCHECK(transport == ice_transport_.get()); + RTC_DCHECK(transport == ice_transport_); RTC_LOG(LS_VERBOSE) << ": ice_transport writable state changed to " << ice_transport_->writable(); @@ -352,7 +351,7 @@ void DatagramDtlsAdaptor::OnNetworkRouteChanged( void DatagramDtlsAdaptor::OnReceivingState( rtc::PacketTransportInternal* transport) { RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DCHECK(transport == ice_transport_.get()); + RTC_DCHECK(transport == ice_transport_); RTC_LOG(LS_VERBOSE) << "ice_transport receiving state changed to " << ice_transport_->receiving(); diff --git a/p2p/base/datagram_dtls_adaptor.h b/p2p/base/datagram_dtls_adaptor.h index 2f6fdc1236..c1014cfff8 100644 --- a/p2p/base/datagram_dtls_adaptor.h +++ b/p2p/base/datagram_dtls_adaptor.h @@ -43,7 +43,7 @@ class DatagramDtlsAdaptor : public DtlsTransportInternal, // has a virtual getter crypto_options(). Consider removing getter and // removing crypto_options from DatagramDtlsAdaptor. DatagramDtlsAdaptor( - std::unique_ptr ice_transport, + IceTransportInternal* ice_transport, std::unique_ptr datagram_transport, const webrtc::CryptoOptions& crypto_options, webrtc::RtcEventLog* event_log); @@ -130,7 +130,7 @@ class DatagramDtlsAdaptor : public DtlsTransportInternal, rtc::ThreadChecker thread_checker_; webrtc::CryptoOptions crypto_options_; - std::unique_ptr ice_transport_; + IceTransportInternal* ice_transport_; std::unique_ptr datagram_transport_; diff --git a/p2p/base/dtls_transport.cc b/p2p/base/dtls_transport.cc index 46f0f99520..8a8fca2644 100644 --- a/p2p/base/dtls_transport.cc +++ b/p2p/base/dtls_transport.cc @@ -118,13 +118,12 @@ void StreamInterfaceChannel::Close() { state_ = rtc::SS_CLOSED; } -DtlsTransport::DtlsTransport( - std::unique_ptr ice_transport, - const webrtc::CryptoOptions& crypto_options, - webrtc::RtcEventLog* event_log) +DtlsTransport::DtlsTransport(IceTransportInternal* ice_transport, + const webrtc::CryptoOptions& crypto_options, + webrtc::RtcEventLog* event_log) : transport_name_(ice_transport->transport_name()), component_(ice_transport->component()), - ice_transport_(std::move(ice_transport)), + ice_transport_(ice_transport), downward_(NULL), srtp_ciphers_(crypto_options.GetSupportedDtlsSrtpCryptoSuites()), ssl_max_version_(rtc::SSL_PROTOCOL_DTLS_12), @@ -328,8 +327,7 @@ bool DtlsTransport::ExportKeyingMaterial(const std::string& label, bool DtlsTransport::SetupDtls() { RTC_DCHECK(dtls_role_); - StreamInterfaceChannel* downward = - new StreamInterfaceChannel(ice_transport_.get()); + StreamInterfaceChannel* downward = new StreamInterfaceChannel(ice_transport_); dtls_.reset(rtc::SSLStreamAdapter::Create(downward)); if (!dtls_) { @@ -425,7 +423,7 @@ int DtlsTransport::SendPacket(const char* data, } IceTransportInternal* DtlsTransport::ice_transport() { - return ice_transport_.get(); + return ice_transport_; } bool DtlsTransport::IsDtlsConnected() { @@ -482,7 +480,7 @@ void DtlsTransport::ConnectToIceTransport() { // impl again void DtlsTransport::OnWritableState(rtc::PacketTransportInternal* transport) { RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DCHECK(transport == ice_transport_.get()); + RTC_DCHECK(transport == ice_transport_); RTC_LOG(LS_VERBOSE) << ToString() << ": ice_transport writable state changed to " << ice_transport_->writable(); @@ -514,7 +512,7 @@ void DtlsTransport::OnWritableState(rtc::PacketTransportInternal* transport) { void DtlsTransport::OnReceivingState(rtc::PacketTransportInternal* transport) { RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DCHECK(transport == ice_transport_.get()); + RTC_DCHECK(transport == ice_transport_); RTC_LOG(LS_VERBOSE) << ToString() << ": ice_transport " "receiving state changed to " @@ -531,7 +529,7 @@ void DtlsTransport::OnReadPacket(rtc::PacketTransportInternal* transport, const int64_t& packet_time_us, int flags) { RTC_DCHECK_RUN_ON(&thread_checker_); - RTC_DCHECK(transport == ice_transport_.get()); + RTC_DCHECK(transport == ice_transport_); RTC_DCHECK(flags == 0); if (!dtls_active_) { diff --git a/p2p/base/dtls_transport.h b/p2p/base/dtls_transport.h index ceef1dcf32..bf3e056bea 100644 --- a/p2p/base/dtls_transport.h +++ b/p2p/base/dtls_transport.h @@ -91,14 +91,15 @@ class StreamInterfaceChannel : public rtc::StreamInterface { // as the constructor. class DtlsTransport : public DtlsTransportInternal { public: - // |ice_transport| is the ICE transport this DTLS transport is wrapping. + // |ice_transport| is the ICE transport this DTLS transport is wrapping. It + // must outlive this DTLS transport. // // |crypto_options| are the options used for the DTLS handshake. This affects // whether GCM crypto suites are negotiated. // // |event_log| is an optional RtcEventLog for logging state changes. It should // outlive the DtlsTransport. - explicit DtlsTransport(std::unique_ptr ice_transport, + explicit DtlsTransport(IceTransportInternal* ice_transport, const webrtc::CryptoOptions& crypto_options, webrtc::RtcEventLog* event_log); @@ -222,8 +223,8 @@ class DtlsTransport : public DtlsTransportInternal { std::string transport_name_; int component_; DtlsTransportState dtls_state_ = DTLS_TRANSPORT_NEW; - // Underlying ice_transport, owned by this class. - std::unique_ptr ice_transport_; + // Underlying ice_transport, not owned by this class. + IceTransportInternal* ice_transport_; std::unique_ptr dtls_; // The DTLS stream StreamInterfaceChannel* downward_; // Wrapper for ice_transport_, owned by dtls_. diff --git a/p2p/base/dtls_transport_unittest.cc b/p2p/base/dtls_transport_unittest.cc index b71d65e9c2..3c1cd2f685 100644 --- a/p2p/base/dtls_transport_unittest.cc +++ b/p2p/base/dtls_transport_unittest.cc @@ -77,18 +77,18 @@ class DtlsTestClient : public sigslot::has_slots<> { } // Set up fake ICE transport and real DTLS transport under test. void SetupTransports(IceRole role, int async_delay_ms = 0) { - std::unique_ptr fake_ice_transport; - fake_ice_transport.reset(new FakeIceTransport("fake", 0)); - fake_ice_transport->SetAsync(true); - fake_ice_transport->SetAsyncDelay(async_delay_ms); - fake_ice_transport->SetIceRole(role); - fake_ice_transport->SetIceTiebreaker((role == ICEROLE_CONTROLLING) ? 1 : 2); + fake_ice_transport_.reset(new FakeIceTransport("fake", 0)); + fake_ice_transport_->SetAsync(true); + fake_ice_transport_->SetAsyncDelay(async_delay_ms); + fake_ice_transport_->SetIceRole(role); + fake_ice_transport_->SetIceTiebreaker((role == ICEROLE_CONTROLLING) ? 1 + : 2); // Hook the raw packets so that we can verify they are encrypted. - fake_ice_transport->SignalReadPacket.connect( + fake_ice_transport_->SignalReadPacket.connect( this, &DtlsTestClient::OnFakeIceTransportReadPacket); dtls_transport_ = absl::make_unique( - std::move(fake_ice_transport), webrtc::CryptoOptions(), + fake_ice_transport_.get(), webrtc::CryptoOptions(), /*event_log=*/nullptr); dtls_transport_->SetSslMaxProtocolVersion(ssl_max_version_); // Note: Certificate may be null here if testing passthrough. diff --git a/p2p/base/no_op_dtls_transport.cc b/p2p/base/no_op_dtls_transport.cc index aef3fae55a..dda668b25c 100644 --- a/p2p/base/no_op_dtls_transport.cc +++ b/p2p/base/no_op_dtls_transport.cc @@ -32,10 +32,10 @@ namespace cricket { NoOpDtlsTransport::NoOpDtlsTransport( - std::unique_ptr ice_transport, + IceTransportInternal* ice_transport, const webrtc::CryptoOptions& crypto_options) : crypto_options_(webrtc::CryptoOptions::NoGcm()), - ice_transport_(std::move(ice_transport)) { + ice_transport_(ice_transport) { RTC_DCHECK(ice_transport_); ice_transport_->SignalWritableState.connect( this, &NoOpDtlsTransport::OnWritableState); @@ -102,7 +102,7 @@ bool NoOpDtlsTransport::SetSslMaxProtocolVersion( return true; } IceTransportInternal* NoOpDtlsTransport::ice_transport() { - return ice_transport_.get(); + return ice_transport_; } void NoOpDtlsTransport::OnReadyToSend(rtc::PacketTransportInternal* transport) { diff --git a/p2p/base/no_op_dtls_transport.h b/p2p/base/no_op_dtls_transport.h index 3b6252bc3c..7111b29988 100644 --- a/p2p/base/no_op_dtls_transport.h +++ b/p2p/base/no_op_dtls_transport.h @@ -54,7 +54,7 @@ constexpr int kNoOpDtlsTransportComponent = -1; // unprotect RTCP packet". class NoOpDtlsTransport : public DtlsTransportInternal { public: - NoOpDtlsTransport(std::unique_ptr ice_transport, + NoOpDtlsTransport(IceTransportInternal* ice_transport, const webrtc::CryptoOptions& crypto_options); ~NoOpDtlsTransport() override; @@ -102,7 +102,7 @@ class NoOpDtlsTransport : public DtlsTransportInternal { rtc::ThreadChecker thread_checker_; webrtc::CryptoOptions crypto_options_; - std::unique_ptr ice_transport_; + IceTransportInternal* ice_transport_; bool is_writable_ = false; }; diff --git a/p2p/base/transport_factory_interface.h b/p2p/base/transport_factory_interface.h index 6da55345d2..e7eead7827 100644 --- a/p2p/base/transport_factory_interface.h +++ b/p2p/base/transport_factory_interface.h @@ -33,7 +33,7 @@ class TransportFactoryInterface { int component) = 0; virtual std::unique_ptr CreateDtlsTransport( - std::unique_ptr ice, + IceTransportInternal* ice, const webrtc::CryptoOptions& crypto_options) = 0; }; diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 26311d19db..3098681cb9 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -93,6 +93,8 @@ JsepTransportDescription& JsepTransportDescription::operator=( JsepTransport::JsepTransport( const std::string& mid, const rtc::scoped_refptr& local_certificate, + std::unique_ptr ice_transport, + std::unique_ptr rtcp_ice_transport, std::unique_ptr unencrypted_rtp_transport, std::unique_ptr sdes_transport, std::unique_ptr dtls_srtp_transport, @@ -102,6 +104,8 @@ JsepTransport::JsepTransport( : network_thread_(rtc::Thread::Current()), mid_(mid), local_certificate_(local_certificate), + ice_transport_(std::move(ice_transport)), + rtcp_ice_transport_(std::move(rtcp_ice_transport)), unencrypted_rtp_transport_(std::move(unencrypted_rtp_transport)), sdes_transport_(std::move(sdes_transport)), dtls_srtp_transport_(std::move(dtls_srtp_transport)), @@ -115,7 +119,12 @@ JsepTransport::JsepTransport( std::move(rtcp_dtls_transport)) : nullptr), media_transport_(std::move(media_transport)) { + RTC_DCHECK(ice_transport_); RTC_DCHECK(rtp_dtls_transport_); + // |rtcp_ice_transport_| must be present iff |rtcp_dtls_transport_| is + // present. + RTC_DCHECK_EQ((rtcp_ice_transport_ != nullptr), + (rtcp_dtls_transport_ != nullptr)); RTC_DCHECK(!datagram_transport() || !media_transport_); // Verify the "only one out of these three can be set" invariant. if (unencrypted_rtp_transport_) { diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index fce21be6e6..9add2e5778 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -86,6 +86,8 @@ class JsepTransport : public sigslot::has_slots<>, JsepTransport( const std::string& mid, const rtc::scoped_refptr& local_certificate, + std::unique_ptr ice_transport, + std::unique_ptr rtcp_ice_transport, std::unique_ptr unencrypted_rtp_transport, std::unique_ptr sdes_transport, std::unique_ptr dtls_srtp_transport, @@ -310,6 +312,11 @@ class JsepTransport : public sigslot::has_slots<>, std::unique_ptr remote_description_ RTC_GUARDED_BY(network_thread_); + // Ice transport which may be used by any of upper-layer transports (below). + // Owned by JsepTransport and guaranteed to outlive the transports below. + const std::unique_ptr ice_transport_; + const std::unique_ptr rtcp_ice_transport_; + // To avoid downcasting and make it type safe, keep three unique pointers for // different SRTP mode and only one of these is non-nullptr. // Since these are const, the variables don't need locks; diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 55f1d1cfdb..4f14e00af2 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -474,7 +474,7 @@ JsepTransportController::CreateIceTransport(const std::string transport_name, std::unique_ptr JsepTransportController::CreateDtlsTransport( - std::unique_ptr ice, + cricket::IceTransportInternal* ice, std::unique_ptr datagram_transport) { RTC_DCHECK(network_thread_->IsCurrent()); @@ -485,7 +485,7 @@ JsepTransportController::CreateDtlsTransport( // Create DTLS wrapper around DatagramTransportInterface. dtls = absl::make_unique( - std::move(ice), std::move(datagram_transport), config_.crypto_options, + ice, std::move(datagram_transport), config_.crypto_options, config_.event_log); } else if (config_.media_transport_factory && config_.use_media_transport_for_media && @@ -494,13 +494,13 @@ JsepTransportController::CreateDtlsTransport( // then we don't need to create DTLS. // Otherwise, DTLS is still created. dtls = absl::make_unique( - std::move(ice), config_.crypto_options); + ice, config_.crypto_options); } else if (config_.external_transport_factory) { dtls = config_.external_transport_factory->CreateDtlsTransport( - std::move(ice), config_.crypto_options); + ice, config_.crypto_options); } else { dtls = absl::make_unique( - std::move(ice), config_.crypto_options, config_.event_log); + ice, config_.crypto_options, config_.event_log); } RTC_DCHECK(dtls); @@ -1170,21 +1170,22 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( } std::unique_ptr rtp_dtls_transport = - CreateDtlsTransport(std::move(ice), std::move(datagram_transport)); + CreateDtlsTransport(ice.get(), std::move(datagram_transport)); std::unique_ptr rtcp_dtls_transport; std::unique_ptr unencrypted_rtp_transport; std::unique_ptr sdes_transport; std::unique_ptr dtls_srtp_transport; + std::unique_ptr rtcp_ice; if (config_.rtcp_mux_policy != PeerConnectionInterface::kRtcpMuxPolicyRequire && content_info.type == cricket::MediaProtocolType::kRtp) { RTC_DCHECK(media_transport == nullptr); RTC_DCHECK(datagram_transport == nullptr); - rtcp_dtls_transport = CreateDtlsTransport( - CreateIceTransport(content_info.name, /*rtcp=*/true), - /*datagram_transport=*/nullptr); + rtcp_ice = CreateIceTransport(content_info.name, /*rtcp=*/true); + rtcp_dtls_transport = CreateDtlsTransport(rtcp_ice.get(), + /*datagram_transport=*/nullptr); } if (datagram_transport) { @@ -1217,10 +1218,10 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( std::unique_ptr jsep_transport = absl::make_unique( - content_info.name, certificate_, std::move(unencrypted_rtp_transport), - std::move(sdes_transport), std::move(dtls_srtp_transport), - std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport), - std::move(media_transport)); + content_info.name, certificate_, std::move(ice), std::move(rtcp_ice), + std::move(unencrypted_rtp_transport), std::move(sdes_transport), + std::move(dtls_srtp_transport), std::move(rtp_dtls_transport), + std::move(rtcp_dtls_transport), std::move(media_transport)); jsep_transport->SignalRtcpMuxActive.connect( this, &JsepTransportController::UpdateAggregateStates_n); diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index a79817c67b..ffc9515a60 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -345,7 +345,7 @@ class JsepTransportController : public sigslot::has_slots<> { bool local); std::unique_ptr CreateDtlsTransport( - std::unique_ptr ice, + cricket::IceTransportInternal* ice, std::unique_ptr datagram_transport); std::unique_ptr CreateIceTransport( const std::string transport_name, diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 8081df9432..d796b236d3 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -69,11 +69,10 @@ class FakeTransportFactory : public cricket::TransportFactoryInterface { } std::unique_ptr CreateDtlsTransport( - std::unique_ptr ice, + cricket::IceTransportInternal* ice, const webrtc::CryptoOptions& crypto_options) override { - std::unique_ptr fake_ice( - static_cast(ice.release())); - return absl::make_unique(std::move(fake_ice)); + return absl::make_unique( + static_cast(ice)); } }; diff --git a/pc/jsep_transport_unittest.cc b/pc/jsep_transport_unittest.cc index ac17d6f1f6..ec64379618 100644 --- a/pc/jsep_transport_unittest.cc +++ b/pc/jsep_transport_unittest.cc @@ -71,15 +71,15 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> { SrtpMode srtp_mode) { auto ice = absl::make_unique(kTransportName, ICE_CANDIDATE_COMPONENT_RTP); - auto rtp_dtls_transport = - absl::make_unique(std::move(ice)); + auto rtp_dtls_transport = absl::make_unique(ice.get()); + std::unique_ptr rtcp_ice; std::unique_ptr rtcp_dtls_transport; if (!rtcp_mux_enabled) { - ice = absl::make_unique(kTransportName, - ICE_CANDIDATE_COMPONENT_RTCP); + rtcp_ice = absl::make_unique( + kTransportName, ICE_CANDIDATE_COMPONENT_RTCP); rtcp_dtls_transport = - absl::make_unique(std::move(ice)); + absl::make_unique(rtcp_ice.get()); } std::unique_ptr unencrypted_rtp_transport; @@ -105,10 +105,11 @@ class JsepTransport2Test : public ::testing::Test, public sigslot::has_slots<> { // more logic that require unit tests. Note that creation of media_transport // is covered in jseptransportcontroller_unittest. auto jsep_transport = absl::make_unique( - kTransportName, /*local_certificate=*/nullptr, - std::move(unencrypted_rtp_transport), std::move(sdes_transport), - std::move(dtls_srtp_transport), std::move(rtp_dtls_transport), - std::move(rtcp_dtls_transport), /*media_transport=*/nullptr); + kTransportName, /*local_certificate=*/nullptr, std::move(ice), + std::move(rtcp_ice), std::move(unencrypted_rtp_transport), + std::move(sdes_transport), std::move(dtls_srtp_transport), + std::move(rtp_dtls_transport), std::move(rtcp_dtls_transport), + /*media_transport=*/nullptr); signal_rtcp_mux_active_received_ = false; jsep_transport->SignalRtcpMuxActive.connect(