From 9cd9ad93385823a2af51f308d288de8da481a7fa Mon Sep 17 00:00:00 2001
From: vikas soni <[email protected]>
Date: Tue, 12 May 2026 13:26:53 -0700
Subject: [PATCH] [GPU Security]: Fix thread-safety issues in CompoundImageBacking.

This CL addresses potential security vulnerabilities where a raw
SharedImageBackingFactory pointer could escape the synchronization scope
or a thread-affine WeakPtr be used across threads.

The fix involves:

1. Moving dynamic backing allocation logic entirely inside the
SharedImageFactoryRef::Execute lambda in GetOrAllocateBacking.

2. Consolidating backing creation into a single method that assumes the
factory lock is held.

3. Updating the lazy allocation callback to use SharedImageFactoryRef to
safely look up the correct factory by type under lock.

4. Adding SharedImageFactory::GetFactoryByType to support safe lookup.

These changes ensure that all factory-dependent operations in
CompoundImageBacking are performed under the factory lock and are
thread-safe.

Bug: 511766407
Change-Id: Iab94d024f7cb5c43e17a4237f5572c80bf96b373
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7838134
Reviewed-by: Vasiliy Telezhnikov <[email protected]>
Commit-Queue: vikas soni <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1629530}
---

diff --git a/gpu/command_buffer/service/shared_image/compound_image_backing.cc b/gpu/command_buffer/service/shared_image/compound_image_backing.cc
index 45c1821..3846f7a 100644
--- a/gpu/command_buffer/service/shared_image/compound_image_backing.cc
+++ b/gpu/command_buffer/service/shared_image/compound_image_backing.cc
@@ -6,6 +6,7 @@
 
 #include <ostream>
 
+#include "base/check_is_test.h"
 #include "base/feature_list.h"
 #include "base/functional/bind.h"
 #include "base/logging.h"
@@ -1021,6 +1022,7 @@
           std::move(buffer_usage)),
       shared_image_factory_(std::move(shared_image_factory)),
       copy_manager_(std::move(copy_manager)) {
+  CHECK(gpu_backing_factory);
   // If the backing is thread-safe, the base class enables an internal lock that
   // protects the |elements_| vector and other metadata from concurrent access.
   DCHECK(shm_backing);
@@ -1050,10 +1052,11 @@
 
   // CreateBackingFromBackingFactory will be called on demand. Hence this is
   // lazy backing creation.
-  gpu_element.create_callback =
-      base::BindOnce(&CompoundImageBacking::CreateBackingFromBackingFactory,
-                     base::Unretained(this), std::move(gpu_backing_factory),
-                     si_info.debug_label, GetGpuSharedImageUsage(usage));
+  SharedImageBackingType factory_type = gpu_backing_factory->GetBackingType();
+  gpu_element.create_callback = base::BindOnce(
+      &CompoundImageBacking::LazyCreateBacking, base::Unretained(this),
+      factory_type, std::move(gpu_backing_factory),
+      GetGpuSharedImageUsage(usage), si_info.debug_label);
   elements_.push_back(std::move(gpu_element));
   max_elements_allocated_ = 2;
 }
