Create use-after-free detector for Metal textures

filament/backend/include/private/backend/DriverAPI.inc

@@ -219,7 +219,7 @@ DECL_DRIVER_API_R_N(backend::TextureHandle, importTexture,
         backend::TextureUsage, usage)
 
 DECL_DRIVER_API_R_N(backend::SamplerGroupHandle, createSamplerGroup,
-        uint32_t, size)
+        uint32_t, size, const char*, name)
 
 DECL_DRIVER_API_R_N(backend::RenderPrimitiveHandle, createRenderPrimitive,
         backend::VertexBufferHandle, vbh,

filament/backend/src/metal/MetalContext.h

@@ -24,6 +24,8 @@
 #include <Metal/Metal.h>
 #include <QuartzCore/QuartzCore.h>
 
+#include <utils/FixedCircularBuffer.h>
+
 #include <array>
 #include <atomic>
 #include <stack>
@@ -52,6 +54,11 @@ struct MetalVertexBuffer;
 
 constexpr static uint8_t MAX_SAMPLE_COUNT = 8;  // Metal devices support at most 8 MSAA samples
 
+// Keep track of this many most recently freed textures, marking them as "terminated". If the Metal
+// backend is asked to bind a texture that has been marked terminated, we throw an Obj-C error,
+// which is helpful for debugging use-after-free issues in release builds.
+constexpr static size_t kMetalFreedTextureListSize = 64;
+
 struct MetalContext {
     MetalDriver* driver;
     id<MTLDevice> device = nullptr;
@@ -111,6 +118,13 @@ struct MetalContext {
     tsl::robin_set<MetalSamplerGroup*> samplerGroups;
     tsl::robin_set<MetalTexture*> textures;
 
+    // This circle buffer implements delayed destruction for Metal texture handles. It keeps a
+    // handle to the most recent kMetalFreedTextureListSize 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 error instead of crashing.
+    utils::FixedCircularBuffer<Handle<HwTexture>, kMetalFreedTextureListSize> texturesToDestroy;
+
     MetalBufferPool* bufferPool;
 
     MetalSwapChain* currentDrawSwapChain = nil;

filament/backend/src/metal/MetalDriver.mm

@@ -303,8 +303,8 @@
         target, levels, format, samples, width, height, depth, usage, metalTexture));
 }
 
