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/BUILD.gn

diff --git a/pc/BUILD.gn b/pc/BUILD.gn
index 79b9a08..f06f78c 100644
--- a/pc/BUILD.gn
+++ b/pc/BUILD.gn
@@ -1025,6 +1025,7 @@
     "../rtc_base:stringutils",
     "../rtc_base:threading",
     "../rtc_base:timeutils",
+    "../rtc_base/synchronization:mutex",
     "../rtc_base/third_party/sigslot:sigslot",
   ]
   absl_deps = [

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

pc/rtc_stats_collector.h

diff --git a/pc/rtc_stats_collector.h b/pc/rtc_stats_collector.h
index 0c297c4..9117528 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 @@
   // 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<std::string, CertificateStatsPair> cached_certificates_by_transport_;
+  Mutex cached_certificates_mutex_;
+  std::map<std::string, CertificateStatsPair> cached_certificates_by_transport_
+      RTC_GUARDED_BY(cached_certificates_mutex_);
 
   Call::Stats call_stats_;