admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[Stats] Avoid DCHECK crashing if SSRCs are not unique.

Browse Source 
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.

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-Commit-Position: refs/heads/main@{#38197}
This commit is contained in:
Henrik Boström 2022-09-19 11:33:23 +02:00 committed by WebRTC LUCI CQ
parent d1875a232c
commit da6297dc53
4 changed files with 130 additions and 14 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
api/stats/rtc_stats_report.h
View File

@ -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(); }

46
pc/rtc_stats_collector.cc
View File

@ -2020,17 +2020,28 @@ void RTCStatsCollector::ProduceAudioRTPStreamStats_n(
.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;
@ -2059,9 +2070,14 @@ void RTCStatsCollector::ProduceAudioRTPStreamStats_n(
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,
@ -2115,7 +2131,10 @@ void RTCStatsCollector::ProduceVideoRTPStreamStats_n(
.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;
@ -2144,9 +2163,14 @@ void RTCStatsCollector::ProduceVideoRTPStreamStats_n(
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,

76
pc/rtc_stats_collector_unittest.cc
View File

@ -1009,6 +1009,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;

9
stats/rtc_stats_report.cc
View File

@ -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 {