-void MetalDriver::createSamplerGroupR(Handle<HwSamplerGroup> sbh, uint32_t size) {
-    mContext->samplerGroups.insert(construct_handle<MetalSamplerGroup>(sbh, size));
+void MetalDriver::createSamplerGroupR(Handle<HwSamplerGroup> sbh, uint32_t size, const char* name) {
+    mContext->samplerGroups.insert(construct_handle<MetalSamplerGroup>(sbh, size, name));
 }
 
 void MetalDriver::createRenderPrimitiveR(Handle<HwRenderPrimitive> rph,
@@ -530,8 +530,19 @@
         return;
     }
 
-    mContext->textures.erase(handle_cast<MetalTexture>(th));
-    destruct_handle<MetalTexture>(th);
+    auto* metalTexture = handle_cast<MetalTexture>(th);
+    mContext->textures.erase(metalTexture);
+
+    // Free memory from the texture and mark it as freed.
+    metalTexture->terminate();
+
+    // Delay the destruction of this texture handle.
+    if (mContext->texturesToDestroy.full()) {
+        Handle<HwTexture> handleToFree = mContext->texturesToDestroy.pop();
+        destruct_handle<MetalTexture>(handleToFree);
+    }
+    assert_invariant(!mContext->texturesToDestroy.full());
+    mContext->texturesToDestroy.push(th);
 }
 
 void MetalDriver::destroyRenderTarget(Handle<HwRenderTarget> rth) {
@@ -557,6 +568,12 @@
 }
 
 void MetalDriver::terminate() {
+    // Terminate any oustanding MetalTextures.
+    while (mContext->texturesToDestroy.size() > 0) {
+        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();
@@ -854,22 +871,26 @@
     assert_invariant(sb->size == data.size / sizeof(SamplerDescriptor));
     auto const* const samplers = (SamplerDescriptor const*) data.buffer;
 
-#ifndef NDEBUG
-    // In debug builds, verify that all the textures in the sampler group are still alive.
+    // Verify that all the textures in the sampler group are still alive.
     // These bugs lead to memory corruption and can be difficult to track down.
     for (size_t s = 0; s < data.size / sizeof(SamplerDescriptor); s++) {
         if (!samplers[s].t) {
             continue;
         }
+        // The difference between this check and the one below is that in release, we do this for
+        // only a set number of recently freed textures, while the debug check is exhaustive.
+        auto* metalTexture = handle_cast<MetalTexture>(samplers[s].t);
+        metalTexture->checkUseAfterFree(sb->debugName.c_str_safe(), s);
+#ifndef NDEBUG
         auto iter = mContext->textures.find(handle_cast<MetalTexture>(samplers[s].t));
         if (iter == mContext->textures.end()) {
             utils::slog.e << "updateSamplerGroup: texture #"
                           << (int) s << " is dead, texture handle = "
                           << samplers[s].t << utils::io::endl;
         }
         assert_invariant(iter != mContext->textures.end());
-    }
 #endif
+    }
 
     // Create a MTLArgumentEncoder for these textures.
     // Ideally, we would create this encoder at createSamplerGroup time, but we need to know the
@@ -1357,23 +1378,27 @@
 
     id<MTLCommandBuffer> cmdBuffer = getPendingCommandBuffer(mContext);
 
-#ifndef NDEBUG
-    // In debug builds, verify that all the textures in the sampler group are still alive.
+    // Verify that all the textures in the sampler group are still alive.
     // These bugs lead to memory corruption and can be difficult to track down.
     const auto& handles = samplerGroup->getTextureHandles();
     for (size_t s = 0; s < handles.size(); s++) {
         if (!handles[s]) {
             continue;
         }
-        auto iter = mContext->textures.find(handle_cast<MetalTexture>(handles[s]));
+        // The difference between this check and the one below is that in release, we do this for
+        // only a set number of recently freed textures, while the debug check is exhaustive.
+        auto* metalTexture = handle_cast<MetalTexture>(handles[s]);
+        metalTexture->checkUseAfterFree(samplerGroup->debugName.c_str_safe(), s);
+#ifndef NDEBUG
+        auto iter = mContext->textures.find(metalTexture);
         if (iter == mContext->textures.end()) {
             utils::slog.e << "finalizeSamplerGroup: texture #"
                           << (int) s << " is dead, texture handle = "
                           << handles[s] << utils::io::endl;
         }
         assert_invariant(iter != mContext->textures.end());
-    }
 #endif
+    }
 
     utils::FixedCapacityVector<id<MTLTexture>> newTextures(samplerGroup->size, nil);
     for (size_t binding = 0; binding < samplerGroup->size; binding++) {

filament/backend/src/metal/MetalHandles.h

@@ -238,6 +238,12 @@ 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; }
+    void checkUseAfterFree(const char* samplerGroupDebugName, size_t textureIndex);
+
 private:
     void loadSlice(uint32_t level, MTLRegion region, uint32_t byteOffset, uint32_t slice,
             PixelBufferDescriptor const& data) noexcept;
