diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc index e1475e0ded..275c82c259 100644 --- a/pc/data_channel_controller.cc +++ b/pc/data_channel_controller.cc @@ -227,7 +227,7 @@ void DataChannelController::OnDataChannelOpenMessage( const std::string& label, const InternalDataChannelInit& config) { rtc::scoped_refptr channel( - InternalCreateDataChannelWithProxy(label, &config)); + InternalCreateDataChannelWithProxy(label, config)); if (!channel.get()) { RTC_LOG(LS_ERROR) << "Failed to create DataChannel from the OPEN message."; return; @@ -240,7 +240,7 @@ void DataChannelController::OnDataChannelOpenMessage( rtc::scoped_refptr DataChannelController::InternalCreateDataChannelWithProxy( const std::string& label, - const InternalDataChannelInit* config) { + const InternalDataChannelInit& config) { RTC_DCHECK_RUN_ON(signaling_thread()); if (pc_->IsClosed()) { return nullptr; @@ -258,26 +258,31 @@ DataChannelController::InternalCreateDataChannelWithProxy( rtc::scoped_refptr DataChannelController::InternalCreateSctpDataChannel( const std::string& label, - const InternalDataChannelInit* config) { + const InternalDataChannelInit& config) { RTC_DCHECK_RUN_ON(signaling_thread()); - if (config && !config->IsValid()) { + if (!config.IsValid()) { RTC_LOG(LS_ERROR) << "Failed to initialize the SCTP data channel due to " "invalid DataChannelInit."; return nullptr; } - InternalDataChannelInit new_config = - config ? (*config) : InternalDataChannelInit(); + InternalDataChannelInit new_config = config; StreamId sid(new_config.id); if (!sid.HasValue()) { - rtc::SSLRole role; - // TODO(bugs.webrtc.org/11547): `GetSctpSslRole` likely involves a hop to - // 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); + // TODO(bugs.webrtc.org/11547): Use this call to the network thread more + // broadly to initialize the channel on the network thread, assign + // an id and/or other things that belong on the network thread. + // Move `sid_allocator_` to the network thread. + absl::optional role = network_thread()->BlockingCall([this] { + RTC_DCHECK_RUN_ON(network_thread()); + return pc_->GetSctpSslRole_n(); + }); + + if (!role) + role = new_config.fallback_ssl_role; + + if (role) { + sid = sid_allocator_.AllocateSid(*role); if (!sid.HasValue()) return nullptr; } diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h index 6275d6249e..8291670de6 100644 --- a/pc/data_channel_controller.h +++ b/pc/data_channel_controller.h @@ -82,8 +82,7 @@ class DataChannelController : public SctpDataChannelControllerInterface, // be offered in a SessionDescription, and wraps it in a proxy object. rtc::scoped_refptr InternalCreateDataChannelWithProxy( const std::string& label, - const InternalDataChannelInit* - config) /* RTC_RUN_ON(signaling_thread()) */; + const InternalDataChannelInit& config); void AllocateSctpSids(rtc::SSLRole role); // Checks if any data channel has been added. @@ -104,8 +103,7 @@ class DataChannelController : public SctpDataChannelControllerInterface, private: rtc::scoped_refptr InternalCreateSctpDataChannel( const std::string& label, - const InternalDataChannelInit* - config) /* RTC_RUN_ON(signaling_thread()) */; + const InternalDataChannelInit& config); // Parses and handles open messages. Returns true if the message is an open // message and should be considered to be handled, false otherwise. diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc index 1e575e60e9..f5575b7770 100644 --- a/pc/data_channel_controller_unittest.cc +++ b/pc/data_channel_controller_unittest.cc @@ -65,8 +65,7 @@ TEST_F(DataChannelControllerTest, CreateAndDestroy) { TEST_F(DataChannelControllerTest, CreateDataChannelEarlyRelease) { DataChannelController dcc(pc_.get()); auto channel = dcc.InternalCreateDataChannelWithProxy( - "label", - std::make_unique(DataChannelInit()).get()); + "label", InternalDataChannelInit(DataChannelInit())); channel = nullptr; // dcc holds a reference to channel, so not destroyed yet } @@ -75,8 +74,7 @@ TEST_F(DataChannelControllerTest, CreateDataChannelEarlyClose) { EXPECT_FALSE(dcc.HasDataChannels()); EXPECT_FALSE(dcc.HasUsedDataChannels()); auto channel = dcc.InternalCreateDataChannelWithProxy( - "label", - std::make_unique(DataChannelInit()).get()); + "label", InternalDataChannelInit(DataChannelInit())); EXPECT_TRUE(dcc.HasDataChannels()); EXPECT_TRUE(dcc.HasUsedDataChannels()); channel->Close(); @@ -87,8 +85,7 @@ TEST_F(DataChannelControllerTest, CreateDataChannelEarlyClose) { TEST_F(DataChannelControllerTest, CreateDataChannelLateRelease) { auto dcc = std::make_unique(pc_.get()); auto channel = dcc->InternalCreateDataChannelWithProxy( - "label", - std::make_unique(DataChannelInit()).get()); + "label", InternalDataChannelInit(DataChannelInit())); dcc.reset(); channel = nullptr; } @@ -96,8 +93,7 @@ TEST_F(DataChannelControllerTest, CreateDataChannelLateRelease) { TEST_F(DataChannelControllerTest, CloseAfterControllerDestroyed) { auto dcc = std::make_unique(pc_.get()); auto channel = dcc->InternalCreateDataChannelWithProxy( - "label", - std::make_unique(DataChannelInit()).get()); + "label", InternalDataChannelInit(DataChannelInit())); dcc.reset(); channel->Close(); } @@ -106,8 +102,7 @@ TEST_F(DataChannelControllerTest, AsyncChannelCloseTeardown) { DataChannelController dcc(pc_.get()); rtc::scoped_refptr channel = dcc.InternalCreateDataChannelWithProxy( - "label", - std::make_unique(DataChannelInit()).get()); + "label", InternalDataChannelInit(DataChannelInit())); SctpDataChannel* inner_channel = DowncastProxiedDataChannelInterfaceToSctpDataChannelForTesting( channel.get()); @@ -146,9 +141,9 @@ TEST_F(DataChannelControllerTest, MaxChannels) { NiceMock transport; int channel_id = 0; - ON_CALL(*pc_, GetSctpSslRole).WillByDefault([&](rtc::SSLRole* role) { - *role = (channel_id & 1) ? rtc::SSL_SERVER : rtc::SSL_CLIENT; - return true; + ON_CALL(*pc_, GetSctpSslRole_n).WillByDefault([&]() { + return absl::optional((channel_id & 1) ? rtc::SSL_SERVER + : rtc::SSL_CLIENT); }); DataChannelController dcc(pc_.get()); @@ -160,8 +155,7 @@ TEST_F(DataChannelControllerTest, MaxChannels) { for (channel_id = 0; channel_id <= cricket::kMaxSctpStreams; ++channel_id) { rtc::scoped_refptr channel = dcc.InternalCreateDataChannelWithProxy( - "label", - std::make_unique(DataChannelInit()).get()); + "label", InternalDataChannelInit(DataChannelInit())); if (channel_id == cricket::kMaxSctpStreams) { // We've reached the maximum and the previous call should have failed. diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 38e52b6a13..4ae7642fce 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1394,14 +1394,17 @@ PeerConnection::CreateDataChannelOrError(const std::string& label, bool first_datachannel = !data_channel_controller_.HasUsedDataChannels(); - std::unique_ptr internal_config; + InternalDataChannelInit internal_config; if (config) { - internal_config.reset(new InternalDataChannelInit(*config)); + internal_config = InternalDataChannelInit(*config); } + + internal_config.fallback_ssl_role = sdp_handler_->GuessSslRole(); + // TODO(bugs.webrtc.org/12796): Return a more specific error. rtc::scoped_refptr channel( data_channel_controller_.InternalCreateDataChannelWithProxy( - label, internal_config.get())); + label, internal_config)); if (!channel.get()) { return RTCError(RTCErrorType::INTERNAL_ERROR, "Data channel creation failed"); @@ -2229,50 +2232,10 @@ void PeerConnection::StopRtcEventLog_w() { } } -bool PeerConnection::GetSctpSslRole(rtc::SSLRole* role) { - RTC_DCHECK_RUN_ON(signaling_thread()); - 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 = 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; - } - - *role = *dtls_role; - return true; -} - -absl::optional PeerConnection::GetSctpSslRole_n( - absl::optional is_caller) { +absl::optional PeerConnection::GetSctpSslRole_n() { 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; + return sctp_mid_n_ ? transport_controller_->GetDtlsRole(*sctp_mid_n_) + : absl::nullopt; } bool PeerConnection::GetSslRole(const std::string& content_name, diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 1e50ec891b..d1b197c069 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -314,9 +314,7 @@ class PeerConnection : public PeerConnectionInternal, sdp_handler_->signaling_state() == PeerConnectionInterface::kClosed; } // 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; + absl::optional GetSctpSslRole_n() override; void OnSctpDataChannelStateChanged( DataChannelInterface* channel, diff --git a/pc/peer_connection_internal.h b/pc/peer_connection_internal.h index debe830126..2ddb57b50c 100644 --- a/pc/peer_connection_internal.h +++ b/pc/peer_connection_internal.h @@ -76,13 +76,7 @@ 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 absl::optional GetSctpSslRole_n() = 0; virtual PeerConnectionInterface::IceConnectionState ice_connection_state_internal() = 0; virtual void SetIceConnectionState( diff --git a/pc/sctp_data_channel.h b/pc/sctp_data_channel.h index e0f8bb24b6..5217505da9 100644 --- a/pc/sctp_data_channel.h +++ b/pc/sctp_data_channel.h @@ -70,6 +70,11 @@ struct InternalDataChannelInit : public DataChannelInit { bool IsValid() const; OpenHandshakeRole open_handshake_role; + // Optional fallback or backup flag from PC that's used for non-prenegotiated + // stream ids in situations where we cannot determine the SSL role from the + // transport for purposes of generating a stream ID. + // See: https://www.rfc-editor.org/rfc/rfc8832.html#name-protocol-overview + absl::optional fallback_ssl_role; }; // Helper class to allocate unique IDs for SCTP DataChannels. diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 8dc6d84762..f19cd13ac7 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -3134,7 +3134,7 @@ void SdpOfferAnswerHandler::OnOperationsChainEmpty() { } } -absl::optional SdpOfferAnswerHandler::is_caller() { +absl::optional SdpOfferAnswerHandler::is_caller() const { RTC_DCHECK_RUN_ON(signaling_thread()); return is_caller_; } @@ -3234,11 +3234,14 @@ void SdpOfferAnswerHandler::AllocateSctpSids() { return; } - absl::optional role = - network_thread()->BlockingCall([this, is_caller = is_caller()] { - RTC_DCHECK_RUN_ON(network_thread()); - return pc_->GetSctpSslRole_n(is_caller); - }); + absl::optional role = network_thread()->BlockingCall([this] { + RTC_DCHECK_RUN_ON(network_thread()); + return pc_->GetSctpSslRole_n(); + }); + + if (!role) { + role = GuessSslRole(); + } if (role) { // TODO(webrtc:11547): Make this call on the network thread too once @@ -3247,6 +3250,49 @@ void SdpOfferAnswerHandler::AllocateSctpSids() { } } +absl::optional SdpOfferAnswerHandler::GuessSslRole() const { + RTC_DCHECK_RUN_ON(signaling_thread()); + if (!pc_->sctp_mid()) + return absl::nullopt; + + // TODO(bugs.webrtc.org/13668): This guesswork is guessing wrong (returning + // SSL_CLIENT = ACTIVE) if remote offer has role ACTIVE, but we'll be able + // to detect that by looking at the SDP. + // + // The phases of establishing an SCTP session are: + // + // Offerer: + // + // * Before negotiation: Neither is_caller nor sctp_mid exists. + // * After setting an offer as local description: is_caller is known (true), + // sctp_mid is known, but we don't know the SSL role for sure (or if we'll + // eventually get an SCTP session). + // * After setting an answer as the remote description: We know is_caller, + // sctp_mid and that we'll get the SCTP channel established (m-section + // wasn't rejected). + // * Special case: The SCTP m-section was rejected: Close datachannels. + // * We MAY know the SSL role if we offered actpass and got back active or + // passive; if the other end is a webrtc implementation, it will be active. + // * After the TLS handshake: We have a definitive answer on the SSL role. + // + // Answerer: + // + // * After setting an offer as remote description: We know is_caller (false). + // * If there was an SCTP session, we know the SCTP mid. We also know the + // SSL role, since if the remote offer was actpass or passive, we'll answer + // active, and if the remote offer was active, we're passive. + // * Special case: No SCTP m= line. We don't know for sure if the remote + // doesn't support it or just didn't offer it. Not sure what we do in this + // case (logic would suggest fire a `negotiationneeded` event and generate a + // subsequent offer, but this needs to be tested). + // * After the TLS handshake: We know that TLS obeyed the protocol. There + // should be an error surfaced somewhere if it didn't. + // * "Guessing" should always be correct if we get an SCTP session and are not + // the offerer. + + return is_caller() ? rtc::SSL_SERVER : rtc::SSL_CLIENT; +} + 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 ff075bcb5d..80a21391b9 100644 --- a/pc/sdp_offer_answer.h +++ b/pc/sdp_offer_answer.h @@ -156,10 +156,15 @@ class SdpOfferAnswerHandler : public SdpStateProvider { bool AddStream(MediaStreamInterface* local_stream); void RemoveStream(MediaStreamInterface* local_stream); - absl::optional is_caller(); + absl::optional is_caller() const; bool HasNewIceCredentials(); void UpdateNegotiationNeeded(); void AllocateSctpSids(); + // Based on the negotiation state, guess what the SSLRole might be without + // directly getting the information from the transport. + // This is used for allocating stream ids for data channels. + // See also `InternalDataChannelInit::fallback_ssl_role`. + absl::optional GuessSslRole() const; // 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 69b0106d1f..0508673d36 100644 --- a/pc/test/fake_peer_connection_base.h +++ b/pc/test/fake_peer_connection_base.h @@ -320,9 +320,7 @@ class FakePeerConnectionBase : public PeerConnectionInternal { cricket::PortAllocator* port_allocator() override { return nullptr; } 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 { + absl::optional GetSctpSslRole_n() override { return absl::nullopt; } PeerConnectionInterface::IceConnectionState ice_connection_state_internal() diff --git a/pc/test/mock_peer_connection_internal.h b/pc/test/mock_peer_connection_internal.h index a3ee7d8933..81f8903bd5 100644 --- a/pc/test/mock_peer_connection_internal.h +++ b/pc/test/mock_peer_connection_internal.h @@ -231,11 +231,7 @@ class MockPeerConnectionInternal : public PeerConnectionInternal { MOCK_METHOD(cricket::PortAllocator*, port_allocator, (), (override)); 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(absl::optional, GetSctpSslRole_n, (), (override)); MOCK_METHOD(PeerConnectionInterface::IceConnectionState, ice_connection_state_internal, (),