From 02b14267cb023e7ec9fc52618bd12464ceb6afb7 Mon Sep 17 00:00:00 2001
From: Thomas Smith <[email protected]>
Date: Mon, 12 Oct 2026 11:23:25 -0400
Subject: [PATCH] [ganesh] Imagination FBO workaround v2

Overhaul ensure_previous_framebuffer_not_deleted workaround to prevent
driver use-after-free on FBO deletion.

The previous workaround had several flaws:
1. It only rebound FBO 0 immediately before deletion, which actually left the FBO being deleted as the driver's "previously bound" FBO. Imagination drivers retain an internal reference to the previous complete FBO, leading to a use-after-free when that FBO is deleted.

2. In surfaceless contexts, FBO 0 is undefined/incomplete, so the driver
retains its reference to the earlier complete FBO anyway.

3. It only checked the draw framebuffer binding. Read framebuffers left
bound after blits or MSAA resolves were deleted while still bound.

4. Context resets caused Ganesh to assume FBO 0 without issuing a bind,
causing subsequent deletions to skip the unbind check.

To fix this, mirror Chromium's GLES2DecoderImpl workaround:
- Allocate a 1x1 complete guard FBO backed by a renderbuffer (to avoid disturbing texture state) that is held for the context's lifetime and intentionally never deleted.

- In bindFramebuffer(), pre-bind the guard FBO before every target binding so no Skia FBO is ever left behind as the driver's previous binding.

- In deleteFramebuffer(), unconditionally rebind FBO 0 (which pre-binds the guard FBO), ensuring the FBO being deleted is neither current nor previous on either the draw or read target.

- Add GLFramebufferDeletionTest to validate binding invariants and probe driver behavior.

Bug: b/569165786
Change-Id: Ic515d1a6cb5d3fe401bc746e80eac2c7beaa11e0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/1383616
Reviewed-by: Robert Phillips <[email protected]>
Commit-Queue: Thomas Smith <[email protected]>
---
diff --git a/src/gpu/ganesh/gl/GrGLGpu.cpp b/src/gpu/ganesh/gl/GrGLGpu.cpp
index 2dd155e..00238e6 100644
--- a/src/gpu/ganesh/gl/GrGLGpu.cpp
+++ b/src/gpu/ganesh/gl/GrGLGpu.cpp
@@ -697,6 +697,12 @@
         fHWBoundRenderTargetUniqueID.makeInvalid();
         fHWSRGBFramebuffer = kUnknown_TriState;
         fBoundDrawFramebuffer = 0;
+        // GrGpu starts with every reset bit dirty and all entry points call handleDirtyContext(),
+        // so the first reset happens before Ganesh binds any framebuffer.
+        if (this->caps()->workarounds().ensure_previous_framebuffer_not_deleted &&
+            !fTriedToCreateCompleteFBO) {
+            this->createCompleteFramebuffer();
+        }
     }
 
     // we assume these values
@@ -3204,7 +3210,75 @@
 #endif
 }
 
