commit b3dd1738e4ab384e0e68b7eede43e6d289fdb417
author Henrik Boström <hbos@webrtc.org> Fri Sep 30 15:22:44 2022
committer WebRTC LUCI CQ <webrtc-scoped@luci-project-accounts.iam.gserviceaccount.com> Fri Sep 30 18:21:20 2022
tree deb161740d876b3a1c410f79d45735b8a245e323
parent b8f31dc5c6e24b4c05a68f991fa4b7dbd9a4cc90

Fix race in RTCStatsCollector's cache.

`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 <mbonadei@webrtc.org>
Commit-Queue: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#38262}

pc/rtc_stats_collector.cc

diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc
index 3883a64..3dd10e2 100644
--- a/pc/rtc_stats_collector.cc
+++ b/pc/rtc_stats_collector.cc
@@ -1414,6 +1414,7 @@
 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 @@
   rtc::Thread::ScopedDisallowBlockingCalls no_blocking_calls;
 
   std::map<std::string, CertificateStatsPair> 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 @@
           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()));