diff --git a/api/stats/rtc_stats_report.h b/api/stats/rtc_stats_report.h index 2ced422370..f84272cb81 100644 --- a/api/stats/rtc_stats_report.h +++ b/api/stats/rtc_stats_report.h @@ -17,6 +17,7 @@ #include #include #include +#include #include #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 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 + T* TryAddStats(std::unique_ptr 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 8bcc72d4f8..569312ceeb 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -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 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 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, diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index f3c4ee1de2..8ffdf7245c 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc @@ -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 report = stats_->GetStatsReport(); + auto inbound_rtps = report->GetStatsOfType(); + auto outbound_rtps = report->GetStatsOfType(); + // 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 4fbd82508e..187adadd7e 100644 --- a/stats/rtc_stats_report.cc +++ b/stats/rtc_stats_report.cc @@ -71,12 +71,15 @@ rtc::scoped_refptr RTCStatsReport::Copy() const { } void RTCStatsReport::AddStats(std::unique_ptr 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 {