+void GrGLGpu::createCompleteFramebuffer() {
+    SkASSERT(this->caps()->workarounds().ensure_previous_framebuffer_not_deleted);
+    SkASSERT(!fTriedToCreateCompleteFBO && !fCompleteFBOID);
+    fTriedToCreateCompleteFBO = true;
+
+    GrGLuint fboid = 0;
+    GrGLuint rbid = 0;
+    GL_CALL(GenFramebuffers(1, &fboid));
+    GL_CALL(GenRenderbuffers(1, &rbid));
+    if (!fboid || !rbid) {
+        // Neither object has been bound, so deleting them can't trigger the driver bug.
+        if (fboid) {
+            GL_CALL(DeleteFramebuffers(1, &fboid));
+        }
+        if (rbid) {
+            GL_CALL(DeleteRenderbuffers(1, &rbid));
+        }
+        return;
+    }
+
+    // Chromium attaches a texture; a renderbuffer doesn't disturb texture unit or pixel unpack
+    // state. RGBA4 is color-renderable in core ES 2.0, but prefer RGBA8 where it is supported.
+    GrGLenum format = this->glCaps().canFormatBeFBOColorAttachment(GrGLFormat::kRGBA8)
+                              ? GR_GL_RGBA8
+                              : GR_GL_RGBA4;
+    GL_CALL(BindRenderbuffer(GR_GL_RENDERBUFFER, rbid));
+    GL_CALL(RenderbufferStorage(GR_GL_RENDERBUFFER, format, 1, 1));
+
+    // Bind directly; bindFramebuffer() would try to pre-bind the complete FBO.
+    this->invalidateBoundRenderTarget();
+    GL_CALL(BindFramebuffer(GR_GL_FRAMEBUFFER, fboid));
+    fBoundDrawFramebuffer = fboid;
+    this->onFBOChanged();
+    GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_COLOR_ATTACHMENT0,
+                                    GR_GL_RENDERBUFFER, rbid));
+
+#if defined(__has_feature)
+#define SK_GL_COMPLETE_FBO_IS_TSAN __has_feature(thread_sanitizer)
+#else
+#define SK_GL_COMPLETE_FBO_IS_TSAN 0
+#endif
+// See skbug.com/40043725
+#if defined(SK_DEBUG) && !SK_GL_COMPLETE_FBO_IS_TSAN
+    if (!this->glCaps().skipErrorChecks()) {
+        GrGLenum status;
+        GL_CALL_RET(status, CheckFramebufferStatus(GR_GL_FRAMEBUFFER));
+        if (status != GR_GL_FRAMEBUFFER_COMPLETE) {
+            // Keep using it: pre-binding an incomplete FBO is no worse than not pre-binding.
+            SkDebugf("WARNING: ensure_previous_framebuffer_not_deleted FBO is incomplete (0x%x)\n",
+                     status);
+        }
+        if (this->glCaps().rebindColorAttachmentAfterCheckFramebufferStatus()) {
+            GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_COLOR_ATTACHMENT0,
+                                            GR_GL_RENDERBUFFER, rbid));
+        }
+    }
+#endif
+#undef SK_GL_COMPLETE_FBO_IS_TSAN
+
+    fCompleteFBOID = fboid;
+}
+
 void GrGLGpu::bindFramebuffer(GrGLenum target, GrGLuint fboid) {
+    if (fCompleteFBOID && fboid != fCompleteFBOID) {
+        // ensure_previous_framebuffer_not_deleted: make the complete FBO the previous binding of
+        // this target so the driver never retains a reference to an FBO we may later delete.
+        // This mirrors Chromium's GLES2DecoderImpl::BindFramebuffer().
+        GL_CALL(BindFramebuffer(target, fCompleteFBOID));
+    }
     GL_CALL(BindFramebuffer(target, fboid));
     if (target == GR_GL_FRAMEBUFFER || target == GR_GL_DRAW_FRAMEBUFFER) {
         fBoundDrawFramebuffer = fboid;
@@ -3220,31 +3294,39 @@
         this->caps()->workarounds().flush_queries_before_deleting_or_unbinding_fbo) {
         this->forcefullyFlushQueries();
     }
-    if (fboid == fBoundDrawFramebuffer) {
-        if (this->caps()->workarounds().unbind_attachments_on_bound_render_fbo_delete) {
-            // This workaround only applies to deleting currently bound framebuffers
-            // on Adreno 420.  Because this is a somewhat rare case, instead of
-            // tracking all the attachments of every framebuffer instead just always
-            // unbind all attachments.
-            GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_COLOR_ATTACHMENT0,
-                                            GR_GL_RENDERBUFFER, 0));
-            GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_STENCIL_ATTACHMENT,
-                                            GR_GL_RENDERBUFFER, 0));
-            GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_DEPTH_ATTACHMENT,
-                                            GR_GL_RENDERBUFFER, 0));
-        }
+    if (fboid == fBoundDrawFramebuffer &&
+        this->caps()->workarounds().unbind_attachments_on_bound_render_fbo_delete) {
+        // This workaround only applies to deleting currently bound framebuffers
+        // on Adreno 420.  Because this is a somewhat rare case, instead of
+        // tracking all the attachments of every framebuffer instead just always
+        // unbind all attachments.
+        GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_COLOR_ATTACHMENT0,
+                                        GR_GL_RENDERBUFFER, 0));
+        GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_STENCIL_ATTACHMENT,
+                                        GR_GL_RENDERBUFFER, 0));
+        GL_CALL(FramebufferRenderbuffer(GR_GL_FRAMEBUFFER, GR_GL_DEPTH_ATTACHMENT,
+                                        GR_GL_RENDERBUFFER, 0));
+    }
 
