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

Fix race in RTCStatsCollector's cache.

Browse Source 
`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}
This commit is contained in:
Henrik Boström 2022-09-30 17:22:44 +02:00 committed by WebRTC LUCI CQ
parent b8f31dc5c6
commit b3dd1738e4
3 changed files with 11 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
pc/BUILD.gn
View File

@ -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 = [

8
pc/rtc_stats_collector.cc
View File

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

5
pc/rtc_stats_collector.h
View File

@ -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<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_;