From 7ac4947ceeccf92492fe37fb8feba9ae7ce7ae70 Mon Sep 17 00:00:00 2001
From: Lambros Lambrou <[email protected]>
Date: Fri, 15 May 2026 09:17:24 -0700
Subject: [PATCH] Synchronize WebrtcEventLogData with mutex locking

Resolve potential threading race conditions and data corruption in
WebrtcEventLogData by adding base::Lock synchronization.

Changes include:
- Added base::Lock to protect internal circular_deque sections_.
- Implemented MAGI-reviewed zero-allocation buffer recycling in CreateNewSection().
- Added CHECK_GT validation to SetMaxSectionSizeForTest and SetMaxSectionsForTest.
- Eliminated base::checked_cast<int> to prevent DoS integer overflow crashes.
- Added multithreaded concurrency stress test.

TAG=agy
CONV=057084ee-cea7-42d5-a8ca-4575cf9a75e3

Bug: 513231432
Change-Id: I64bab172d521e704f75289cd3af0eb30ddc60123
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7848427
Commit-Queue: Joe Downing <[email protected]>
Reviewed-by: Joe Downing <[email protected]>
Auto-Submit: Lambros Lambrou <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1631341}
---

diff --git a/remoting/protocol/webrtc_event_log_data.cc b/remoting/protocol/webrtc_event_log_data.cc
index 322e277..e1b3c943 100644
--- a/remoting/protocol/webrtc_event_log_data.cc
+++ b/remoting/protocol/webrtc_event_log_data.cc
@@ -6,6 +6,7 @@
 
 #include <utility>
 
+#include "base/check_op.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 
@@ -21,26 +22,33 @@
 WebrtcEventLogData::~WebrtcEventLogData() = default;
 
 void WebrtcEventLogData::SetMaxSectionSizeForTest(int max_section_size) {
+  base::AutoLock lock(lock_);
+  CHECK_GT(max_section_size, 0);
   max_section_size_ = max_section_size;
 }
 
 void WebrtcEventLogData::SetMaxSectionsForTest(int max_sections) {
+  base::AutoLock lock(lock_);
+  CHECK_GT(max_sections, 0);
   max_sections_ = max_sections;
   sections_.reserve(max_sections_);
 }
 
 base::circular_deque<WebrtcEventLogData::LogSection>
 WebrtcEventLogData::TakeLogData() {
+  base::AutoLock lock(lock_);
   auto result = std::move(sections_);
 
-  // The |sections_| container is still valid but unspecified. Call Clear() to
-  // be certain it is empty (and the correct capacity is reserved).
-  Clear();
+  // The |sections_| container is still valid but unspecified. Call
+  // ClearLocked() to be certain it is empty (and the correct capacity is
+  // reserved).
+  ClearLocked();
   return result;
 }
 
 void WebrtcEventLogData::Write(std::string_view log_event) {
-  if (base::checked_cast<int>(log_event.size()) > max_section_size_) {
+  base::AutoLock lock(lock_);
+  if (log_event.size() > static_cast<size_t>(max_section_size_)) {
     LOG(WARNING) << "Oversized RTC log event: size = " << log_event.size();
   }
 
@@ -54,6 +62,11 @@
 }
 
 void WebrtcEventLogData::Clear() {
+  base::AutoLock lock(lock_);
+  ClearLocked();
+}
+
+void WebrtcEventLogData::ClearLocked() {
   sections_.clear();
   sections_.reserve(max_sections_);
 }
@@ -65,17 +78,21 @@
 
   // The event log entries are packet headers generated by WebRTC, and it is
   // assumed that the sizes are small enough to prevent integer overflow.
-  return base::checked_cast<int>(sections_.back().size() + log_event_size) >
-         max_section_size_;
+  return sections_.back().size() + log_event_size >
+         static_cast<size_t>(max_section_size_);
 }
 
 void WebrtcEventLogData::CreateNewSection() {
   if (static_cast<int>(sections_.size()) >= max_sections_) {
-    // Discard oldest section to make room.
+    // Recycle oldest section buffer to avoid heap allocation under lock.
+    auto section = std::move(sections_.front());
     sections_.pop_front();
+    section.clear();
+    sections_.push_back(std::move(section));
+  } else {
+    sections_.emplace_back();
+    sections_.back().reserve(max_section_size_);
   }
-  sections_.emplace_back();
-  sections_.back().reserve(max_section_size_);
 }
 
 }  // namespace remoting::protocol
diff --git a/remoting/protocol/webrtc_event_log_data.h b/remoting/protocol/webrtc_event_log_data.h
index 53e2b9d..3d98da1 100644
--- a/remoting/protocol/webrtc_event_log_data.h
+++ b/remoting/protocol/webrtc_event_log_data.h
@@ -10,6 +10,8 @@
 #include <vector>
 
 #include "base/containers/circular_deque.h"
