From b712493f1794e3d3b322b6707f2181a70419bc28 Mon Sep 17 00:00:00 2001 From: Pedro Pontes Date: Tue, 1 Nov 2022 22:38:44 +0100 Subject: [PATCH] chore: cherry-pick eef098d1c7d5 from webrtc --- patches/webrtc/.patches | 1 + patches/webrtc/cherry-pick-eef098d1c7d5.patch | 254 ++++++++++++++++++ 2 files changed, 255 insertions(+) create mode 100644 patches/webrtc/cherry-pick-eef098d1c7d5.patch diff --git a/patches/webrtc/.patches b/patches/webrtc/.patches index 9cde1db8895b3..d13b98bac1cb5 100644 --- a/patches/webrtc/.patches +++ b/patches/webrtc/.patches @@ -1 +1,2 @@ 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 new file mode 100644 index 0000000000000..f441c432b7a74 --- /dev/null +++ b/patches/webrtc/cherry-pick-eef098d1c7d5.patch @@ -0,0 +1,254 @@ +From eef098d1c7d50613d8bff2467d674525a9d0c57c Mon Sep 17 00:00:00 2001 +From: Henrik Boström +Date: Mon, 19 Sep 2022 11:33:23 +0200 +Subject: [PATCH] [Merge-107] [Stats] Avoid DCHECK crashing if SSRCs are not unique. + +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 2ced422..f84272c 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 @@ + + 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 c27533a..014ae39 100644 +--- a/pc/rtc_stats_collector.cc ++++ b/pc/rtc_stats_collector.cc +@@ -2015,17 +2015,28 @@ + .value()); + inbound_audio->track_identifier = audio_track->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.get(), transport_id); ++ voice_receiver_info, mid, *inbound_audio_ptr, transport_id); + // Add stats. + 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; +@@ -2054,9 +2065,14 @@ + RTCMediaSourceStatsIDFromKindAndAttachment(cricket::MEDIA_TYPE_AUDIO, + attachment_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, +@@ -2110,7 +2126,10 @@ + .value()); + inbound_video->track_identifier = video_track->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; +@@ -2139,9 +2158,14 @@ + RTCMediaSourceStatsIDFromKindAndAttachment(cricket::MEDIA_TYPE_VIDEO, + attachment_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 f3c4ee1..8ffdf72 100644 +--- a/pc/rtc_stats_collector_unittest.cc ++++ b/pc/rtc_stats_collector_unittest.cc +@@ -1009,6 +1009,82 @@ + 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 4fbd825..187adad 100644 +--- a/stats/rtc_stats_report.cc ++++ b/stats/rtc_stats_report.cc +@@ -71,12 +71,15 @@ + } + + 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 {