From 801cbe2ab3a1096d424d22f4fa35d7297850a51e Mon Sep 17 00:00:00 2001 From: Pedro Pontes Date: Wed, 9 Nov 2022 16:51:33 +0100 Subject: [PATCH] revert: cherry-pick eef098d1c7d5 from webrtc (#36264) Revert "chore: cherry-pick eef098d1c7d5 from webrtc (#36216)" This reverts commit 22129338587d597a37242279cb8b8f06a2925938. --- patches/webrtc/.patches | 1 - patches/webrtc/cherry-pick-eef098d1c7d5.patch | 255 ------------------ 2 files changed, 256 deletions(-) delete mode 100644 patches/webrtc/cherry-pick-eef098d1c7d5.patch diff --git a/patches/webrtc/.patches b/patches/webrtc/.patches index d13b98bac1cb5..9cde1db8895b3 100644 --- a/patches/webrtc/.patches +++ b/patches/webrtc/.patches @@ -1,2 +1 @@ add_thread_local_to_x_error_trap_cc.patch -cherry-pick-eef098d1c7d5.patch diff --git a/patches/webrtc/cherry-pick-eef098d1c7d5.patch b/patches/webrtc/cherry-pick-eef098d1c7d5.patch deleted file mode 100644 index fe1db04b315d9..0000000000000 --- a/patches/webrtc/cherry-pick-eef098d1c7d5.patch +++ /dev/null @@ -1,255 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= -Date: Mon, 19 Sep 2022 11:33:23 +0200 -Subject: Avoid DCHECK crashing if SSRCs are not unique. -MIME-Version: 1.0 -Content-Type: text/plain; charset=UTF-8 -Content-Transfer-Encoding: 8bit - -To properly handle SSRC collisions in non-BUNDLE we need to change how -RTP stats IDs are generated, but that is a riskier change to be dealt -with in a separate CL. - -For now, we just make sure that crashing is not a possibility during -SSRC collisions as a mitigation for https://crbug.com/1361612. This is -achieved by adding a TryAddStats() method to RTCStatsReport returning -whether successful. - -(cherry picked from commit da6297dc53cb2eaae7b1c5381652de9d707a7d48) - -Bug: chromium:1361612 -Change-Id: I8577ae4c84a7c1eb3c7527e9efd8d1b0254269a3 -Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/275766 -Reviewed-by: Harald Alvestrand -Commit-Queue: Henrik Boström -Cr-Original-Commit-Position: refs/heads/main@{#38197} -Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/277900 -Reviewed-by: Evan Shrubsole -Cr-Commit-Position: refs/branch-heads/5304@{#4} -Cr-Branched-From: 024bd84ca1cf7d3650c27912a3b5bfbb54da152a-refs/heads/main@{#38083} - -diff --git a/api/stats/rtc_stats_report.h b/api/stats/rtc_stats_report.h -index 2ced422370c79fab5ff3d95c24110b468a3eb9ee..f84272cb81f9e1ae1a8926f6627e221bbe456c2d 100644 ---- a/api/stats/rtc_stats_report.h -+++ b/api/stats/rtc_stats_report.h -@@ -17,6 +17,7 @@ - #include - #include - #include -+#include - #include - - #include "api/ref_counted_base.h" -@@ -68,6 +69,18 @@ class RTC_EXPORT RTCStatsReport final - - int64_t timestamp_us() const { return timestamp_us_; } - void AddStats(std::unique_ptr stats); -+ // On success, returns a non-owning pointer to `stats`. If the stats ID is not -+ // unique, `stats` is not inserted and nullptr is returned. -+ template -+ T* TryAddStats(std::unique_ptr stats) { -+ T* stats_ptr = stats.get(); -+ if (!stats_ -+ .insert(std::make_pair(std::string(stats->id()), std::move(stats))) -+ .second) { -+ return nullptr; -+ } -+ return stats_ptr; -+ } - const RTCStats* Get(const std::string& id) const; - size_t size() const { return stats_.size(); } - -diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc -index be6c23e8430cb2b19839c5b2cb7eebaa576a92cb..10dfb1501984f9cc99fa04432abbd9c74bc7b708 100644 ---- a/pc/rtc_stats_collector.cc -+++ b/pc/rtc_stats_collector.cc -@@ -1938,6 +1938,12 @@ void RTCStatsCollector::ProduceAudioRTPStreamStats_n( - inbound_audio->track_identifier = audio_track->id(); - } - inbound_audio->transport_id = transport_id; -+ auto* inbound_audio_ptr = report->TryAddStats(std::move(inbound_audio)); -+ if (!inbound_audio_ptr) { -+ RTC_LOG(LS_ERROR) -+ << "Unable to add audio 'inbound-rtp' to report, ID is not unique."; -+ continue; -+ } - // Remote-outbound. - auto remote_outbound_audio = CreateRemoteOutboundAudioStreamStats( - voice_receiver_info, mid, inbound_audio->id(), transport_id); -@@ -1945,10 +1951,15 @@ void RTCStatsCollector::ProduceAudioRTPStreamStats_n( - if (remote_outbound_audio) { - // When the remote outbound stats are available, the remote ID for the - // local inbound stats is set. -- inbound_audio->remote_id = remote_outbound_audio->id(); -- report->AddStats(std::move(remote_outbound_audio)); -+ auto* remote_outbound_audio_ptr = -+ report->TryAddStats(std::move(remote_outbound_audio)); -+ if (remote_outbound_audio_ptr) { -+ inbound_audio_ptr->remote_id = remote_outbound_audio_ptr->id(); -+ } else { -+ RTC_LOG(LS_ERROR) << "Unable to add audio 'remote-outbound-rtp' to " -+ << "report, ID is not unique."; -+ } - } -- report->AddStats(std::move(inbound_audio)); - } - // Outbound. - std::map audio_outbound_rtps; -@@ -1976,9 +1987,14 @@ void RTCStatsCollector::ProduceAudioRTPStreamStats_n( - attachment_id); - } - outbound_audio->transport_id = transport_id; -- audio_outbound_rtps.insert( -- std::make_pair(outbound_audio->id(), outbound_audio.get())); -- report->AddStats(std::move(outbound_audio)); -+ auto audio_outbound_pair = -+ std::make_pair(outbound_audio->id(), outbound_audio.get()); -+ if (report->TryAddStats(std::move(outbound_audio))) { -+ audio_outbound_rtps.insert(std::move(audio_outbound_pair)); -+ } else { -+ RTC_LOG(LS_ERROR) -+ << "Unable to add audio 'outbound-rtp' to report, ID is not unique."; -+ } - } - // Remote-inbound. - // These are Report Block-based, information sent from the remote endpoint, -@@ -2031,7 +2047,10 @@ void RTCStatsCollector::ProduceVideoRTPStreamStats_n( - inbound_video->track_identifier = video_track->id(); - } - inbound_video->transport_id = transport_id; -- report->AddStats(std::move(inbound_video)); -+ if (!report->TryAddStats(std::move(inbound_video))) { -+ RTC_LOG(LS_ERROR) -+ << "Unable to add video 'inbound-rtp' to report, ID is not unique."; -+ } - } - // Outbound - std::map video_outbound_rtps; -@@ -2059,9 +2078,14 @@ void RTCStatsCollector::ProduceVideoRTPStreamStats_n( - attachment_id); - } - outbound_video->transport_id = transport_id; -- video_outbound_rtps.insert( -- std::make_pair(outbound_video->id(), outbound_video.get())); -- report->AddStats(std::move(outbound_video)); -+ auto video_outbound_pair = -+ std::make_pair(outbound_video->id(), outbound_video.get()); -+ if (report->TryAddStats(std::move(outbound_video))) { -+ video_outbound_rtps.insert(std::move(video_outbound_pair)); -+ } else { -+ RTC_LOG(LS_ERROR) -+ << "Unable to add video 'outbound-rtp' to report, ID is not unique."; -+ } - } - // Remote-inbound - // These are Report Block-based, information sent from the remote endpoint, -diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc -index d3c4584555d7c467273c0f5fca9e6318f22d3b0b..61b1e1c0767e5b85f64d58134d7c46bd2e12e1f2 100644 ---- a/pc/rtc_stats_collector_unittest.cc -+++ b/pc/rtc_stats_collector_unittest.cc -@@ -1011,6 +1011,82 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsSingle) { - ExpectReportContainsCertificateInfo(report, *remote_certinfo); - } - -+// These SSRC collisions are legal. -+TEST_F(RTCStatsCollectorTest, ValidSsrcCollisionDoesNotCrash) { -+ // BUNDLE audio/video inbound/outbound. Unique SSRCs needed within the BUNDLE. -+ cricket::VoiceMediaInfo mid1_info; -+ mid1_info.receivers.emplace_back(); -+ mid1_info.receivers[0].add_ssrc(1); -+ mid1_info.senders.emplace_back(); -+ mid1_info.senders[0].add_ssrc(2); -+ pc_->AddVoiceChannel("Mid1", "Transport1", mid1_info); -+ cricket::VideoMediaInfo mid2_info; -+ mid2_info.receivers.emplace_back(); -+ mid2_info.receivers[0].add_ssrc(3); -+ mid2_info.senders.emplace_back(); -+ mid2_info.senders[0].add_ssrc(4); -+ pc_->AddVideoChannel("Mid2", "Transport1", mid2_info); -+ // Now create a second BUNDLE group with SSRCs colliding with the first group -+ // (but again no collisions within the group). -+ cricket::VoiceMediaInfo mid3_info; -+ mid3_info.receivers.emplace_back(); -+ mid3_info.receivers[0].add_ssrc(1); -+ mid3_info.senders.emplace_back(); -+ mid3_info.senders[0].add_ssrc(2); -+ pc_->AddVoiceChannel("Mid3", "Transport2", mid3_info); -+ cricket::VideoMediaInfo mid4_info; -+ mid4_info.receivers.emplace_back(); -+ mid4_info.receivers[0].add_ssrc(3); -+ mid4_info.senders.emplace_back(); -+ mid4_info.senders[0].add_ssrc(4); -+ pc_->AddVideoChannel("Mid4", "Transport2", mid4_info); -+ -+ // This should not crash (https://crbug.com/1361612). -+ rtc::scoped_refptr report = stats_->GetStatsReport(); -+ auto inbound_rtps = report->GetStatsOfType(); -+ auto outbound_rtps = report->GetStatsOfType(); -+ // TODO(https://crbug.com/webrtc/14443): When valid SSRC collisions are -+ // handled correctly, we should expect to see 4 of each type of object here. -+ EXPECT_EQ(inbound_rtps.size(), 2u); -+ EXPECT_EQ(outbound_rtps.size(), 2u); -+} -+ -+// These SSRC collisions are illegal, so it is not clear if this setup can -+// happen even when talking to a malicious endpoint, but simulate illegal SSRC -+// collisions just to make sure we don't crash in even the most extreme cases. -+TEST_F(RTCStatsCollectorTest, InvalidSsrcCollisionDoesNotCrash) { -+ // One SSRC to rule them all. -+ cricket::VoiceMediaInfo mid1_info; -+ mid1_info.receivers.emplace_back(); -+ mid1_info.receivers[0].add_ssrc(1); -+ mid1_info.senders.emplace_back(); -+ mid1_info.senders[0].add_ssrc(1); -+ pc_->AddVoiceChannel("Mid1", "BundledTransport", mid1_info); -+ cricket::VideoMediaInfo mid2_info; -+ mid2_info.receivers.emplace_back(); -+ mid2_info.receivers[0].add_ssrc(1); -+ mid2_info.senders.emplace_back(); -+ mid2_info.senders[0].add_ssrc(1); -+ pc_->AddVideoChannel("Mid2", "BundledTransport", mid2_info); -+ cricket::VoiceMediaInfo mid3_info; -+ mid3_info.receivers.emplace_back(); -+ mid3_info.receivers[0].add_ssrc(1); -+ mid3_info.senders.emplace_back(); -+ mid3_info.senders[0].add_ssrc(1); -+ pc_->AddVoiceChannel("Mid3", "BundledTransport", mid3_info); -+ cricket::VideoMediaInfo mid4_info; -+ mid4_info.receivers.emplace_back(); -+ mid4_info.receivers[0].add_ssrc(1); -+ mid4_info.senders.emplace_back(); -+ mid4_info.senders[0].add_ssrc(1); -+ pc_->AddVideoChannel("Mid4", "BundledTransport", mid4_info); -+ -+ // This should not crash (https://crbug.com/1361612). -+ stats_->GetStatsReport(); -+ // Because this setup is illegal, there is no "right answer" to how the report -+ // should look. We only care about not crashing. -+} -+ - TEST_F(RTCStatsCollectorTest, CollectRTCCodecStats) { - // Audio - cricket::VoiceMediaInfo voice_media_info; -diff --git a/stats/rtc_stats_report.cc b/stats/rtc_stats_report.cc -index 4fbd82508e7a2f415e12c2cc402b5630a53b1da1..187adadd7e928f45ddd727ad30d6ad6bfa81c32d 100644 ---- a/stats/rtc_stats_report.cc -+++ b/stats/rtc_stats_report.cc -@@ -71,12 +71,15 @@ rtc::scoped_refptr RTCStatsReport::Copy() const { - } - - void RTCStatsReport::AddStats(std::unique_ptr stats) { -+#if RTC_DCHECK_IS_ON - auto result = -+#endif - stats_.insert(std::make_pair(std::string(stats->id()), std::move(stats))); -+#if RTC_DCHECK_IS_ON - RTC_DCHECK(result.second) -- << "A stats object with ID " << result.first->second->id() -- << " is already " -- "present in this stats report."; -+ << "A stats object with ID \"" << result.first->second->id() << "\" is " -+ << "already present in this stats report."; -+#endif - } - - const RTCStats* RTCStatsReport::Get(const std::string& id) const {