-        if (this->caps()->workarounds().ensure_previous_framebuffer_not_deleted) {
-            // Some drivers keep an internal reference to the previously bound
-            // framebuffer, so make sure it isn't the one being deleted.
-            this->bindFramebuffer(GR_GL_FRAMEBUFFER, 0);
-        }
+    if (this->caps()->workarounds().ensure_previous_framebuffer_not_deleted) {
+        // Some drivers keep an internal reference to the previously bound framebuffer, so make
+        // sure the FBO being deleted is neither the current nor the previous binding of either
+        // target. fBoundDrawFramebuffer isn't enough to decide whether that's needed: the read
+        // binding isn't tracked (blits and MSAA resolves leave their source FBO bound to
+        // GL_READ_FRAMEBUFFER), a context reset assumes 0 without binding it, and the FBO may be
+        // the previous binding even when it isn't the current one. So always rebind 0 (which
+        // pre-binds fCompleteFBOID), leaving current == 0 and previous == fCompleteFBOID for
+        // both targets.
+        SkASSERT(!fCompleteFBOID || fboid != fCompleteFBOID);
+        this->invalidateBoundRenderTarget();
+        this->bindFramebuffer(GR_GL_FRAMEBUFFER, 0);
     }
 
     GL_CALL(DeleteFramebuffers(1, &fboid));
 
     // Deleting the currently bound framebuffer rebinds to 0.
     if (fboid == fBoundDrawFramebuffer) {
+        fBoundDrawFramebuffer = 0;
         this->onFBOChanged();
     }
 }
diff --git a/src/gpu/ganesh/gl/GrGLGpu.h b/src/gpu/ganesh/gl/GrGLGpu.h
index 56ac7ce..a2d3213 100644
--- a/src/gpu/ganesh/gl/GrGLGpu.h
+++ b/src/gpu/ganesh/gl/GrGLGpu.h
@@ -269,6 +269,10 @@
 
     std::unique_ptr<GrSemaphore> prepareTextureForCrossContextUsage(GrTexture*) override;
 
+    // All of Ganesh's framebuffer binds and deletions must go through these two functions. With
+    // ensure_previous_framebuffer_not_deleted enabled, bindFramebuffer() binds fCompleteFBOID
+    // before the requested FBO, and deleteFramebuffer() first rebinds 0, so the FBO being deleted
+    // is never the current or previous binding of either the draw or read target.
     void bindFramebuffer(GrGLenum fboTarget, GrGLuint fboid);
     void deleteFramebuffer(GrGLuint fboid);
 
@@ -363,6 +367,10 @@
 
     void onFBOChanged();
 
+    // Creates fCompleteFBOID for ensure_previous_framebuffer_not_deleted. Called at most once, from
+    // the first onResetContext() that resets kRenderTarget_GrGLBackendState.
+    void createCompleteFramebuffer();
+
     // Returns whether the texture is successfully created. On success, a non-zero texture ID is
     // returned. On failure, zero is returned.
     // The texture is populated with |texels|, if it is non-null.
@@ -637,6 +645,16 @@
 
     GrGLuint                    fStencilClearFBOID;
 
