Fix use-after-free if BindGroup is the last owner of its BindGroupLayout

Destruction of the BindGroup needs to ensure that the BindGroupLayout is
destroyed after the BindGroup. This is done by using a custom deleter which
first creates an extra reference to the BGL before deleting the BindGroup.

Bug: dawn:355
Change-Id: I819bbce13473ee4738eaa304f6dac90e0501302a
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/19060
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

src/dawn_native/BindGroup.cpp

@@ -257,6 +257,14 @@ namespace dawn_native {
         }
     }
 
+    void BindGroupBase::DeleteThis() {
+        // Add another ref to the layout so that if this is the last ref, the layout
+        // is destroyed after the bind group. The bind group is slab-allocated inside
+        // memory owned by the layout (except for the null backend).
+        Ref<BindGroupLayoutBase> layout = mLayout;
+        RefCounted::DeleteThis();
+    }
+
     BindGroupBase::BindGroupBase(DeviceBase* device, ObjectBase::ErrorTag tag)
         : ObjectBase(device, tag), mBindingData() {
     }

src/dawn_native/BindGroup.h

@@ -73,6 +73,7 @@ namespace dawn_native {
 
       private:
         BindGroupBase(DeviceBase* device, ObjectBase::ErrorTag tag);
+        void DeleteThis() override;
 
         Ref<BindGroupLayoutBase> mLayout;
         BindGroupLayoutBase::BindingDataPointers mBindingData;

src/dawn_native/RefCounted.cpp

@@ -72,8 +72,12 @@ namespace dawn_native {
             // memory barrier, when an acquire load on mRefCount (using the `ldar` instruction)
             // should be enough and could end up being faster.
             std::atomic_thread_fence(std::memory_order_acquire);
-            delete this;
+            DeleteThis();
         }
     }
 
+    void RefCounted::DeleteThis() {
+        delete this;
+    }
+
 }  // namespace dawn_native

src/dawn_native/RefCounted.h

@@ -33,7 +33,10 @@ namespace dawn_native {
 
       protected:
         virtual ~RefCounted() = default;
+        // A Derived class may override this if they require a custom deleter.
+        virtual void DeleteThis();
 
+      private:
         std::atomic_uint64_t mRefCount;
     };
 

src/tests/end2end/BindGroupTests.cpp

@@ -876,4 +876,23 @@ TEST_P(BindGroupTests, ArbitraryBindingNumbers) {
     DoTest(red, black, blue, RGBA8(64, 0, 255, 0));
 }
 
+// This is a regression test for crbug.com/dawn/355 which tests that destruction of a bind group
+// that holds the last reference to its bind group layout does not result in a use-after-free. In
+// the bug, the destructor of BindGroupBase, when destroying member mLayout,
+// Ref<BindGroupLayoutBase> assigns to Ref::mPointee, AFTER calling Release(). After the BGL is
+// destroyed, the storage for |mPointee| has been freed.
+TEST_P(BindGroupTests, LastReferenceToBindGroupLayout) {
+    wgpu::BufferDescriptor bufferDesc;
+    bufferDesc.size = sizeof(float);
+    bufferDesc.usage = wgpu::BufferUsage::Uniform;
+    wgpu::Buffer buffer = device.CreateBuffer(&bufferDesc);
+
+    wgpu::BindGroup bg;
+    {
+        wgpu::BindGroupLayout bgl = utils::MakeBindGroupLayout(
+            device, {{0, wgpu::ShaderStage::Vertex, wgpu::BindingType::UniformBuffer}});
+        bg = utils::MakeBindGroup(device, bgl, {{0, buffer, 0, sizeof(float)}});
+    }
+}
+
 DAWN_INSTANTIATE_TEST(BindGroupTests, D3D12Backend(), MetalBackend(), OpenGLBackend(), VulkanBackend());