From 46d63b00ac54db566df482976c8bcee7d932181d Mon Sep 17 00:00:00 2001
From: kylechar <[email protected]>
Date: Wed, 14 Oct 2026 09:29:35 -0700
Subject: [PATCH] Fix SoftwareImageDecodeCache UAF

SoftwareImageDecodeCache had a bug where if image decode tasks were
stopped and restarted there could be a collision on
ImageDecodeRequestId. If the CompositorFrameSink is lost the
ImageDecodeCache object is recreated and
ImageController::SetImageDecodeCache() runs. That calls
StopWorkerTasks() which cancels pending decode requests but didn't
cancel posted tasks. GenerateTasksForOrphanedRequests() then schedules
new tasks reusing the same ImageDecodeRequestId range. If a posted
ImageDecodeCompleted() from before StopWorkerTasks() runs and the
ImageDecodeRequestId matches one of the new request there is a
collision.

The collision can result in a CacheEntry being deleted while there is
still a pending request for it. Avoid collisions by cancelling all
posted ImageDecocdeComplete() tasks in StopWorkTasks().

Bug: 569286604
Change-Id: I2a2214efb8a99f0e88b35ee1f5841c91a4831f7d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/8567362
Reviewed-by: Vladimir Levin <[email protected]>
Commit-Queue: Kyle Charbonneau <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1718683}
---
diff --git a/cc/tiles/image_controller.cc b/cc/tiles/image_controller.cc
index 87ac2ce..3cb62a2c 100644
--- a/cc/tiles/image_controller.cc
+++ b/cc/tiles/image_controller.cc
@@ -90,6 +90,10 @@
 
     // Now, begin cleanup.
 
+    // Cancel any posted tasks to avoid request id collisions.
+    weak_ptr_factory_.InvalidateWeakPtrs();
+    worker_state_->weak_ptr = weak_ptr_factory_.GetWeakPtr();
+
     // Unlock all of the locked images (note that this vector would only be
     // populated if we actually need to unref the image.
     for (auto& image_pair : requested_locked_images_) {
diff --git a/cc/tiles/image_controller.h b/cc/tiles/image_controller.h
index 29deb2c..982d37d 100644
--- a/cc/tiles/image_controller.h
+++ b/cc/tiles/image_controller.h
@@ -139,7 +139,10 @@
     WorkerTaskState task_state GUARDED_BY(lock) = WorkerTaskState::kNoTask;
 
     const scoped_refptr<base::SequencedTaskRunner> origin_task_runner;
-    const base::WeakPtr<ImageController> weak_ptr;
+    // `weak_ptr` may be replaced while worker tasks are running so access is
+    // guarded by the lock. WeakPtr is not thread safe and copies of `weak_ptr`
+    // can still only be dereferenced on `origin_task_runner`.
+    base::WeakPtr<ImageController> weak_ptr GUARDED_BY(lock);
   };
 
   void ForEachDecodeRequest(base::FunctionRef<void(ImageDecodeRequest&)>);
diff --git a/cc/tiles/software_image_decode_cache_unittest.cc b/cc/tiles/software_image_decode_cache_unittest.cc
index 7d2a375..bd5b2ab2 100644
--- a/cc/tiles/software_image_decode_cache_unittest.cc
+++ b/cc/tiles/software_image_decode_cache_unittest.cc
@@ -4,13 +4,25 @@
 
 #include "cc/tiles/software_image_decode_cache.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/feature_list.h"
+#include "base/functional/bind.h"
+#include "base/functional/callback_helpers.h"
+#include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/thread_pool.h"
+#include "base/test/bind.h"
+#include "base/threading/thread_restrictions.h"
 #include "cc/base/features.h"
 #include "cc/paint/draw_image.h"
 #include "cc/paint/paint_image_builder.h"
 #include "cc/test/fake_paint_image_generator.h"
 #include "cc/test/skia_common.h"
 #include "cc/test/test_tile_task_runner.h"
+#include "cc/tiles/image_controller.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkAlphaType.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -2384,5 +2396,134 @@
             cache_.GetMaxNumCacheEntriesForTesting());
 }
 
