diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index 7eb1501a59..e1475e0ded 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -275,6 +275,7 @@ DataChannelController::InternalCreateSctpDataChannel( // the network thread. (unless there's no transport). Change this so that // the role is checked on the network thread and any network thread related // initialization is done at the same time (to avoid additional hops). + // Use `GetSctpSslRole_n` on the network thread. if (pc_->GetSctpSslRole(&role)) { sid = sid_allocator_.AllocateSid(role); if (!sid.HasValue()) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 16c803d246..38e52b6a13 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -2231,41 +2231,48 @@ void PeerConnection::StopRtcEventLog_w() { bool PeerConnection::GetSctpSslRole(rtc::SSLRole* role) { RTC_DCHECK_RUN_ON(signaling_thread()); - if (!local_description() || !remote_description()) { - RTC_LOG(LS_VERBOSE) - << "Local and Remote descriptions must be applied to get the " - "SSL Role of the SCTP transport."; - return false; - } - if (!data_channel_controller_.data_channel_transport()) { + if (!sctp_mid_s_ || !data_channel_controller_.data_channel_transport()) { RTC_LOG(LS_INFO) << "Non-rejected SCTP m= section is needed to get the " "SSL Role of the SCTP transport."; return false; } - absl::optional dtls_role; - if (sctp_mid_s_) { - dtls_role = network_thread()->BlockingCall([this] { - RTC_DCHECK_RUN_ON(network_thread()); - return transport_controller_->GetDtlsRole(*sctp_mid_n_); - }); - if (!dtls_role && sdp_handler_->is_caller().has_value()) { - // This works fine if we are the offerer, but can be a mistake if - // we are the answerer and the remote offer is ACTIVE. In that - // case, we will guess the role wrong. - // TODO(bugs.webrtc.org/13668): Check if this actually happens. - RTC_LOG(LS_ERROR) - << "Possible risk: DTLS role guesser is active, is_caller is " - << *sdp_handler_->is_caller(); - dtls_role = - *sdp_handler_->is_caller() ? rtc::SSL_SERVER : rtc::SSL_CLIENT; - } - if (dtls_role) { - *role = *dtls_role; - return true; - } + absl::optional dtls_role = network_thread()->BlockingCall( + [this, is_caller = sdp_handler_->is_caller()] { + RTC_DCHECK_RUN_ON(network_thread()); + return GetSctpSslRole_n(is_caller); + }); + if (!dtls_role) { + return false; } - return false; + + *role = *dtls_role; + return true; +} + +absl::optional PeerConnection::GetSctpSslRole_n( + absl::optional is_caller) { + RTC_DCHECK_RUN_ON(network_thread()); + if (!sctp_mid_n_) + return absl::nullopt; + + absl::optional dtls_role = + transport_controller_->GetDtlsRole(*sctp_mid_n_); + if (!dtls_role) { + if (!is_caller.has_value()) + return absl::nullopt; + + // This works fine if we are the offerer, but can be a mistake if + // we are the answerer and the remote offer is ACTIVE. In that + // case, we will guess the role wrong. + // TODO(bugs.webrtc.org/13668): Check if this actually happens. + RTC_LOG(LS_ERROR) + << "Possible risk: DTLS role guesser is active, is_caller is " + << *is_caller; + dtls_role = *is_caller ? rtc::SSL_SERVER : rtc::SSL_CLIENT; + } + + return dtls_role; } bool PeerConnection::GetSslRole(const std::string& content_name, diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 3298203b39..1e50ec891b 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -315,6 +315,8 @@ class PeerConnection : public PeerConnectionInternal, } // Get current SSL role used by SCTP's underlying transport. bool GetSctpSslRole(rtc::SSLRole* role) override; + absl::optional GetSctpSslRole_n( + absl::optional is_caller) override; void OnSctpDataChannelStateChanged( DataChannelInterface* channel, diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index 262d2d5e19..debe830126 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -76,7 +76,13 @@ class PeerConnectionSdpMethods { virtual LegacyStatsCollector* legacy_stats() = 0; // Returns the observer. Will crash on CHECK if the observer is removed. virtual PeerConnectionObserver* Observer() const = 0; + // TODO(webrtc:11547): Remove `GetSctpSslRole` and require `GetSctpSslRole_n` + // instead. Currently `GetSctpSslRole` relied upon by `DataChannelController`. + // Once that path has been updated to use `GetSctpSslRole_n`, this method + // can be removed. virtual bool GetSctpSslRole(rtc::SSLRole* role) = 0; + virtual absl::optional GetSctpSslRole_n( + absl::optional is_caller) = 0; virtual PeerConnectionInterface::IceConnectionState ice_connection_state_internal() = 0; virtual void SetIceConnectionState( diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index a80f297c85..953dc86c40 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -1739,10 +1739,7 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( // If setting the description decided our SSL role, allocate any necessary // SCTP sids. - rtc::SSLRole role; - if (pc_->GetSctpSslRole(&role)) { - data_channel_controller()->AllocateSctpSids(role); - } + AllocateSctpSids(); if (IsUnifiedPlan()) { if (ConfiguredForMedia()) { @@ -1990,10 +1987,7 @@ void SdpOfferAnswerHandler::ApplyRemoteDescription( // If setting the description decided our SSL role, allocate any necessary // SCTP sids. - rtc::SSLRole role; - if (pc_->GetSctpSslRole(&role)) { - data_channel_controller()->AllocateSctpSids(role); - } + AllocateSctpSids(); if (operation->unified_plan()) { ApplyRemoteDescriptionUpdateTransceiverState(operation->type()); @@ -3231,6 +3225,28 @@ void SdpOfferAnswerHandler::UpdateNegotiationNeeded() { GenerateNegotiationNeededEvent(); } +void SdpOfferAnswerHandler::AllocateSctpSids() { + RTC_DCHECK_RUN_ON(signaling_thread()); + if (!local_description() || !remote_description()) { + RTC_DLOG(LS_VERBOSE) + << "Local and Remote descriptions must be applied to get the " + "SSL Role of the SCTP transport."; + return; + } + + absl::optional role = + network_thread()->BlockingCall([this, is_caller = is_caller()] { + RTC_DCHECK_RUN_ON(network_thread()); + return pc_->GetSctpSslRole_n(is_caller); + }); + + if (role) { + // TODO(webrtc:11547): Make this call on the network thread too once + // `AllocateSctpSids` has been updated. + data_channel_controller()->AllocateSctpSids(*role); + } +} + bool SdpOfferAnswerHandler::CheckIfNegotiationIsNeeded() { RTC_DCHECK_RUN_ON(signaling_thread()); // 1. If any implementation-specific negotiation is required, as described diff --git a/pc/sdp_offer_answer.h b/pc/sdp_offer_answer.h index 2124ed8697..ff075bcb5d 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -159,6 +159,7 @@ class SdpOfferAnswerHandler : public SdpStateProvider { absl::optional is_caller(); bool HasNewIceCredentials(); void UpdateNegotiationNeeded(); + void AllocateSctpSids(); // Destroys all BaseChannels and destroys the SCTP data channel, if present. void DestroyAllChannels(); diff --git a/pc/test/fake_peer_connection_base.h b/pc/test/fake_peer_connection_base.h index 5a1859ef5d..69b0106d1f 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -321,6 +321,10 @@ class FakePeerConnectionBase : public PeerConnectionInternal { LegacyStatsCollector* legacy_stats() override { return nullptr; } PeerConnectionObserver* Observer() const override { return nullptr; } bool GetSctpSslRole(rtc::SSLRole* role) override { return false; } + absl::optional GetSctpSslRole_n( + absl::optional is_caller) override { + return absl::nullopt; + } PeerConnectionInterface::IceConnectionState ice_connection_state_internal() override { return PeerConnectionInterface::IceConnectionState::kIceConnectionNew; diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h index a47d0b7eeb..a3ee7d8933 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -232,6 +232,10 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { MOCK_METHOD(LegacyStatsCollector*, legacy_stats, (), (override)); MOCK_METHOD(PeerConnectionObserver*, Observer, (), (const, override)); MOCK_METHOD(bool, GetSctpSslRole, (rtc::SSLRole*), (override)); + MOCK_METHOD(absl::optional, + GetSctpSslRole_n, + (absl::optional), + (override)); MOCK_METHOD(PeerConnectionInterface::IceConnectionState, ice_connection_state_internal, (),