From b3dd1738e4ab384e0e68b7eede43e6d289fdb417 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Fri, 30 Sep 2022 17:22:44 +0200 Subject: [PATCH] Fix race in RTCStatsCollector's cache. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `cached_certificates_by_transport_` is used on the network thread, but can be cleared from the signaling thread. To fix the race where clear happens at the same time as stats collecting, a mutex is added. This mutex should very rarely be contended in practise since ClearCachedStatsReport() typically only happen during renegotiation (e.g. when someone joins/leaves) and getStats only happens once per second or less (typically). NOTRY=Everything passes except unrelated purple bot Bug: webrtc:14510 Change-Id: Iaf539a5cc8c87184fa0a87b9c889a13b941a9ad1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/277620 Reviewed-by: Mirko Bonadei Commit-Queue: Henrik Boström Cr-Commit-Position: refs/heads/main@{#38262} --- pc/BUILD.gn | 1 + pc/rtc_stats_collector.cc | 8 ++++++-- pc/rtc_stats_collector.h | 5 ++++- 3 files changed, 11 insertions(+), 3 deletions(-) diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 79b9a089b4..f06f78c940 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -1025,6 +1025,7 @@ rtc_source_set("rtc_stats_collector") { "../rtc_base:stringutils", "../rtc_base:threading", "../rtc_base:timeutils", + "../rtc_base/synchronization:mutex", "../rtc_base/third_party/sigslot:sigslot", ] absl_deps = [ diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index 3883a64c7b..3dd10e249d 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc @@ -1414,6 +1414,7 @@ void RTCStatsCollector::GetStatsReportInternal( void RTCStatsCollector::ClearCachedStatsReport() { RTC_DCHECK_RUN_ON(signaling_thread_); cached_report_ = nullptr; + MutexLock lock(&cached_certificates_mutex_); cached_certificates_by_transport_.clear(); } @@ -2248,14 +2249,16 @@ RTCStatsCollector::PrepareTransportCertificateStats_n( rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls; std::map transport_cert_stats; - if (!cached_certificates_by_transport_.empty()) { + { + MutexLock lock(&cached_certificates_mutex_); // Copy the certificate info from the cache, avoiding expensive // rtc::SSLCertChain::GetStats() calls. for (const auto& pair : cached_certificates_by_transport_) { transport_cert_stats.insert( std::make_pair(pair.first, pair.second.Copy())); } - } else { + } + if (transport_cert_stats.empty()) { // Collect certificate info. for (const auto& entry : transport_stats_by_name) { const std::string& transport_name = entry.first; @@ -2277,6 +2280,7 @@ RTCStatsCollector::PrepareTransportCertificateStats_n( std::make_pair(transport_name, std::move(certificate_stats_pair))); } // Copy the result into the certificate cache for future reference. + MutexLock lock(&cached_certificates_mutex_); for (const auto& pair : transport_cert_stats) { cached_certificates_by_transport_.insert( std::make_pair(pair.first, pair.second.Copy())); diff --git a/pc/rtc_stats_collector.h b/pc/rtc_stats_collector.h index 0c297c45a7..91175289e8 100644 --- a/pc/rtc_stats_collector.h +++ b/pc/rtc_stats_collector.h @@ -42,6 +42,7 @@ #include "rtc_base/ref_count.h" #include "rtc_base/ssl_certificate.h" #include "rtc_base/ssl_identity.h" +#include "rtc_base/synchronization/mutex.h" #include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/thread.h" #include "rtc_base/time_utils.h" @@ -285,7 +286,9 @@ class RTCStatsCollector : public rtc::RefCountInterface, // This cache avoids having to call rtc::SSLCertChain::GetStats(), which can // relatively expensive. ClearCachedStatsReport() needs to be called on // negotiation to ensure the cache is not obsolete. - std::map cached_certificates_by_transport_; + Mutex cached_certificates_mutex_; + std::map cached_certificates_by_transport_ + RTC_GUARDED_BY(cached_certificates_mutex_); Call::Stats call_stats_;