github/electron
1
1
Fork 0
mirror of https://github.com/electron/electron.git synced 2026-04-10 03:01:51 -04:00
Code Issues Packages Projects Releases Wiki Activity

revert: cherry-pick eef098d1c7d5 from webrtc (#36264)

Browse Source 
Revert "chore: cherry-pick eef098d1c7d5 from webrtc (#36216)"

This reverts commit 2212933858.
This commit is contained in:
Pedro Pontes
2022-11-09 16:51:33 +01:00
committed by GitHub
parent e857073d65
commit 801cbe2ab3
2 changed files with 0 additions and 256 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
patches/webrtc/.patches
View File

@@ -1,2 +1 @@
add_thread_local_to_x_error_trap_cc.patch
cherry-pick-eef098d1c7d5.patch

255
patches/webrtc/cherry-pick-eef098d1c7d5.patch
View File

@@ -1,255 +0,0 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= <hbos@webrtc.org>
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 <hta@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Original-Commit-Position: refs/heads/main@{#38197}
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/277900
Reviewed-by: Evan Shrubsole <eshr@webrtc.org>
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 <map>
#include <memory>
#include <string>
+#include <utility>
#include <vector>
#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<const RTCStats> 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 <typename T>
+ T* TryAddStats(std::unique_ptr<T> 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<std::string, RTCOutboundRTPStreamStats*> 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<std::string, RTCOutboundRTPStreamStats*> 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<const RTCStatsReport> report = stats_->GetStatsReport();
+ auto inbound_rtps = report->GetStatsOfType<RTCInboundRTPStreamStats>();
+ auto outbound_rtps = report->GetStatsOfType<RTCOutboundRTPStreamStats>();
+ // 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> RTCStatsReport::Copy() const {
}
void RTCStatsReport::AddStats(std::unique_ptr<const RTCStats> 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 {