Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick eef098d1c7d5 from webrtc
- Loading branch information
Showing
2 changed files
with
255 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
add_thread_local_to_x_error_trap_cc.patch | ||
cherry-pick-eef098d1c7d5.patch |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,254 @@ | ||
From eef098d1c7d50613d8bff2467d674525a9d0c57c Mon Sep 17 00:00:00 2001 | ||
From: Henrik Boström <hbos@webrtc.org> | ||
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 <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 2ced422..f84272c 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 @@ | ||
|
||
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 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<std::string, RTCOutboundRTPStreamStats*> 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<std::string, RTCOutboundRTPStreamStats*> 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<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 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<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 { |