+class BlockingPaintImageGenerator : public FakePaintImageGenerator {
+ public:
+  explicit BlockingPaintImageGenerator(const SkImageInfo& info)
+      : FakePaintImageGenerator(info),
+        decode_started_(base::WaitableEvent::ResetPolicy::MANUAL,
+                        base::WaitableEvent::InitialState::NOT_SIGNALED),
+        allow_finish_(base::WaitableEvent::ResetPolicy::MANUAL,
+                      base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+
+  bool GetPixels(SkPixmap dst_pixmap,
+                 size_t frame_index,
+                 PaintImage::GeneratorClientId client_id,
+                 uint32_t lazy_pixel_ref) override {
+    base::ScopedAllowBaseSyncPrimitivesForTesting allow_sync;
+    decode_started_.Signal();
+    allow_finish_.Wait();
+    return FakePaintImageGenerator::GetPixels(dst_pixmap, frame_index,
+                                              client_id, lazy_pixel_ref);
+  }
+
+  void WaitUntilDecodeStarted() {
+    base::ScopedAllowBaseSyncPrimitivesForTesting allow_sync;
+    decode_started_.Wait();
+  }
+  void AllowDecodeToFinish() { allow_finish_.Signal(); }
+  void ResetEvents() {
+    decode_started_.Reset();
+    allow_finish_.Reset();
+  }
+
+ private:
+  ~BlockingPaintImageGenerator() override = default;
+
+  base::WaitableEvent decode_started_;
+  base::WaitableEvent allow_finish_;
+};
+
+TEST_F(SoftwareImageDecodeCacheTest,
+       OrphanedRequestStaleCompletionWhileDecoding) {
+  auto origin_task_runner = base::SequencedTaskRunner::GetCurrentDefault();
+  auto worker_task_runner =
+      base::ThreadPool::CreateSequencedTaskRunner(base::TaskTraits());
+
+  ImageController controller(origin_task_runner, worker_task_runner,
+                             base::DoNothing());
+  TestSoftwareImageDecodeCache cache1;
+  controller.SetImageDecodeCache(&cache1);
+
+  auto generator = sk_make_sp<BlockingPaintImageGenerator>(
+      SkImageInfo::MakeN32Premul(100, 100, DefaultSkColorSpace()));
+  PaintImage paint_image = PaintImageBuilder::WithDefault()
+                               .set_id(PaintImage::GetNextId())
+                               .set_paint_image_generator(generator)
+                               .TakePaintImage();
+  DrawImage draw_image(
+      paint_image, false,
+      SkIRect::MakeWH(paint_image.width(), paint_image.height()),
+      PaintFlags::FilterQuality::kLow,
+      CreateMatrix(SkSize::Make(1.f, 1.f), true),
+      PaintImage::kDefaultFrameIndex, DefaultTargetColorParams());
+
+  // Queue an image decode on `cache1` and wait until the worker thread starts
+  // decoding.
+  bool callback_called = false;
+  controller.QueueImageDecode(
+      draw_image,
+      base::BindLambdaForTesting(
+          [&](ImageController::ImageDecodeRequestId id,
+              ImageController::ImageDecodeResult result) {
+            callback_called = true;
+            EXPECT_EQ(result, ImageController::ImageDecodeResult::SUCCESS);
+            controller.UnlockImageDecode(id);
+          }),
+      /*speculative=*/false);
+  generator->WaitUntilDecodeStarted();
+
+  // Allow the first decode on the worker thread to finish. When
+  // `RunOnWorkerThread()` returns, the worker posts a stale
+  // `ImageDecodeCompleted(id)` task to `origin_task_runner`. Before running
+  // `origin_task_runner`, simulate a compositor frame sink loss and recreation,
+  // `SetImageDecodeCache(nullptr)` followed by `SetImageDecodeCache(&cache2)`.
+  // `StopWorkerTasks()` completes the first decode and re-queues the request as
+  // an orphaned request with the same `id`.
+  generator->AllowDecodeToFinish();
+  controller.SetImageDecodeCache(nullptr);
+
+  TestSoftwareImageDecodeCache cache2;
+
+  // Reset the blocking events for the second decode of `draw_image` when the
+  // orphaned request is regenerated on `cache2`.
+  generator->ResetEvents();
+  controller.SetImageDecodeCache(&cache2);
+
+  // Wait until the worker thread picks up the regenerated request (which
+  // reuses the same `id` and is now in `requests_needing_completion[id]`) and
+  // enters `DecodeImageIfNecessary()` with `cache2.lock_` unlocked.
+  generator->WaitUntilDecodeStarted();
+
+  // Run `origin_task_runner`. The stale `ImageDecodeCompleted(id)` task from
+  // previous step must be cancelled by `StopWorkerTasks()` so it does not
+  // prematurely complete and unref the in-progress request on `cache2`.
+  base::RunLoop run_loop;
+  origin_task_runner->PostTask(FROM_HERE, run_loop.QuitWhenIdleClosure());
+  run_loop.Run();
+  EXPECT_FALSE(callback_called);
+
+  // Populate `cache2` with >1000 unlocked entries. Because `draw_image` still
+  // has `ref_count > 0`, `ReduceCacheUsageUntilWithinLimit()` must not evict
+  // or free its `CacheEntry` while `DecodeImageIfNecessary()` is running.
+  for (size_t i = 0; i < cache2.GetMaxNumCacheEntriesForTesting(); ++i) {
+    PaintImage small_image = CreatePaintImage(1, 1);
+    DrawImage small_draw_image(small_image, false, SkIRect::MakeWH(1, 1),
+                               PaintFlags::FilterQuality::kLow,
+                               CreateMatrix(SkSize::Make(1.f, 1.f), true),
+                               PaintImage::kDefaultFrameIndex,
+                               DefaultTargetColorParams());
+    DecodedDrawImage small_decoded =
+        cache2.GetDecodedImageForDraw(small_draw_image);
+    cache2.DrawWithImageFinished(small_draw_image, small_decoded);
+  }
+
+  // Now let the worker thread finish `DoDecodeImage()` on `cache2` and verify
+  // the real completion callback runs cleanly.
+  generator->AllowDecodeToFinish();
+  controller.FlushDecodeTasksForTesting();
+  EXPECT_TRUE(callback_called);
+  controller.SetImageDecodeCache(nullptr);
+}
+
 }  // namespace
 }  // namespace cc

