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
256 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,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: [Merge-107] [Stats] 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 { |