Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: cherry-pick eef098d1c7d5 from webrtc #36216

Merged
merged 2 commits into from Nov 2, 2022
Merged

chore: cherry-pick eef098d1c7d5 from webrtc #36216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
29b1916
chore: cherry-pick eef098d1c7d5 from webrtc
ppontes Nov 1, 2022
f1585cb
chore: update patches
patchup[bot] Nov 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions patches/webrtc/.patches
View file
Open in desktop
@@ -1 +1,2 @@
add_thread_local_to_x_error_trap_cc.patch
cherry-pick-eef098d1c7d5.patch
255 changes: 255 additions & 0 deletions patches/webrtc/cherry-pick-eef098d1c7d5.patch
View file
Open in desktop
@@ -0,0 +1,255 @@
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 {