From b42930453e84e27d8c6f67dd1988d6a9d3290750 Mon Sep 17 00:00:00 2001 From: Gabriel Charette Date: Thu, 27 Jul 2023 21:36:29 +0000 Subject: [PATCH] [Merge M115] [blink] Fix UAF in NonMainThreadTaskQueue The issue is that WorkerThreadScheduler::OnTaskCompleted's PerformMicrotaskCheckpoint() might result in a blink heap GC which may collect NonMainThreadWebSchedulingTaskQueueImpl (owned by GarbageCollected) which might own the last ref to NonMainThreadTaskQueue. If the NonMainThreadTaskQueue is deleted, there's a UAF in the follow-up call to task_queue->OnTaskRunTimeReported(task_timing); Retain a ref to NonMainThreadTaskQueue throughout OnTaskCompleted() to prevent this. The other option, proposed @ crbug.com/1464113#c3 was to bind the ref ahead of time in the `on_task_completed_handler` but I am leery that this might prevent deleting queues with pending tasks. R=altimin@chromium.org (cherry picked from commit 3463ed58f68034e68a1291b6413776c2b72994e8) Bug: 1464113 Change-Id: I877c609244ab90a0af1c87c317cf5a55e2fa60ff Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4678047 Reviewed-by: Alexander Timin Reviewed-by: Etienne Pierre-Doray Commit-Queue: Gabriel Charette Auto-Submit: Gabriel Charette Cr-Original-Commit-Position: refs/heads/main@{#1170760} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4706693 Commit-Queue: Etienne Pierre-Doray Cr-Commit-Position: refs/branch-heads/5790@{#1857} Cr-Branched-From: 1d71a337b1f6e707a13ae074dca1e2c34905eb9f-refs/heads/main@{#1148114} --- .../platform/scheduler/worker/non_main_thread_task_queue.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/third_party/blink/renderer/platform/scheduler/worker/non_main_thread_task_queue.cc b/third_party/blink/renderer/platform/scheduler/worker/non_main_thread_task_queue.cc index fac6a19c9f91e..f88419df8056d 100644 --- a/third_party/blink/renderer/platform/scheduler/worker/non_main_thread_task_queue.cc +++ b/third_party/blink/renderer/platform/scheduler/worker/non_main_thread_task_queue.cc @@ -69,6 +69,10 @@ void NonMainThreadTaskQueue::OnTaskCompleted( base::LazyNow* lazy_now) { // |non_main_thread_scheduler_| can be nullptr in tests. if (non_main_thread_scheduler_) { + // The last ref to `non_main_thread_scheduler_` might be released as part of + // this task's cleanup microtasks, make sure it lives through its own + // cleanup: crbug.com/1464113. + auto self_ref = WrapRefCounted(this); non_main_thread_scheduler_->OnTaskCompleted(this, task, task_timing, lazy_now); }