From 3ebea91a160ef132216d375fbaa84c523d3f1f07 Mon Sep 17 00:00:00 2001 From: xurui Date: Fri, 2 Aug 2024 17:11:07 +0800 Subject: [PATCH] =?UTF-8?q?=E4=BF=AE=E5=A4=8DCVE-2024-3170=20=EF=BC=88cher?= =?UTF-8?q?ry=20picked=20commit=20from=20?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pc/jsep_transport_controller.cc | 168 ++++++++++++---------- pc/jsep_transport_controller.h | 33 ++++- pc/sdp_offer_answer.cc | 14 +- test/peer_scenario/scenario_connection.cc | 7 +- 4 files changed, 131 insertions(+), 91 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index b7e9f361..143929b0 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -73,14 +73,18 @@ JsepTransportController::~JsepTransportController() { RTCError JsepTransportController::SetLocalDescription( SdpType type, - const cricket::SessionDescription* description) { + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc) { + RTC_DCHECK(local_desc); TRACE_EVENT0("webrtc", "JsepTransportController::SetLocalDescription"); + if (!network_thread_->IsCurrent()) { - return network_thread_->Invoke( - RTC_FROM_HERE, [=] { return SetLocalDescription(type, description); }); + return network_thread_->Invoke( + RTC_FROM_HERE, [=] { return SetLocalDescription(type, local_desc, remote_desc); }); } RTC_DCHECK_RUN_ON(network_thread_); + if (!initial_offerer_.has_value()) { initial_offerer_.emplace(type == SdpType::kOffer); if (*initial_offerer_) { @@ -89,20 +93,22 @@ RTCError JsepTransportController::SetLocalDescription( SetIceRole_n(cricket::ICEROLE_CONTROLLED); } } - return ApplyDescription_n(/*local=*/true, type, description); + return ApplyDescription_n(/*local=*/true, type, local_desc, remote_desc); } RTCError JsepTransportController::SetRemoteDescription( SdpType type, - const cricket::SessionDescription* description) { + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc) { + RTC_DCHECK(remote_desc); TRACE_EVENT0("webrtc", "JsepTransportController::SetRemoteDescription"); if (!network_thread_->IsCurrent()) { - return network_thread_->Invoke( - RTC_FROM_HERE, [=] { return SetRemoteDescription(type, description); }); + return network_thread_->BlockingCall( + [=] { return SetRemoteDescription(type, local_desc, remote_desc); }); } RTC_DCHECK_RUN_ON(network_thread_); - return ApplyDescription_n(/*local=*/false, type, description); + return ApplyDescription_n(/*local=*/false, type, local_desc, remote_desc); } RtpTransportInternal* JsepTransportController::GetRtpTransport( @@ -551,18 +557,20 @@ JsepTransportController::GetActiveDtlsTransports() { RTCError JsepTransportController::ApplyDescription_n( bool local, SdpType type, - const cricket::SessionDescription* description) { + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc) { TRACE_EVENT0("webrtc", "JsepTransportController::ApplyDescription_n"); - RTC_DCHECK(description); - if (local) { - local_desc_ = description; - } else { - remote_desc_ = description; - } + // Stash away the description object that we'll be applying (since this + // function is used for both local and remote). + const cricket::SessionDescription* description = + local ? local_desc : remote_desc; + + RTC_DCHECK(description); RTCError error; - error = ValidateAndMaybeUpdateBundleGroups(local, type, description); + error = + ValidateAndMaybeUpdateBundleGroups(local, type, local_desc, remote_desc); if (!error.ok()) { return error; } @@ -669,7 +677,11 @@ RTCError JsepTransportController::ApplyDescription_n( RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups( bool local, SdpType type, - const cricket::SessionDescription* description) { + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc) { + const cricket::SessionDescription* description = + local ? local_desc : remote_desc; + RTC_DCHECK(description); std::vector new_bundle_groups = @@ -735,72 +747,74 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroups( } } } else if (type == SdpType::kAnswer) { - std::vector offered_bundle_groups = - local ? remote_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE) - : local_desc_->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); - - std::map - offered_bundle_groups_by_mid; - for (const cricket::ContentGroup* offered_bundle_group : - offered_bundle_groups) { - for (const std::string& content_name : - offered_bundle_group->content_names()) { - offered_bundle_groups_by_mid[content_name] = offered_bundle_group; + if ((local && remote_desc) || (!local && local_desc)) { + std::vector offered_bundle_groups = + local ? remote_desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE) + : local_desc->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE); + + std::map + offered_bundle_groups_by_mid; + for (const cricket::ContentGroup* offered_bundle_group : + offered_bundle_groups) { + for (const std::string& content_name : + offered_bundle_group->content_names()) { + offered_bundle_groups_by_mid[content_name] = offered_bundle_group; + } } - } - std::map - new_bundle_groups_by_offered_bundle_groups; - for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) { - if (!new_bundle_group->FirstContentName()) { - // Empty groups could be a subset of any group. - continue; - } - // The group in the answer (new_bundle_group) must have a corresponding - // group in the offer (original_group), because the answer groups may only - // be subsets of the offer groups. - auto it = offered_bundle_groups_by_mid.find( - *new_bundle_group->FirstContentName()); - if (it == offered_bundle_groups_by_mid.end()) { - return RTCError(RTCErrorType::INVALID_PARAMETER, - "A BUNDLE group was added in the answer that did not " - "exist in the offer."); - } - const cricket::ContentGroup* offered_bundle_group = it->second; - if (new_bundle_groups_by_offered_bundle_groups.find( - offered_bundle_group) != - new_bundle_groups_by_offered_bundle_groups.end()) { - return RTCError(RTCErrorType::INVALID_PARAMETER, - "A MID in the answer has changed group."); - } - new_bundle_groups_by_offered_bundle_groups.insert( - std::make_pair(offered_bundle_group, new_bundle_group)); - for (const std::string& content_name : - new_bundle_group->content_names()) { - it = offered_bundle_groups_by_mid.find(content_name); - // The BUNDLE group in answer should be a subset of offered group. - if (it == offered_bundle_groups_by_mid.end() || - it->second != offered_bundle_group) { + std::map + new_bundle_groups_by_offered_bundle_groups; + for (const cricket::ContentGroup* new_bundle_group : new_bundle_groups) { + if (!new_bundle_group->FirstContentName()) { + // Empty groups could be a subset of any group. + continue; + } + // The group in the answer (new_bundle_group) must have a corresponding + // group in the offer (original_group), because the answer groups may + // only be subsets of the offer groups. + auto it = offered_bundle_groups_by_mid.find( + *new_bundle_group->FirstContentName()); + if (it == offered_bundle_groups_by_mid.end()) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "A BUNDLE group in answer contains a MID='" + - content_name + - "' that was not in the offered group."); + "A BUNDLE group was added in the answer that did not " + "exist in the offer."); } - } - } - - for (const auto& bundle_group : bundles_.bundle_groups()) { - for (const std::string& content_name : bundle_group->content_names()) { - // An answer that removes m= sections from pre-negotiated BUNDLE group - // without rejecting it, is invalid. - auto it = new_bundle_groups_by_mid.find(content_name); - if (it == new_bundle_groups_by_mid.end()) { - auto* content_info = description->GetContentByName(content_name); - if (!content_info || !content_info->rejected) { + const cricket::ContentGroup* offered_bundle_group = it->second; + if (new_bundle_groups_by_offered_bundle_groups.find( + offered_bundle_group) != + new_bundle_groups_by_offered_bundle_groups.end()) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "A MID in the answer has changed group."); + } + new_bundle_groups_by_offered_bundle_groups.insert( + std::make_pair(offered_bundle_group, new_bundle_group)); + for (const std::string& content_name : + new_bundle_group->content_names()) { + it = offered_bundle_groups_by_mid.find(content_name); + // The BUNDLE group in answer should be a subset of offered group. + if (it == offered_bundle_groups_by_mid.end() || + it->second != offered_bundle_group) { return RTCError(RTCErrorType::INVALID_PARAMETER, - "Answer cannot remove m= section with mid='" + + "A BUNDLE group in answer contains a MID='" + content_name + - "' from already-established BUNDLE group."); + "' that was not in the offered group."); + } + } + } + + for (const auto& bundle_group : bundles_.bundle_groups()) { + for (const std::string& content_name : bundle_group->content_names()) { + // An answer that removes m= sections from pre-negotiated BUNDLE group + // without rejecting it, is invalid. + auto it = new_bundle_groups_by_mid.find(content_name); + if (it == new_bundle_groups_by_mid.end()) { + auto* content_info = description->GetContentByName(content_name); + if (!content_info || !content_info->rejected) { + return RTCError(RTCErrorType::INVALID_PARAMETER, + "Answer cannot remove m= section with mid='" + + content_name + + "' from already-established BUNDLE group."); + } } } } diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 4e065668..75241fb2 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -156,11 +156,23 @@ class JsepTransportController : public sigslot::has_slots<> { // level, creating/destroying transport objects as needed and updating their // properties. This includes RTP, DTLS, and ICE (but not SCTP). At least not // yet? May make sense to in the future. + // + // `local_desc` must always be valid. If a remote description has previously + // been set via a call to `SetRemoteDescription()` then `remote_desc` should + // point to that description object in order to keep the current local and + // remote session descriptions in sync. RTCError SetLocalDescription(SdpType type, - const cricket::SessionDescription* description); - + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc); + // Call to apply a remote description (See `SetLocalDescription()` for local). + // + // `remote_desc` must always be valid. If a local description has previously + // been set via a call to `SetLocalDescription()` then `local_desc` should + // point to that description object in order to keep the current local and + // remote session descriptions in sync. RTCError SetRemoteDescription(SdpType type, - const cricket::SessionDescription* description); + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc); // Get transports to be used for the provided `mid`. If bundling is enabled, // calling GetRtpTransport for multiple MIDs may yield the same object. @@ -320,14 +332,23 @@ class JsepTransportController : public sigslot::has_slots<> { CallbackList signal_ice_candidate_pair_changed_ RTC_GUARDED_BY(network_thread_); + // Called from SetLocalDescription and SetRemoteDescription. + // When `local` is true, local_desc must be valid. Similarly when + // `local` is false, remote_desc must be valid. The description counterpart + // to the one that's being applied, may be nullptr but when it's supplied + // the counterpart description's content groups will be kept up to date for + // `type == SdpType::kAnswer`. RTCError ApplyDescription_n(bool local, SdpType type, - const cricket::SessionDescription* description) + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc) RTC_RUN_ON(network_thread_); RTCError ValidateAndMaybeUpdateBundleGroups( bool local, SdpType type, - const cricket::SessionDescription* description); + const cricket::SessionDescription* local_desc, + const cricket::SessionDescription* remote_desc) + RTC_RUN_ON(network_thread_); RTCError ValidateContent(const cricket::ContentInfo& content_info); void HandleRejectedContent(const cricket::ContentInfo& content_info) @@ -470,8 +491,6 @@ class JsepTransportController : public sigslot::has_slots<> { const Config config_; bool active_reset_srtp_params_ RTC_GUARDED_BY(network_thread_); - const cricket::SessionDescription* local_desc_ = nullptr; - const cricket::SessionDescription* remote_desc_ = nullptr; absl::optional initial_offerer_; cricket::IceConfig ice_config_; diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 3e62eabc..6b5810e1 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -1802,8 +1802,10 @@ RTCError SdpOfferAnswerHandler::ReplaceRemoteDescription( ReportSimulcastApiVersion(kSimulcastVersionApplyRemoteDescription, *session_desc); + const auto* local = local_description(); // NOTE: This will perform an Invoke() to the network thread. - return transport_controller()->SetRemoteDescription(sdp_type, session_desc); + return transport_controller()->SetRemoteDescription( + sdp_type, local ? local->description() : nullptr, session_desc); } void SdpOfferAnswerHandler::ApplyRemoteDescription( @@ -4477,13 +4479,15 @@ RTCError SdpOfferAnswerHandler::PushdownTransportDescription( if (source == cricket::CS_LOCAL) { const SessionDescriptionInterface* sdesc = local_description(); RTC_DCHECK(sdesc); - return transport_controller()->SetLocalDescription(type, - sdesc->description()); + const auto* remote = remote_description(); + return transport_controller()->SetLocalDescription( + type, sdesc->description(), remote ? remote->description() : nullptr); } else { const SessionDescriptionInterface* sdesc = remote_description(); RTC_DCHECK(sdesc); - return transport_controller()->SetRemoteDescription(type, - sdesc->description()); + const auto* local = local_description(); + return transport_controller()->SetRemoteDescription( + type, local ? local->description() : nullptr, sdesc->description()); } } diff --git a/test/peer_scenario/scenario_connection.cc b/test/peer_scenario/scenario_connection.cc index dac21d55..b944ad45 100644 --- a/test/peer_scenario/scenario_connection.cc +++ b/test/peer_scenario/scenario_connection.cc @@ -169,7 +169,9 @@ void ScenarioIceConnectionImpl::SetRemoteSdp(SdpType type, }); auto res = jsep_controller_->SetRemoteDescription( - remote_description_->GetType(), remote_description_->description()); + remote_description_->GetType(), + local_description_ ? local_description_->description() : nullptr, + remote_description_->description()); RTC_CHECK(res.ok()) << res.message(); RtpDemuxerCriteria criteria; for (const auto& content : remote_description_->description()->contents()) { @@ -199,7 +201,8 @@ void ScenarioIceConnectionImpl::SetLocalSdp(SdpType type, RTC_DCHECK_RUN_ON(signaling_thread_); local_description_ = webrtc::CreateSessionDescription(type, local_sdp); auto res = jsep_controller_->SetLocalDescription( - local_description_->GetType(), local_description_->description()); + local_description_->GetType(), local_description_->description(), + remote_description_ ? remote_description_->description() : nullptr); RTC_CHECK(res.ok()) << res.message(); jsep_controller_->MaybeStartGathering(); } -- Gitee