Skip to content

Commit

Permalink
chore: cherry-pick 6b2643846ae3 from chromium (#33169)
Browse files Browse the repository at this point in the history
* chore: cherry-pick 6b2643846ae3 from chromium

* chore: update patches

Co-authored-by: PatchUp <73610968+patchup[bot]@users.noreply.github.com>
  • Loading branch information
nornagon and patchup[bot] committed Mar 7, 2022
1 parent b11b40e commit 013b38c
Show file tree
Hide file tree
Showing 2 changed files with 151 additions and 0 deletions.
1 change: 1 addition & 0 deletions patches/chromium/.patches
Expand Up @@ -127,3 +127,4 @@ cherry-pick-0081bb347e67.patch
m98_fs_fix_fileutil_lifetime_issue.patch
cleanup_pausablecriptexecutor_usage.patch
fix_don_t_restore_maximized_windows_when_calling_showinactive.patch
cherry-pick-6b2643846ae3.patch
150 changes: 150 additions & 0 deletions patches/chromium/cherry-pick-6b2643846ae3.patch
@@ -0,0 +1,150 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Ted Meyer <tmathmeyer@chromium.org>
Date: Thu, 24 Feb 2022 17:39:53 +0000
Subject: Guard BatchingMediaLog::event_handlers_ with lock
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It seems that despite MediaLog::OnWebMediaPlayerDestroyed and
MediaLog::AddLogRecord both grabbing a lock,
BatchingMediaLog::AddLogRecordLocked can escape the lock handle by
posting BatchingMediaLog::SendQueuedMediaEvents, causing a race.

When the addition of an event is interrupted by the deletion of a player
due to player culling in MediaInspectorContextImpl, a UAF can occur.

R=​dalecurtis

(cherry picked from commit 34526c3d0a857a22618e4d77c7f63b5ca6f8d3d2)

Bug: 1295786
Change-Id: I77df94988f806e4d98924669d27860e50455299d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3451494
Commit-Queue: Ted (Chromium) Meyer <tmathmeyer@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#970815}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3483655
Reviewed-by: Victor-Gabriel Savu <vsavu@google.com>
Owners-Override: Victor-Gabriel Savu <vsavu@google.com>
Commit-Queue: Roger Felipe Zanoni da Silva <rzanoni@google.com>
Cr-Commit-Position: refs/branch-heads/4664@{#1508}
Cr-Branched-From: 24dc4ee75e01a29d390d43c9c264372a169273a7-refs/heads/main@{#929512}

diff --git a/content/renderer/media/batching_media_log.cc b/content/renderer/media/batching_media_log.cc
index 79e6df99f2161da1ef551562c8277d4e02739d91..e2d33781f8c4254595e216202f3c9b3b55377e33 100644
--- a/content/renderer/media/batching_media_log.cc
+++ b/content/renderer/media/batching_media_log.cc
@@ -52,9 +52,9 @@ BatchingMediaLog::BatchingMediaLog(
scoped_refptr<base::SingleThreadTaskRunner> task_runner,
std::vector<std::unique_ptr<EventHandler>> event_handlers)
: task_runner_(std::move(task_runner)),
- event_handlers_(std::move(event_handlers)),
tick_clock_(base::DefaultTickClock::GetInstance()),
last_ipc_send_time_(tick_clock_->NowTicks()),
+ event_handlers_(std::move(event_handlers)),
ipc_send_pending_(false),
logged_rate_limit_warning_(false) {
// Pre-bind the WeakPtr on the right thread since we'll receive calls from
@@ -76,6 +76,7 @@ BatchingMediaLog::~BatchingMediaLog() {
}

void BatchingMediaLog::OnWebMediaPlayerDestroyedLocked() {
+ base::AutoLock lock(lock_);
for (const auto& handler : event_handlers_)
handler->OnWebMediaPlayerDestroyed();
}
@@ -198,32 +199,30 @@ std::string BatchingMediaLog::MediaEventToMessageString(

void BatchingMediaLog::SendQueuedMediaEvents() {
DCHECK(task_runner_->BelongsToCurrentThread());
+ base::AutoLock auto_lock(lock_);

- std::vector<media::MediaLogRecord> events_to_send;
- {
- base::AutoLock auto_lock(lock_);
- DCHECK(ipc_send_pending_);
- ipc_send_pending_ = false;
-
- if (last_duration_changed_event_) {
- queued_media_events_.push_back(*last_duration_changed_event_);
- last_duration_changed_event_.reset();
- }
+ DCHECK(ipc_send_pending_);
+ ipc_send_pending_ = false;

- if (last_buffering_state_event_) {
- queued_media_events_.push_back(*last_buffering_state_event_);
- last_buffering_state_event_.reset();
- }
+ if (last_duration_changed_event_) {
+ queued_media_events_.push_back(*last_duration_changed_event_);
+ last_duration_changed_event_.reset();
+ }

- queued_media_events_.swap(events_to_send);
- last_ipc_send_time_ = tick_clock_->NowTicks();
+ if (last_buffering_state_event_) {
+ queued_media_events_.push_back(*last_buffering_state_event_);
+ last_buffering_state_event_.reset();
}

- if (events_to_send.empty())
+ last_ipc_send_time_ = tick_clock_->NowTicks();
+
+ if (queued_media_events_.empty())
return;

for (const auto& handler : event_handlers_)
- handler->SendQueuedMediaEvents(events_to_send);
+ handler->SendQueuedMediaEvents(queued_media_events_);
+
+ queued_media_events_.clear();
}

void BatchingMediaLog::SetTickClockForTesting(
diff --git a/content/renderer/media/batching_media_log.h b/content/renderer/media/batching_media_log.h
index 6753ebe6605b239bdff84ff7f169cb00ea7d2b64..d3dd09d0c881c4894d0df1b1c6d18c53dcf480b8 100644
--- a/content/renderer/media/batching_media_log.h
+++ b/content/renderer/media/batching_media_log.h
@@ -67,9 +67,6 @@ class CONTENT_EXPORT BatchingMediaLog : public media::MediaLog {

scoped_refptr<base::SingleThreadTaskRunner> task_runner_;

- // impl for sending queued events.
- std::vector<std::unique_ptr<EventHandler>> event_handlers_;
-
// |lock_| protects access to all of the following member variables. It
// allows any render process thread to AddEvent(), while preserving their
// sequence for throttled send on |task_runner_| and coherent retrieval by
@@ -77,19 +74,24 @@ class CONTENT_EXPORT BatchingMediaLog : public media::MediaLog {
// guarantees provided by MediaLog, since SendQueuedMediaEvents must also
// be synchronized with respect to AddEvent.
mutable base::Lock lock_;
- const base::TickClock* tick_clock_;
- base::TimeTicks last_ipc_send_time_;
- std::vector<media::MediaLogRecord> queued_media_events_;
+ const base::TickClock* tick_clock_ GUARDED_BY(lock_);
+ base::TimeTicks last_ipc_send_time_ GUARDED_BY(lock_);
+ std::vector<media::MediaLogRecord> queued_media_events_ GUARDED_BY(lock_);
+
+ // impl for sending queued events.
+ std::vector<std::unique_ptr<EventHandler>> event_handlers_ GUARDED_BY(lock_);

// For enforcing max 1 pending send.
- bool ipc_send_pending_;
+ bool ipc_send_pending_ GUARDED_BY(lock_);

// True if we've logged a warning message about exceeding rate limits.
- bool logged_rate_limit_warning_;
+ bool logged_rate_limit_warning_ GUARDED_BY(lock_);

// Limits the number of events we send over IPC to one.
- absl::optional<media::MediaLogRecord> last_duration_changed_event_;
- absl::optional<media::MediaLogRecord> last_buffering_state_event_;
+ absl::optional<media::MediaLogRecord> last_duration_changed_event_
+ GUARDED_BY(lock_);
+ absl::optional<media::MediaLogRecord> last_buffering_state_event_
+ GUARDED_BY(lock_);

// Holds the earliest MEDIA_ERROR_LOG_ENTRY event added to this log. This is
// most likely to contain the most specific information available describing

0 comments on commit 013b38c

Please sign in to comment.