+#include "base/synchronization/lock.h"
+#include "base/thread_annotations.h"
 
 namespace remoting::protocol {
 
@@ -50,24 +52,29 @@
 
  private:
   // Returns true if a new section must be created to store the event.
-  bool NeedNewSection(size_t log_event_size) const;
+  bool NeedNewSection(size_t log_event_size) const
+      EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
   // Appends a new section of zero size to the end of the list, removing the
   // oldest one if necessary. On return, the section at the end (the list's
   // "back") will be empty, ready to accept the new data.
-  void CreateNewSection();
+  void CreateNewSection() EXCLUSIVE_LOCKS_REQUIRED(lock_);
 
-  base::circular_deque<LogSection> sections_;
+  // Removes all event data without acquiring lock_.
+  void ClearLocked() EXCLUSIVE_LOCKS_REQUIRED(lock_);
+
+  mutable base::Lock lock_;
+  base::circular_deque<LogSection> sections_ GUARDED_BY(lock_);
 
   // Value chosen to keep the memory-usage within reasonable limits, but also
   // allow for recording "most" sessions entirely.
   const int kMaxSections = 1000;
-  int max_sections_ = kMaxSections;
+  int max_sections_ GUARDED_BY(lock_) = kMaxSections;
 
   // A larger value will reduce memory-allocations at the cost of discarding a
   // larger chunk of the event log.
   const int kMaxSectionSize = 102400;  // 100K
-  int max_section_size_ = kMaxSectionSize;
+  int max_section_size_ GUARDED_BY(lock_) = kMaxSectionSize;
 };
 
 }  // namespace remoting::protocol
diff --git a/remoting/protocol/webrtc_event_log_data_unittest.cc b/remoting/protocol/webrtc_event_log_data_unittest.cc
index be43155..7c71854 100644
--- a/remoting/protocol/webrtc_event_log_data_unittest.cc
+++ b/remoting/protocol/webrtc_event_log_data_unittest.cc
@@ -4,6 +4,11 @@
 
 #include "remoting/protocol/webrtc_event_log_data.h"
 
+#include "base/barrier_closure.h"
+#include "base/functional/bind.h"
+#include "base/run_loop.h"
+#include "base/task/thread_pool.h"
+#include "base/test/task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace remoting::protocol {
@@ -67,4 +72,32 @@
   EXPECT_TRUE(data.empty());
 }
 
+TEST(WebrtcEventLogDataTest, MultiThreadedAccess) {
+  base::test::TaskEnvironment task_environment;
+  WebrtcEventLogData event_log;
+  event_log.SetMaxSectionSizeForTest(100);
+
+  constexpr int kNumTasks = 10;
+  base::RunLoop run_loop;
+  auto barrier_closure =
+      base::BarrierClosure(kNumTasks, run_loop.QuitClosure());
+
+  for (int i = 0; i < kNumTasks; ++i) {
+    base::ThreadPool::PostTask(FROM_HERE, base::BindOnce(
+                                              [](WebrtcEventLogData* log,
+                                                 base::RepeatingClosure done) {
+                                                for (int j = 0; j < 100; ++j) {
+                                                  log->Write("test");

diff --git a/remoting/protocol/webrtc_event_log_data_unittest.cc b/remoting/protocol/webrtc_event_log_data_unittest.cc
index be43155..7c71854 100644
--- a/remoting/protocol/webrtc_event_log_data_unittest.cc
+++ b/remoting/protocol/webrtc_event_log_data_unittest.cc
@@ -4,6 +4,11 @@
 
 #include "remoting/protocol/webrtc_event_log_data.h"
 
+#include "base/barrier_closure.h"
+#include "base/functional/bind.h"
+#include "base/run_loop.h"
+#include "base/task/thread_pool.h"
+#include "base/test/task_environment.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace remoting::protocol {
@@ -67,4 +72,32 @@
   EXPECT_TRUE(data.empty());
 }
 
+TEST(WebrtcEventLogDataTest, MultiThreadedAccess) {
+  base::test::TaskEnvironment task_environment;
+  WebrtcEventLogData event_log;
+  event_log.SetMaxSectionSizeForTest(100);
+
+  constexpr int kNumTasks = 10;
+  base::RunLoop run_loop;
+  auto barrier_closure =
+      base::BarrierClosure(kNumTasks, run_loop.QuitClosure());
+
+  for (int i = 0; i < kNumTasks; ++i) {
+    base::ThreadPool::PostTask(FROM_HERE, base::BindOnce(
+                                              [](WebrtcEventLogData* log,
+                                                 base::RepeatingClosure done) {
+                                                for (int j = 0; j < 100; ++j) {
+                                                  log->Write("test");
+                                                  if (j % 10 == 0) {
+                                                    log->TakeLogData();
+                                                  }
+                                                }
+                                                done.Run();
+                                              },
+                                              &event_log, barrier_closure));
+  }
+
+  run_loop.Run();
+}
+
 }  // namespace remoting::protocol