From 013ad6e7097238654847bfa5a5fb2c6c3823ee41 Mon Sep 17 00:00:00 2001
From: Alexis Cruz-Ayala <[email protected]>
Date: Fri, 17 Jul 2026 12:51:34 -0400
Subject: [PATCH] [Security] Add a SmallPathIDChangeListener to SmallPathAtlasMgr

There was a vulnerability where SkPathID could have duplicate entries if an attacker created and discarded enough Small, Complex Paths to overwhelm the SmallPathAtlasMgr cache. This could lead to the attacker submitting a request to the cache for a path, and receiving a victim's path instead.

The solution involved mirroring what is done in the SoftwarePathRenderer.cpp and using an SkIDChangeListener to listen for when a path is modified or discarded and delete the entry in the cache. This way, duplicate entries are avoided.

A test was added that shows that an ID collision can be forced amongst SmallPaths with complex geometry; any hash of two small, complex paths would rely only on the genID, making it easier to hit a duplicate entry.

Bug: b/501759192
Change-Id: Id80a0167370dfca1b7dac511f1ddcfda16a72fc1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1294996
Reviewed-by: Florin Malita <[email protected]>
Commit-Queue: Alexis Cruz-Ayala <[email protected]>
---

diff --git a/src/gpu/ganesh/ops/SmallPathAtlasMgr.cpp b/src/gpu/ganesh/ops/SmallPathAtlasMgr.cpp
index 6fa8667..81ca65f 100644
--- a/src/gpu/ganesh/ops/SmallPathAtlasMgr.cpp
+++ b/src/gpu/ganesh/ops/SmallPathAtlasMgr.cpp
@@ -11,10 +11,12 @@
 #include "include/gpu/GpuTypes.h"
 #include "include/gpu/ganesh/GrBackendSurface.h"
 #include "include/gpu/ganesh/GrTypes.h"
+#include "include/private/SkIDChangeListener.h"
 #include "include/private/SkTo.h"
 #include "include/private/gpu/ganesh/GrTypesPriv.h"
 #include "src/gpu/MaskFormat.h"
 #include "src/gpu/ganesh/GrCaps.h"
+#include "src/gpu/ganesh/geometry/GrStyledShape.h"
 #include "src/gpu/ganesh/ops/SmallPathShapeData.h"
 
 #include <cstddef>
@@ -86,11 +88,19 @@
     delete shapeData;
 }
 
-SmallPathShapeData* SmallPathAtlasMgr::findOrCreate(const SmallPathShapeDataKey& key) {
+SmallPathShapeData* SmallPathAtlasMgr::findOrCreate(const SmallPathShapeDataKey& key,
+                                                    const GrStyledShape& shape) {
     auto shapeData = fShapeCache.find(key);
+
+    if (shapeData && shapeData->fIDChangeListener->hasChanged()) {
+        this->deleteCacheEntry(shapeData);
+        shapeData = nullptr;
+    }
+
     if (!shapeData) {
         // TODO: move the key into the ctor
         shapeData = new SmallPathShapeData(key);
+        shape.addGenIDChangeListener(shapeData->fIDChangeListener);
         fShapeCache.add(shapeData);
         fShapeList.addToTail(shapeData);
 #ifdef DF_PATH_TRACKING
@@ -108,7 +118,7 @@
     SmallPathShapeDataKey key(shape, desiredDimension);
 
     // TODO: move the key into 'findOrCreate'
-    return this->findOrCreate(key);
+    return this->findOrCreate(key, shape);
 }
 
 SmallPathShapeData* SmallPathAtlasMgr::findOrCreate(const GrStyledShape& shape,
@@ -116,7 +126,7 @@
     SmallPathShapeDataKey key(shape, ctm);
 
     // TODO: move the key into 'findOrCreate'
-    return this->findOrCreate(key);
+    return this->findOrCreate(key, shape);
 }
 
 GrDrawOpAtlas::ErrorCode SmallPathAtlasMgr::addToAtlas(GrResourceProvider* resourceProvider,
diff --git a/src/gpu/ganesh/ops/SmallPathAtlasMgr.h b/src/gpu/ganesh/ops/SmallPathAtlasMgr.h
index c72fd42..9e17694 100644
--- a/src/gpu/ganesh/ops/SmallPathAtlasMgr.h
+++ b/src/gpu/ganesh/ops/SmallPathAtlasMgr.h
@@ -93,7 +93,7 @@
     void deleteCacheEntry(SmallPathShapeData*);
 
 private:
-    SmallPathShapeData* findOrCreate(const SmallPathShapeDataKey&);
+    SmallPathShapeData* findOrCreate(const SmallPathShapeDataKey&, const GrStyledShape&);
 
     void evict(GrPlotLocator) override;
 
diff --git a/src/gpu/ganesh/ops/SmallPathShapeData.h b/src/gpu/ganesh/ops/SmallPathShapeData.h
index 2828b0d..908c5ad 100644
--- a/src/gpu/ganesh/ops/SmallPathShapeData.h
+++ b/src/gpu/ganesh/ops/SmallPathShapeData.h
@@ -12,6 +12,7 @@
 #if !defined(SK_ENABLE_OPTIMIZE_SIZE)
 
 #include "include/core/SkRect.h"
+#include "include/private/SkIDChangeListener.h"
 #include "include/private/SkTemplates.h"
 #include "src/core/SkChecksum.h"
 #include "src/core/SkTInternalLList.h"
@@ -25,6 +26,18 @@
 
 namespace skgpu::ganesh {
 
+class SmallPathIDChangeListener : public SkIDChangeListener {
+public:
+    SmallPathIDChangeListener() : fHasChanged(false) {}
+
+    bool hasChanged() const { return fHasChanged.load(std::memory_order_relaxed); }
+
+    void changed() override { fHasChanged.store(true, std::memory_order_relaxed); }
+
+private:
+    std::atomic<bool> fHasChanged;
+};
+
 class SmallPathShapeDataKey {
 public:
     // TODO: add a move variant
@@ -58,7 +71,9 @@
 
 class SmallPathShapeData {
 public:
-    SmallPathShapeData(const SmallPathShapeDataKey& key) : fKey(key) {}
+    SmallPathShapeData(const SmallPathShapeDataKey& key)
+            : fKey(key), fIDChangeListener(sk_make_sp<SmallPathIDChangeListener>()) {}
+    ~SmallPathShapeData() { fIDChangeListener->markShouldDeregister(); }
 
     const SmallPathShapeDataKey fKey;
     SkRect                      fBounds;
@@ -73,6 +88,8 @@
     static inline uint32_t Hash(const SmallPathShapeDataKey& key) {
         return SkChecksum::Hash32(key.data(), sizeof(uint32_t) * key.count32());
     }
+
+    sk_sp<SmallPathIDChangeListener> fIDChangeListener;
 };
 
 }  // namespace skgpu::ganesh