@@ -256,12 +262,15 @@ class MetalTexture : public HwTexture {
 
     uint32_t minLod = UINT_MAX;
     uint32_t maxLod = 0;
+
+    bool terminated = false;
 };
 
 class MetalSamplerGroup : public HwSamplerGroup {
 public:
-    explicit MetalSamplerGroup(size_t size) noexcept
+    explicit MetalSamplerGroup(size_t size, const char* name) noexcept
         : size(size),
+          debugName(name),
           textureHandles(size, Handle<HwTexture>()),
           textures(size, nil),
           samplers(size, nil) {}
@@ -271,12 +280,10 @@ class MetalSamplerGroup : public HwSamplerGroup {
         textureHandles[index] = th;
     }
 
-#ifndef NDEBUG
     // This method is only used for debugging, to ensure all texture handles are alive.
     const auto& getTextureHandles() const {
         return textureHandles;
     }
-#endif
 
     // Encode a MTLTexture into this SamplerGroup at the given index.
     inline void setFinalizedTexture(size_t index, id<MTLTexture> t) {
@@ -322,6 +329,7 @@ class MetalSamplerGroup : public HwSamplerGroup {
     void useResources(id<MTLRenderCommandEncoder> renderPassEncoder);
 
     size_t size;
+    utils::CString debugName;
 
 public:
 

filament/backend/src/metal/MetalHandles.mm

@@ -516,6 +516,29 @@ void presentDrawable(bool presentFrame, void* user) {
     setLodRange(0, levels - 1);
 }
 
+void MetalTexture::terminate() noexcept {
+    texture = nil;
+    swizzledTextureView = nil;
+    lodTextureView = nil;
+    msaaSidecar = nil;
+    externalImage.set(nullptr);
+    terminated = true;
+}
+
+void MetalTexture::checkUseAfterFree(const char* samplerGroupDebugName, size_t textureIndex) {
+    if (UTILS_LIKELY(!isTerminated())) {
+        return;
+    }
+    NSString* reason = [NSString stringWithFormat:
+                                         @"Filament Metal texture use after free, sampler group = "
+                                         @"%s, texture index = %zu",
+                                 samplerGroupDebugName, textureIndex];
+    NSException* useAfterFreeException = [NSException exceptionWithName:@"MetalTextureUseAfterFree"
+                                                                 reason:reason
+                                                               userInfo:nil];
+    [useAfterFreeException raise];
+}
+
 MetalTexture::~MetalTexture() {
     externalImage.set(nullptr);
 }

filament/backend/src/opengl/OpenGLDriver.cpp

@@ -555,7 +555,7 @@ void OpenGLDriver::createProgramR(Handle<HwProgram> ph, Program&& program) {
     CHECK_GL_ERROR(utils::slog.e)
 }
 
-void OpenGLDriver::createSamplerGroupR(Handle<HwSamplerGroup> sbh, uint32_t size) {
+void OpenGLDriver::createSamplerGroupR(Handle<HwSamplerGroup> sbh, uint32_t size, const char* name) {
     DEBUG_MARKER()
 
     construct<GLSamplerGroup>(sbh, size);

filament/backend/src/vulkan/VulkanDriver.cpp

@@ -315,7 +315,7 @@ void VulkanDriver::finish(int dummy) {
     FVK_SYSTRACE_END();
 }
 
-void VulkanDriver::createSamplerGroupR(Handle<HwSamplerGroup> sbh, uint32_t count) {
+void VulkanDriver::createSamplerGroupR(Handle<HwSamplerGroup> sbh, uint32_t count, const char* name) {
     auto sg = mResourceAllocator.construct<VulkanSamplerGroup>(sbh, count);
     mResourceManager.acquire(sg);
 }

filament/backend/test/test_FeedbackLoops.cpp

@@ -193,7 +193,7 @@ TEST_F(BackendTest, FeedbackLoops) {
             sparams.filterMag = SamplerMagFilter::LINEAR;
             sparams.filterMin = SamplerMinFilter::LINEAR_MIPMAP_NEAREST;
             samplers.setSampler(0, { texture, sparams });
-            auto sgroup = api.createSamplerGroup(samplers.getSize());
+            auto sgroup = api.createSamplerGroup(samplers.getSize(), "Test");
             api.updateSamplerGroup(sgroup, samplers.toBufferDescriptor(api));
             auto ubuffer = api.createBufferObject(sizeof(MaterialParams),
                     BufferObjectBinding::UNIFORM, BufferUsage::STATIC);

filament/backend/test/test_LoadImage.cpp

@@ -303,7 +303,7 @@ TEST_F(BackendTest, UpdateImage2D) {
         sparams.filterMag = SamplerMagFilter::LINEAR;
         sparams.filterMin = SamplerMinFilter::LINEAR_MIPMAP_NEAREST;
         samplers.setSampler(0, { texture, sparams });
-        auto sgroup = api.createSamplerGroup(samplers.getSize());
+        auto sgroup = api.createSamplerGroup(samplers.getSize(), "Test");
         api.updateSamplerGroup(sgroup, samplers.toBufferDescriptor(api));
 
         api.bindSamplers(0, sgroup);
@@ -394,7 +394,7 @@ TEST_F(BackendTest, UpdateImageSRGB) {
     sparams.filterMag = SamplerMagFilter::LINEAR;
     sparams.filterMin = SamplerMinFilter::LINEAR_MIPMAP_NEAREST;
     samplers.setSampler(0, { texture, sparams });
-    auto sgroup = api.createSamplerGroup(samplers.getSize());
+    auto sgroup = api.createSamplerGroup(samplers.getSize(), "Test");
     api.updateSamplerGroup(sgroup, samplers.toBufferDescriptor(api));
 
     api.bindSamplers(0, sgroup);
@@ -469,7 +469,7 @@ TEST_F(BackendTest, UpdateImageMipLevel) {
     sparams.filterMag = SamplerMagFilter::LINEAR;
     sparams.filterMin = SamplerMinFilter::LINEAR_MIPMAP_NEAREST;
     samplers.setSampler(0, { texture, sparams });
-    auto sgroup = api.createSamplerGroup(samplers.getSize());
+    auto sgroup = api.createSamplerGroup(samplers.getSize(), "Test");
     api.updateSamplerGroup(sgroup, samplers.toBufferDescriptor(api));
 
     api.bindSamplers(0, sgroup);
@@ -556,7 +556,7 @@ TEST_F(BackendTest, UpdateImage3D) {
     sparams.filterMag = SamplerMagFilter::LINEAR;
     sparams.filterMin = SamplerMinFilter::LINEAR_MIPMAP_NEAREST;
     samplers.setSampler(0, { texture, sparams});
-    auto sgroup = api.createSamplerGroup(samplers.getSize());
+    auto sgroup = api.createSamplerGroup(samplers.getSize(), "Test");
     api.updateSamplerGroup(sgroup, samplers.toBufferDescriptor(api));
 
     api.bindSamplers(0, sgroup);

filament/backend/test/test_MipLevels.cpp

@@ -194,7 +194,7 @@ TEST_F(BackendTest, SetMinMaxLevel) {
         samplerParams.filterMag = SamplerMagFilter::NEAREST;
         samplerParams.filterMin = SamplerMinFilter::NEAREST_MIPMAP_NEAREST;
         samplers.setSampler(0, { texture, samplerParams });
-        backend::Handle<HwSamplerGroup> samplerGroup = api.createSamplerGroup(1);
+        backend::Handle<HwSamplerGroup> samplerGroup = api.createSamplerGroup(1, "Test");
         api.updateSamplerGroup(samplerGroup, samplers.toBufferDescriptor(api));
         api.bindSamplers(0, samplerGroup);
 
@@ -242,4 +242,4 @@ TEST_F(BackendTest, SetMinMaxLevel) {
     getDriver().purge();
 }
 
-} // namespace test
+} // namespace test

filament/backend/test/test_RenderExternalImage.cpp

@@ -113,7 +113,7 @@ TEST_F(BackendTest, RenderExternalImageWithoutSet) {
 
     SamplerGroup samplers(1);
     samplers.setSampler(0, { texture, {} });
-    backend::Handle<HwSamplerGroup> samplerGroup = getDriverApi().createSamplerGroup(1);
+    backend::Handle<HwSamplerGroup> samplerGroup = getDriverApi().createSamplerGroup(1, "Test");
     getDriverApi().updateSamplerGroup(samplerGroup, samplers.toBufferDescriptor(getDriverApi()));
     getDriverApi().bindSamplers(0, samplerGroup);
 
@@ -234,7 +234,7 @@ TEST_F(BackendTest, RenderExternalImage) {
 
     SamplerGroup samplers(1);
     samplers.setSampler(0, { texture, {} });
-    backend::Handle<HwSamplerGroup> samplerGroup = getDriverApi().createSamplerGroup(1);
+    backend::Handle<HwSamplerGroup> samplerGroup = getDriverApi().createSamplerGroup(1, "Test");
     getDriverApi().updateSamplerGroup(samplerGroup, samplers.toBufferDescriptor(getDriverApi()));
     getDriverApi().bindSamplers(0, samplerGroup);
 

filament/src/PerViewUniforms.cpp

@@ -43,7 +43,7 @@ PerViewUniforms::PerViewUniforms(FEngine& engine) noexcept
         : mSamplers(PerViewSib::SAMPLER_COUNT) {
     DriverApi& driver = engine.getDriverApi();
 
-    mSamplerGroupHandle = driver.createSamplerGroup(mSamplers.getSize());
+    mSamplerGroupHandle = driver.createSamplerGroup(mSamplers.getSize(), "Per-view samplers");
 
     mUniformBufferHandle = driver.createBufferObject(mUniforms.getSize(),
             BufferObjectBinding::UNIFORM, BufferUsage::DYNAMIC);

filament/src/details/MaterialInstance.cpp

@@ -73,7 +73,7 @@ FMaterialInstance::FMaterialInstance(FEngine& engine,
 
     if (!material->getSamplerInterfaceBlock().isEmpty()) {
         mSamplers = other->getSamplerGroup();
-        mSbHandle = driver.createSamplerGroup(mSamplers.getSize());
+        mSbHandle = driver.createSamplerGroup(mSamplers.getSize(), mMaterial->getName().c_str());
     }
 
     if (material->hasDoubleSidedCapability()) {
@@ -115,7 +115,8 @@ void FMaterialInstance::initDefaultInstance(FEngine& engine, FMaterial const* ma
 
     if (!material->getSamplerInterfaceBlock().isEmpty()) {
         mSamplers = SamplerGroup(material->getSamplerInterfaceBlock().getSize());
-        mSbHandle = driver.createSamplerGroup(mSamplers.getSize());
+        mSbHandle = driver.createSamplerGroup(
+                mSamplers.getSize(), "Material samplers (default material)");
     }
 
     const RasterState& rasterState = material->getRasterState();

filament/src/details/MorphTargetBuffer.cpp

@@ -125,7 +125,8 @@ FMorphTargetBuffer::FMorphTargetBuffer(FEngine& engine, const Builder& builder)
             TextureUsage::DEFAULT);
 
     // create and update sampler group
-    mSbHandle = driver.createSamplerGroup(PerRenderPrimitiveMorphingSib::SAMPLER_COUNT);
+    mSbHandle = driver.createSamplerGroup(
+            PerRenderPrimitiveMorphingSib::SAMPLER_COUNT, "Morph target samplers");
     SamplerGroup samplerGroup(PerRenderPrimitiveMorphingSib::SAMPLER_COUNT);
     samplerGroup.setSampler(PerRenderPrimitiveMorphingSib::POSITIONS, { mPbHandle, {}});
     samplerGroup.setSampler(PerRenderPrimitiveMorphingSib::TANGENTS, { mTbHandle, {}});

filament/src/details/SkinningBuffer.cpp

@@ -240,7 +240,8 @@ FSkinningBuffer::HandleIndicesAndWeights FSkinningBuffer::createIndicesAndWeight
             getSkinningBufferWidth(count),
             getSkinningBufferHeight(count), 1,
             TextureUsage::DEFAULT);
-    samplerHandle = driver.createSamplerGroup(PerRenderPrimitiveSkinningSib::SAMPLER_COUNT);
+    samplerHandle = driver.createSamplerGroup(
+            PerRenderPrimitiveSkinningSib::SAMPLER_COUNT, "Skinning buffer samplers");
     SamplerGroup samplerGroup(PerRenderPrimitiveSkinningSib::SAMPLER_COUNT);
     samplerGroup.setSampler(PerRenderPrimitiveSkinningSib::BONE_INDICES_AND_WEIGHTS,
             { textureHandle, {}});

libs/utils/CMakeLists.txt

@@ -148,6 +148,7 @@ set(TEST_SRCS
         test/test_CyclicBarrier.cpp
         test/test_Entity.cpp
         test/test_FixedCapacityVector.cpp
+        test/test_FixedCircularBuffer.cpp
         test/test_Hash.cpp
         test/test_JobSystem.cpp
         test/test_QuadTreeArray.cpp