diff --git a/cc/tiles/software_image_decode_cache_unittest.cc b/cc/tiles/software_image_decode_cache_unittest.cc
index 7d2a375..bd5b2ab2 100644
--- a/cc/tiles/software_image_decode_cache_unittest.cc
+++ b/cc/tiles/software_image_decode_cache_unittest.cc
@@ -4,13 +4,25 @@
 
 #include "cc/tiles/software_image_decode_cache.h"
 
+#include <memory>
+#include <utility>
+
 #include "base/feature_list.h"
+#include "base/functional/bind.h"
+#include "base/functional/callback_helpers.h"
+#include "base/run_loop.h"
+#include "base/synchronization/waitable_event.h"
+#include "base/task/sequenced_task_runner.h"
+#include "base/task/thread_pool.h"
+#include "base/test/bind.h"
+#include "base/threading/thread_restrictions.h"
 #include "cc/base/features.h"
 #include "cc/paint/draw_image.h"
 #include "cc/paint/paint_image_builder.h"
 #include "cc/test/fake_paint_image_generator.h"
 #include "cc/test/skia_common.h"
 #include "cc/test/test_tile_task_runner.h"
+#include "cc/tiles/image_controller.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/skia/include/core/SkAlphaType.h"
 #include "third_party/skia/include/core/SkBitmap.h"
@@ -2384,5 +2396,134 @@
             cache_.GetMaxNumCacheEntriesForTesting());
 }
 
