Remove textureUseAfterFreePoolSize

google · Oct 2, 2024 · ec44b11 · ec44b11
1 parent 834f497
commit ec44b11
Show file tree

Hide file tree

Showing 7 changed files with 8 additions and 113 deletions.
diff --git a/filament/backend/include/backend/Platform.h b/filament/backend/include/backend/Platform.h
@@ -68,13 +68,6 @@ class UTILS_PUBLIC Platform {
          */
         size_t handleArenaSize = 0;
 
-        /**
-         * This number of most-recently destroyed textures will be tracked for use-after-free.
-         * Throws an exception when a texture is freed but still bound to a SamplerGroup and used in
-         * a draw call. 0 disables completely. Currently only respected by the Metal backend.
-         */
-        size_t textureUseAfterFreePoolSize = 0;
-
         size_t metalUploadBufferSizeBytes = 512 * 1024;
 
         /**

diff --git a/filament/backend/src/metal/MetalContext.h b/filament/backend/src/metal/MetalContext.h
@@ -118,9 +118,6 @@ class MetalDynamicOffsets {
 };
 
 struct MetalContext {
-    explicit MetalContext(size_t metalFreedTextureListSize)
-        : texturesToDestroy(metalFreedTextureListSize) {}
-
     MetalDriver* driver;
     id<MTLDevice> device = nullptr;
     id<MTLCommandQueue> commandQueue = nullptr;
@@ -187,14 +184,6 @@ struct MetalContext {
     // Keeps track of all alive textures.
     tsl::robin_set<MetalTexture*> textures;
 
-    // This circular buffer implements delayed destruction for Metal texture handles. It keeps a
-    // handle to a fixed number of the most recently destroyed texture handles. When we're asked to
-    // destroy a texture handle, we free its texture memory, but keep the MetalTexture object alive,
-    // marking it as "terminated". If we later are asked to use that texture, we can check its
-    // terminated status and throw an Objective-C error instead of crashing, which is helpful for
-    // debugging use-after-free issues in release builds.
-    utils::FixedCircularBuffer<Handle<HwTexture>> texturesToDestroy;
-
     MetalBufferPool* bufferPool;
     MetalBumpAllocator* bumpAllocator;
 

diff --git a/filament/backend/src/metal/MetalDriver.mm b/filament/backend/src/metal/MetalDriver.mm
@@ -105,13 +105,13 @@
     return ConcreteDispatcher<MetalDriver>::make();
 }
 
-MetalDriver::MetalDriver(MetalPlatform* platform, const Platform::DriverConfig& driverConfig) noexcept
-        : mPlatform(*platform),
-          mContext(new MetalContext(driverConfig.textureUseAfterFreePoolSize)),
-          mHandleAllocator("Handles",
-                  driverConfig.handleArenaSize,
-                  driverConfig.disableHandleUseAfterFreeCheck),
-          mStereoscopicType(driverConfig.stereoscopicType) {
+MetalDriver::MetalDriver(
+        MetalPlatform* platform, const Platform::DriverConfig& driverConfig) noexcept
+    : mPlatform(*platform),
+      mContext(new MetalContext),
+      mHandleAllocator(
+              "Handles", driverConfig.handleArenaSize, driverConfig.disableHandleUseAfterFreeCheck),
+      mStereoscopicType(driverConfig.stereoscopicType) {
     mContext->driver = this;
 
     TrackedMetalBuffer::setPlatform(platform);
@@ -347,9 +347,6 @@
 
     auto* descriptorSet = handle_cast<MetalDescriptorSet>(dsh);
     auto* texture = handle_cast<MetalTexture>(th);
-    texture->checkUseAfterFree(
-            "updateDescriptorSetTexture",
-            [&]() { return mHandleAllocator.getHandleTag(th.getId()).c_str_safe(); }, binding);
 
     id<MTLTexture> mtlTexture = texture->getMtlTextureForRead();
     if (texture->target == SamplerType::SAMPLER_EXTERNAL) {
@@ -460,8 +457,6 @@
 void MetalDriver::createTextureViewR(
         Handle<HwTexture> th, Handle<HwTexture> srch, uint8_t baseLevel, uint8_t levelCount) {
     MetalTexture const* src = handle_cast<MetalTexture>(srch);
-    src->checkUseAfterFree("createTextureView",
-            [&]() { return mHandleAllocator.getHandleTag(th.getId()).c_str_safe(); });
     mContext->textures.insert(
             construct_handle<MetalTexture>(th, *mContext, src, baseLevel, levelCount));
 }
@@ -470,8 +465,6 @@
         backend::TextureSwizzle r, backend::TextureSwizzle g, backend::TextureSwizzle b,
         backend::TextureSwizzle a) {
     MetalTexture const* src = handle_cast<MetalTexture>(srch);
-    src->checkUseAfterFree("createTextureViewSwizzle",
-            [&]() { return mHandleAllocator.getHandleTag(th.getId()).c_str_safe(); });
     mContext->textures.insert(construct_handle<MetalTexture>(th, *mContext, src, r, g, b, a));
 }
 
@@ -555,9 +548,6 @@
         FILAMENT_CHECK_PRECONDITION(buffer.handle)
                 << "The COLOR" << i << " flag was specified, but invalid color handle provided.";
         auto colorTexture = handle_cast<MetalTexture>(buffer.handle);
-        colorTexture->checkUseAfterFree("createRenderTarget.color", [&]() {
-            return mHandleAllocator.getHandleTag(buffer.handle.getId()).c_str_safe();
-        });
         FILAMENT_CHECK_PRECONDITION(colorTexture->getMtlTextureForWrite())
                 << "Color texture passed to render target has no texture allocation";
         colorAttachments[i] = { colorTexture, color[i].level, color[i].layer };
@@ -568,9 +558,6 @@
         FILAMENT_CHECK_PRECONDITION(depth.handle)
                 << "The DEPTH flag was specified, but invalid depth handle provided.";
         auto depthTexture = handle_cast<MetalTexture>(depth.handle);
-        depthTexture->checkUseAfterFree("createRenderTarget.depth", [&]() {
-            return mHandleAllocator.getHandleTag(depth.handle.getId()).c_str_safe();
-        });
         FILAMENT_CHECK_PRECONDITION(depthTexture->getMtlTextureForWrite())
                 << "Depth texture passed to render target has no texture allocation.";
         depthAttachment = { depthTexture, depth.level, depth.layer };
@@ -581,9 +568,6 @@
         FILAMENT_CHECK_PRECONDITION(stencil.handle)
                 << "The STENCIL flag was specified, but invalid stencil handle provided.";
         auto stencilTexture = handle_cast<MetalTexture>(stencil.handle);
-        stencilTexture->checkUseAfterFree("createRenderTarget.stencil", [&]() {
-            return mHandleAllocator.getHandleTag(stencil.handle.getId()).c_str_safe();
-        });
         FILAMENT_CHECK_PRECONDITION(stencilTexture->getMtlTextureForWrite())
                 << "Stencil texture passed to render target has no texture allocation.";
         stencilAttachment = { stencilTexture, stencil.level, stencil.layer };
@@ -816,15 +800,7 @@
     auto* metalTexture = handle_cast<MetalTexture>(th);
     mContext->textures.erase(metalTexture);
 
-    // Free memory from the texture and mark it as freed.
-    metalTexture->terminate();
-
-    // Add this texture handle to our texturesToDestroy queue to be destroyed later.
-    if (auto handleToFree = mContext->texturesToDestroy.push(th)) {
-        // If texturesToDestroy is full, then .push evicts the oldest texture handle in the
-        // queue (or simply th, if use-after-free detection is disabled).
-        destruct_handle<MetalTexture>(handleToFree.value());
-    }
+    destruct_handle<MetalTexture>(th);
 }
 
 void MetalDriver::destroyRenderTarget(Handle<HwRenderTarget> rth) {
@@ -874,12 +850,6 @@
 }
 
 void MetalDriver::terminate() {
-    // Terminate any outstanding MetalTextures.
-    while (!mContext->texturesToDestroy.empty()) {
-        Handle<HwTexture> toDestroy = mContext->texturesToDestroy.pop();
-        destruct_handle<MetalTexture>(toDestroy);
-    }
-
     // finish() will flush the pending command buffer and will ensure all GPU work has finished.
     // This must be done before calling bufferPool->reset() to ensure no buffers are in flight.
     finish();
@@ -1148,8 +1118,6 @@
     FILAMENT_CHECK_PRECONDITION(!isInRenderPass(mContext))
             << "update3DImage must be called outside of a render pass.";
     auto tex = handle_cast<MetalTexture>(th);
-    tex->checkUseAfterFree("update3DImage",
-            [&]() { return mHandleAllocator.getHandleTag(th.getId()).c_str_safe(); });
     tex->loadImage(level, MTLRegionMake3D(xoffset, yoffset, zoffset, width, height, depth), data);
     scheduleDestroy(std::move(data));
 
@@ -1186,8 +1154,6 @@
     FILAMENT_CHECK_PRECONDITION(!isInRenderPass(mContext))
             << "generateMipmaps must be called outside of a render pass.";
     auto tex = handle_cast<MetalTexture>(th);
-    tex->checkUseAfterFree("generateMipmaps",
-            [&]() { return mHandleAllocator.getHandleTag(th.getId()).c_str_safe(); });
     tex->generateMipmaps();
 
     DEBUG_LOG("generateMipmaps(th = %d)\n", th.getId());

diff --git a/filament/backend/src/metal/MetalHandles.h b/filament/backend/src/metal/MetalHandles.h
@@ -265,38 +265,7 @@ class MetalTexture : public HwTexture {
 
     MTLPixelFormat devicePixelFormat;
 
-    // Frees memory associated with this texture and marks it as "terminated".
-    // Used to track "use after free" scenario.
-    void terminate() noexcept;
-    bool isTerminated() const noexcept { return terminated; }
-    inline void checkUseAfterFree(const char* operation,
-            utils::Invocable<const char*(void)> getTextureTag, size_t textureIndex) const {
-        if (UTILS_LIKELY(!isTerminated())) {
-            return;
-        }
-        throwUseAfterFreeException([NSString
-                stringWithFormat:@"Filament Metal texture use after free, operation = %s, tag = "
-                                 @"%s, binding = %zu",
-                operation, getTextureTag(), textureIndex]);
-    }
-    inline void checkUseAfterFree(
-            const char* operation, utils::Invocable<const char*(void)> getTextureTag) const {
-        if (UTILS_LIKELY(!isTerminated())) {
-            return;
-        }
-        throwUseAfterFreeException([NSString
-                stringWithFormat:@"Filament Metal texture use after free, operation = %s, tag = %s",
-                operation, getTextureTag()]);
-    }
-
 private:
-    inline void throwUseAfterFreeException(NSString* reason) const {
-        NSException* useAfterFreeException =
-                [NSException exceptionWithName:@"MetalTextureUseAfterFree"
-                                        reason:reason
-                                      userInfo:nil];
-        [useAfterFreeException raise];
-    }
     void loadSlice(uint32_t level, MTLRegion region, uint32_t byteOffset, uint32_t slice,
             PixelBufferDescriptor const& data) noexcept;
     void loadWithCopyBuffer(uint32_t level, uint32_t slice, MTLRegion region, PixelBufferDescriptor const& data,
@@ -312,8 +281,6 @@ class MetalTexture : public HwTexture {
     // Filament swizzling only affects texture reads, so this should not be used when the texture is
     // bound as a render target attachment.
     id<MTLTexture> swizzledTextureView = nil;
-
-    bool terminated = false;
 };
 
 class MetalRenderTarget : public HwRenderTarget {

diff --git a/filament/backend/src/metal/MetalHandles.mm b/filament/backend/src/metal/MetalHandles.mm
@@ -634,14 +634,6 @@ static void func(void* user) {
     texture = externalImage->getMtlTexture();
 }
 
-void MetalTexture::terminate() noexcept {
-    texture = nil;
-    swizzledTextureView = nil;
-    msaaSidecar = nil;
-    externalImage = nullptr;
-    terminated = true;
-}
-
 id<MTLTexture> MetalTexture::getMtlTextureForRead() const noexcept {
     return swizzledTextureView ? swizzledTextureView : texture;
 }

diff --git a/filament/include/filament/Engine.h b/filament/include/filament/Engine.h
@@ -288,16 +288,6 @@ class UTILS_PUBLIC Engine {
          */
         uint32_t jobSystemThreadCount = 0;
 
-        /*
-         * Number of most-recently destroyed textures to track for use-after-free.
-         *
-         * This will cause the backend to throw an exception when a texture is freed but still bound
-         * to a SamplerGroup and used in a draw call. 0 disables completely.
-         *
-         * Currently only respected by the Metal backend.
-         */
-        size_t textureUseAfterFreePoolSize = 0;
-
         /**
          * When uploading vertex or index data, the Filament Metal backend copies data
          * into a shared staging area before transferring it to the GPU. This setting controls

diff --git a/filament/src/details/Engine.cpp b/filament/src/details/Engine.cpp
@@ -103,7 +103,6 @@ Engine* FEngine::create(Engine::Builder const& builder) {
         }
         DriverConfig const driverConfig{
                 .handleArenaSize = instance->getRequestedDriverHandleArenaSize(),
-                .textureUseAfterFreePoolSize = instance->getConfig().textureUseAfterFreePoolSize,
                 .metalUploadBufferSizeBytes = instance->getConfig().metalUploadBufferSizeBytes,
                 .disableParallelShaderCompile = instance->getConfig().disableParallelShaderCompile,
                 .disableHandleUseAfterFreeCheck = instance->getConfig().disableHandleUseAfterFreeCheck,
@@ -701,7 +700,6 @@ int FEngine::loop() {
 
     DriverConfig const driverConfig {
             .handleArenaSize = getRequestedDriverHandleArenaSize(),
-            .textureUseAfterFreePoolSize = mConfig.textureUseAfterFreePoolSize,
             .metalUploadBufferSizeBytes = mConfig.metalUploadBufferSizeBytes,
             .disableParallelShaderCompile = mConfig.disableParallelShaderCompile,
             .disableHandleUseAfterFreeCheck = mConfig.disableHandleUseAfterFreeCheck,