@@ -1851,51 +1854,51 @@
   if (base::FeatureList::IsEnabled(features::kUseDynamicBackingAllocations) &&
       shared_image_factory_) {
     SharedImageUsageSet usage = GetUsageFromAccessStream(stream);
-    SharedImageBackingFactory* gpu_backing_factory = nullptr;
+    std::unique_ptr<SharedImageBacking> new_backing;
     shared_image_factory_->Execute([&](SharedImageFactory* factory) {
-      gpu_backing_factory = factory->GetFactoryByUsage(
-          usage, format(), size(),
-          /*pixel_data=*/{}, gfx::EMPTY_BUFFER, stream, &params);
+      SharedImageBackingFactory* gpu_backing_factory =
+          factory->GetFactoryByUsage(usage, format(), size(),
+                                     /*pixel_data=*/{}, gfx::EMPTY_BUFFER,
+                                     stream, &params);
+      if (gpu_backing_factory) {
+        CreateBackingFromBackingFactory(gpu_backing_factory, debug_label(),
+                                        usage, new_backing);
+      }
     });
 
-    if (gpu_backing_factory) {
-      std::unique_ptr<SharedImageBacking> new_backing;
-      CreateBackingFromBackingFactory(gpu_backing_factory->GetWeakPtr(),
-                                      debug_label(), usage, new_backing);
-      if (new_backing) {
-        UMA_HISTOGRAM_ENUMERATION(
-            "GPU.CompoundImageBacking.DynamicAllocation.BackingType",
-            new_backing->GetType());
-        UMA_HISTOGRAM_ENUMERATION(
-            "GPU.CompoundImageBacking.DynamicAllocation.AccessStream", stream);
-        UMA_HISTOGRAM_SPARSE(
-            "GPU.CompoundImageBacking.DynamicAllocation."
-            "InitialSharedImageUsage",
-            static_cast<int32_t>(static_cast<uint32_t>(this->usage())));
+    if (new_backing) {
+      UMA_HISTOGRAM_ENUMERATION(
+          "GPU.CompoundImageBacking.DynamicAllocation.BackingType",
+          new_backing->GetType());
+      UMA_HISTOGRAM_ENUMERATION(
+          "GPU.CompoundImageBacking.DynamicAllocation.AccessStream", stream);
+      UMA_HISTOGRAM_SPARSE(
+          "GPU.CompoundImageBacking.DynamicAllocation."
+          "InitialSharedImageUsage",
+          static_cast<int32_t>(static_cast<uint32_t>(this->usage())));
 
-        // If the CSI container is thread-safe, we treat newly created backings
-        // as transient if they are not thread-safe. They will be owned by the
-        // representation and destroyed after use. This ensures that a
-        // non-thread-safe backing allocated on one thread doesn't persist in
-        // the thread-safe container, which could lead to race conditions if
-        // accessed from another thread later.
-        if (is_thread_safe() && !new_backing->is_thread_safe()) {
-          out_transient_backing = std::move(new_backing);
-          return out_transient_backing.get();
-        }
-
-        // Else we treat the backing as non-transient and add it to the list of
-        // alive elements. This is done when either the container is not
-        // thread-safe or the new backing itself is thread-safe.
-        ElementHolder element;
-        element.access_streams.Put(stream);
-        element.backing = std::move(new_backing);
-        elements_.push_back(std::move(element));
-        if (elements_.size() > max_elements_allocated_) {
-          max_elements_allocated_ = elements_.size();
-        }
-        return elements_.back().GetBacking();
+      // If the CSI container is thread-safe, we treat newly created backings
+      // as transient if they are not thread-safe. They will be owned by the
+      // representation and destroyed after use. This ensures that a
+      // non-thread-safe backing allocated on one thread doesn't persist in
+      // the thread-safe container, which could lead to race conditions if
+      // accessed from another thread later.
+      if (is_thread_safe() && !new_backing->is_thread_safe()) {
+        out_transient_backing = std::move(new_backing);
+        return out_transient_backing.get();
       }
+
+      // Else we treat the backing as non-transient and add it to the list of
+      // alive elements. This is done when either the container is not
+      // thread-safe or the new backing itself is thread-safe.
+      ElementHolder element;
+      element.access_streams.Put(stream);
+      element.backing = std::move(new_backing);
+      elements_.push_back(std::move(element));
+      if (elements_.size() > max_elements_allocated_) {
+        max_elements_allocated_ = elements_.size();
+      }
+      return elements_.back().GetBacking();
     }
   }
 
@@ -1914,19 +1917,19 @@
 }
 
 void CompoundImageBacking::CreateBackingFromBackingFactory(
-    base::WeakPtr<SharedImageBackingFactory> factory,
+    SharedImageBackingFactory* backing_factory,
     std::string debug_label,
     SharedImageUsageSet usage,
     std::unique_ptr<SharedImageBacking>& backing) {
-  if (!factory) {
-    DLOG(ERROR) << "Can't allocate backing after image has been destroyed";
-    return;
-  }
+  // This method assumes the caller has already ensured the factory is alive
+  // and synchronized (e.g. by holding the SharedImageFactoryRef lock).
+  CHECK(backing_factory);
 
   SharedImageInfo si_info(format(), size(), color_space(), surface_origin(),
-                          alpha_type(), usage, debug_label);
-  backing = factory->CreateSharedImage(mailbox(), si_info, kNullSurfaceHandle,
-                                       /*is_thread_safe=*/false);
+                          alpha_type(), usage, std::move(debug_label));
+  backing =
+      backing_factory->CreateSharedImage(mailbox(), si_info, kNullSurfaceHandle,
+                                         /*is_thread_safe=*/false);
   if (!backing) {
     DLOG(ERROR) << "Failed to allocate GPU backing";
     return;
@@ -1952,13 +1955,41 @@
   // Update peak GPU memory tracking with the new estimated size.
   size_t estimated_size = 0;
   for (auto& element : elements_) {
-    if (element.backing)
+    if (element.backing) {
       estimated_size += element.backing->GetEstimatedSize();
+    }
   }