+class BlockingPaintImageGenerator : public FakePaintImageGenerator {
+ public:
+  explicit BlockingPaintImageGenerator(const SkImageInfo& info)
+      : FakePaintImageGenerator(info),
+        decode_started_(base::WaitableEvent::ResetPolicy::MANUAL,
+                        base::WaitableEvent::InitialState::NOT_SIGNALED),
+        allow_finish_(base::WaitableEvent::ResetPolicy::MANUAL,
+                      base::WaitableEvent::InitialState::NOT_SIGNALED) {}
+
+  bool GetPixels(SkPixmap dst_pixmap,
+                 size_t frame_index,
+                 PaintImage::GeneratorClientId client_id,
+                 uint32_t lazy_pixel_ref) override {
+    base::ScopedAllowBaseSyncPrimitivesForTesting allow_sync;
+    decode_started_.Signal();
+    allow_finish_.Wait();
+    return FakePaintImageGenerator::GetPixels(dst_pixmap, frame_index,
+                                              client_id, lazy_pixel_ref);
+  }
+
+  void WaitUntilDecodeStarted() {
+    base::ScopedAllowBaseSyncPrimitivesForTesting allow_sync;
+    decode_started_.Wait();
+  }
+  void AllowDecodeToFinish() { allow_finish_.Signal(); }
+  void ResetEvents() {
+    decode_started_.Reset();
+    allow_finish_.Reset();
+  }
+
+ private:
+  ~BlockingPaintImageGenerator() override = default;
+
+  base::WaitableEvent decode_started_;
+  base::WaitableEvent allow_finish_;
+};
+
+TEST_F(SoftwareImageDecodeCacheTest,
+       OrphanedRequestStaleCompletionWhileDecoding) {
+  auto origin_task_runner = base::SequencedTaskRunner::GetCurrentDefault();
+  auto worker_task_runner =
+      base::ThreadPool::CreateSequencedTaskRunner(base::TaskTraits());
+
+  ImageController controller(origin_task_runner, worker_task_runner,
+                             base::DoNothing());
+  TestSoftwareImageDecodeCache cache1;
+  controller.SetImageDecodeCache(&cache1);
+
+  auto generator = sk_make_sp<BlockingPaintImageGenerator>(
+      SkImageInfo::MakeN32Premul(100, 100, DefaultSkColorSpace()));
+  PaintImage paint_image = PaintImageBuilder::WithDefault()
+                               .set_id(PaintImage::GetNextId())
+                               .set_paint_image_generator(generator)
+                               .TakePaintImage();
+  DrawImage draw_image(
+      paint_image, false,
+      SkIRect::MakeWH(paint_image.width(), paint_image.height()),
+      PaintFlags::FilterQuality::kLow,
+      CreateMatrix(SkSize::Make(1.f, 1.f), true),
+      PaintImage::kDefaultFrameIndex, DefaultTargetColorParams());
+
+  // Queue an image decode on `cache1` and wait until the worker thread starts
+  // decoding.
+  bool callback_called = false;
+  controller.QueueImageDecode(
+      draw_image,
+      base::BindLambdaForTesting(
+          [&](ImageController::ImageDecodeRequestId id,
+              ImageController::ImageDecodeResult result) {
+            callback_called = true;
+            EXPECT_EQ(result, ImageController::ImageDecodeResult::SUCCESS);
+            controller.UnlockImageDecode(id);
+          }),
+      /*speculative=*/false);
+  generator->WaitUntilDecodeStarted();
+
+  // Allow the first decode on the worker thread to finish. When
+  // `RunOnWorkerThread()` returns, the worker posts a stale
+  // `ImageDecodeCompleted(id)` task to `origin_task_runner`. Before running
+  // `origin_task_runner`, simulate a compositor frame sink loss and recreation,
+  // `SetImageDecodeCache(nullptr)` followed by `SetImageDecodeCache(&cache2)`.
+  // `StopWorkerTasks()` completes the first decode and re-queues the request as
+  // an orphaned request with the same `id`.
+  generator->AllowDecodeToFinish();
+  controller.SetImageDecodeCache(nullptr);
+
+  TestSoftwareImageDecodeCache cache2;
+
+  // Reset the blocking events for the second decode of `draw_image` when the
+  // orphaned request is regenerated on `cache2`.
+  generator->ResetEvents();
+  controller.SetImageDecodeCache(&cache2);
+
+  // Wait until the worker thread picks up the regenerated request (which
+  // reuses the same `id` and is now in `requests_needing_completion[id]`) and
+  // enters `DecodeImageIfNecessary()` with `cache2.lock_` unlocked.
+  generator->WaitUntilDecodeStarted();
+
+  // Run `origin_task_runner`. The stale `ImageDecodeCompleted(id)` task from
+  // previous step must be cancelled by `StopWorkerTasks()` so it does not
+  // prematurely complete and unref the in-progress request on `cache2`.
+  base::RunLoop run_loop;
+  origin_task_runner->PostTask(FROM_HERE, run_loop.QuitWhenIdleClosure());
+  run_loop.Run();
+  EXPECT_FALSE(callback_called);
+
+  // Populate `cache2` with >1000 unlocked entries. Because `draw_image` still
+  // has `ref_count > 0`, `ReduceCacheUsageUntilWithinLimit()` must not evict
+  // or free its `CacheEntry` while `DecodeImageIfNecessary()` is running.
+  for (size_t i = 0; i < cache2.GetMaxNumCacheEntriesForTesting(); ++i) {
+    PaintImage small_image = CreatePaintImage(1, 1);
+    DrawImage small_draw_image(small_image, false, SkIRect::MakeWH(1, 1),
+                               PaintFlags::FilterQuality::kLow,
+                               CreateMatrix(SkSize::Make(1.f, 1.f), true),
+                               PaintImage::kDefaultFrameIndex,
+                               DefaultTargetColorParams());
+    DecodedDrawImage small_decoded =
+        cache2.GetDecodedImageForDraw(small_draw_image);
+    cache2.DrawWithImageFinished(small_draw_image, small_decoded);
+  }
+
+  // Now let the worker thread finish `DoDecodeImage()` on `cache2` and verify
+  // the real completion callback runs cleanly.
+  generator->AllowDecodeToFinish();
+  controller.FlushDecodeTasksForTesting();
+  EXPECT_TRUE(callback_called);
+  controller.SetImageDecodeCache(nullptr);
+}
+
 }  // namespace
 }  // namespace cc