+    // ensure_previous_framebuffer_not_deleted: some drivers (Imagination) keep a reference to the
+    // previously bound *complete* framebuffer and can touch it after it has been deleted. Like
+    // Chromium's GLES2DecoderImpl (gpu_driver_bug_list.json entry 471), bindFramebuffer() binds
+    // this 1x1 complete FBO before every other framebuffer, so it is always the "previous" binding
+    // and no Skia FBO is ever the previous binding when it is deleted. It is created by the first
+    // onResetContext() and is intentionally never deleted (nor is its renderbuffer): the client
+    // owns the GL context, and deleting it would leave a deleted FBO as the previous binding.
+    GrGLuint                    fCompleteFBOID = 0;
+    bool                        fTriedToCreateCompleteFBO = false;
+
     // last scissor / viewport scissor state seen by the GL.
     struct {
         TriState fEnabled;
diff --git a/tests/GLFramebufferDeletionTest.cpp b/tests/GLFramebufferDeletionTest.cpp
new file mode 100644
index 0000000..25d97d7
--- /dev/null
+++ b/tests/GLFramebufferDeletionTest.cpp
@@ -0,0 +1,644 @@
+/*
+ * Copyright 2026 Google LLC
+ *
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.
+ */
+
+// Regression test for b/532941869.
+//
+// Imagination GL drivers can keep an internal reference to the previously bound (complete)
+// framebuffer and touch it after it has been deleted, which turns into a use-after-free inside the
+// driver. Chromium works around the same bug (gpu_driver_bug_list.json entry 471: "IMG drivers can
+// sometimes reference previously bound complete framebuffers"). Skia's
+// ensure_previous_framebuffer_not_deleted workaround does what Chromium's GLES2DecoderImpl does:
+//   - GrGLGpu::bindFramebuffer() binds a never-deleted 1x1 complete FBO before every framebuffer it
+//     binds, so a Skia FBO is never left behind as the previous binding of either target.
+//   - GrGLGpu::deleteFramebuffer() always rebinds FBO 0 first, so the FBO being deleted is not the
+//     current binding of the draw or the read target either.
+//
+// The invariant checked here: an FBO is never deleted while it is the current *or* the previous
+// binding of GL_DRAW_FRAMEBUFFER or GL_READ_FRAMEBUFFER.
+//
+// GLFramebufferDeletion runs Skia-level scenarios on a GrDirectContext with
+// ensure_previous_framebuffer_not_deleted forced on (on every GPU) and hooks glBindFramebuffer and
+// glDeleteFramebuffers to verify the invariant on every deletion.
+// Deleting a client-owned FBO while Skia still has it bound is up to the client to avoid.
+
+#include "tests/Test.h"
+
+#ifdef SK_GL
+
+#include "include/core/SkCanvas.h"
+#include "include/core/SkColor.h"
+#include "include/core/SkColorSpace.h"
+#include "include/core/SkImageInfo.h"
+#include "include/core/SkPaint.h"
+#include "include/core/SkRect.h"
+#include "include/core/SkRefCnt.h"
+#include "include/core/SkSize.h"
+#include "include/core/SkSurface.h"
+#include "include/gpu/GpuTypes.h"
+#include "include/gpu/ganesh/GrBackendSurface.h"
+#include "include/gpu/ganesh/GrContextOptions.h"
+#include "include/gpu/ganesh/GrDirectContext.h"
+#include "include/gpu/ganesh/GrTypes.h"
+#include "include/gpu/ganesh/SkSurfaceGanesh.h"
+#include "include/gpu/ganesh/gl/GrGLBackendSurface.h"
+#include "include/gpu/ganesh/gl/GrGLDirectContext.h"
+#include "include/gpu/ganesh/gl/GrGLFunctions.h"
+#include "include/gpu/ganesh/gl/GrGLInterface.h"
+#include "include/gpu/ganesh/gl/GrGLTypes.h"
+#include "include/private/gpu/ganesh/GrTypesPriv.h"
+#include "src/gpu/ganesh/GrDirectContextPriv.h"
+#include "src/gpu/ganesh/GrGpu.h"
+#include "src/gpu/ganesh/GrSamplerState.h"
+#include "src/gpu/ganesh/GrTexture.h"
+#include "src/gpu/ganesh/gl/GrGLCaps.h"
+#include "src/gpu/ganesh/gl/GrGLDefines.h"
+#include "src/gpu/ganesh/gl/GrGLGpu.h"
+#include "src/gpu/ganesh/gl/GrGLUtil.h"
+#include "tests/CtsEnforcement.h"
+#include "tools/ganesh/gl/GLTestContext.h"
+
+#include <algorithm>
+#include <vector>
+
+namespace {
+
+// GL_READ_FRAMEBUFFER_BINDING (GL/GLES 3.0 and the framebuffer_blit extensions).
+// GR_GL_FRAMEBUFFER_BINDING has the same value as GL_DRAW_FRAMEBUFFER_BINDING.
+constexpr GrGLenum kReadFramebufferBinding = 0x8CAA;
+
+constexpr int kSkiaIterations = 32;
+
+// GrGLGpu doesn't call glCheckFramebufferStatus under TSAN (see skbug.com/40043725); neither do we.
+#if defined(__has_feature)
+#if __has_feature(thread_sanitizer)
+constexpr bool kCanCheckFramebufferStatus = false;
+#else
+constexpr bool kCanCheckFramebufferStatus = true;
+#endif
+#else
+constexpr bool kCanCheckFramebufferStatus = true;
+#endif
+
+bool has_separate_read_draw_targets(const GrGLInterface* gl) {
+    // GL/GLES 3.0, or one of the GLES 2.0 extensions that add GL_READ_FRAMEBUFFER and
+    // GL_DRAW_FRAMEBUFFER (EXT/ANGLE/NV_framebuffer_blit, APPLE_framebuffer_multisample).
+    return GrGLGetVersion(gl) >= GR_GL_VER(3, 0) ||
+           static_cast<bool>(gl->fFunctions.fBlitFramebuffer) ||
+           static_cast<bool>(gl->fFunctions.fResolveMultisampleFramebuffer);
+}
+
+// Makes sure TexImage2D/ReadPixels don't read from or write into a pixel buffer object.
+void unbind_pixel_buffers(const GrGLInterface* gl) {
+    const bool hasPBOs = gl->fStandard == kGL_GrGLStandard
+                                 ? GrGLGetVersion(gl) >= GR_GL_VER(2, 1)
+                                 : GrGLGetVersion(gl) >= GR_GL_VER(3, 0);
+    if (hasPBOs) {
+        GR_GL_CALL(gl, BindBuffer(GR_GL_PIXEL_PACK_BUFFER, 0));
+        GR_GL_CALL(gl, BindBuffer(GR_GL_PIXEL_UNPACK_BUFFER, 0));
+    }
+}
+
+// Creates an uninitialized size x size RGBA texture.
+GrGLuint make_texture(const GrGLInterface* gl, int size) {
+    GrGLuint tex = 0;
+    GR_GL_CALL(gl, GenTextures(1, &tex));
+    GR_GL_CALL(gl, BindTexture(GR_GL_TEXTURE_2D, tex));
+    GR_GL_CALL(gl, TexParameteri(GR_GL_TEXTURE_2D, GR_GL_TEXTURE_MIN_FILTER, GR_GL_NEAREST));
+    GR_GL_CALL(gl, TexParameteri(GR_GL_TEXTURE_2D, GR_GL_TEXTURE_MAG_FILTER, GR_GL_NEAREST));
+    GR_GL_CALL(gl, TexImage2D(GR_GL_TEXTURE_2D, 0, GR_GL_RGBA, size, size, 0, GR_GL_RGBA,
+                              GR_GL_UNSIGNED_BYTE, nullptr));
+    GR_GL_CALL(gl, BindTexture(GR_GL_TEXTURE_2D, 0));
+    return tex;
+}
+
+// Creates an FBO with 'tex' as its color attachment and leaves it bound to GL_FRAMEBUFFER.
+// Returns 0 if the FBO is incomplete.
+GrGLuint make_fbo(const GrGLInterface* gl, GrGLuint tex) {
+    GrGLuint fbo = 0;
+    GR_GL_CALL(gl, GenFramebuffers(1, &fbo));
+    GR_GL_CALL(gl, BindFramebuffer(GR_GL_FRAMEBUFFER, fbo));
+    GR_GL_CALL(gl, FramebufferTexture2D(GR_GL_FRAMEBUFFER, GR_GL_COLOR_ATTACHMENT0,
+                                        GR_GL_TEXTURE_2D, tex, 0));
+    if (kCanCheckFramebufferStatus) {
+        GrGLenum status;
+        GR_GL_CALL_RET(gl, status, CheckFramebufferStatus(GR_GL_FRAMEBUFFER));
+        if (status != GR_GL_FRAMEBUFFER_COMPLETE) {
+            GR_GL_CALL(gl, DeleteFramebuffers(1, &fbo));
+            return 0;
+        }
+    }
+    return fbo;
+}
+
+// Tracks the current and previous binding of GL_DRAW_FRAMEBUFFER and GL_READ_FRAMEBUFFER through a
+// hooked GrGLInterface, and records both at every glDeleteFramebuffers call.
+class FBOBindingWatcher {
+public:
+    // A binding we know nothing about. Never matches a real FBO.
+    static constexpr GrGLuint kUnknownFBO = ~0u;
+
+    struct Binding {
+        GrGLuint fCurrent = kUnknownFBO;
+        GrGLuint fPrevious = kUnknownFBO;
+    };
+
+    struct Deletion {
+        GrGLuint fFBO;
+        Binding fDraw;
+        Binding fRead;
+
+        bool currentDraw() const { return fFBO == fDraw.fCurrent; }
+        bool currentRead() const { return fFBO == fRead.fCurrent; }
+        bool previousDraw() const { return fFBO == fDraw.fPrevious; }
+        bool previousRead() const { return fFBO == fRead.fPrevious; }
+        bool unsafe() const {
+            return this->currentDraw() || this->currentRead() || this->previousDraw() ||
+                   this->previousRead();
+        }
+    };
+
+    // Returns a copy of 'base' that routes glBindFramebuffer and glDeleteFramebuffers through this
+    // watcher. The watcher must outlive any GrDirectContext created with the returned interface,
... (file diff truncated)