From 26b7d8f6d719f13b802fdc85797cb1ea75f4ef98 Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Thu, 9 Apr 2020 21:22:12 +0000 Subject: [PATCH] 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 Reviewed-by: Kai Ninomiya --- src/dawn_native/BindGroup.cpp | 8 ++++++++ src/dawn_native/BindGroup.h | 1 + src/dawn_native/RefCounted.cpp | 6 +++++- src/dawn_native/RefCounted.h | 3 +++ src/tests/end2end/BindGroupTests.cpp | 19 +++++++++++++++++++ 5 files changed, 36 insertions(+), 1 deletion(-) diff --git a/src/dawn_native/BindGroup.cpp b/src/dawn_native/BindGroup.cpp index 421cc2a5ff..832a657c89 100644 --- a/src/dawn_native/BindGroup.cpp +++ b/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 layout = mLayout; + RefCounted::DeleteThis(); + } + BindGroupBase::BindGroupBase(DeviceBase* device, ObjectBase::ErrorTag tag) : ObjectBase(device, tag), mBindingData() { } diff --git a/src/dawn_native/BindGroup.h b/src/dawn_native/BindGroup.h index 8a64bb6f4a..6afee61083 100644 --- a/src/dawn_native/BindGroup.h +++ b/src/dawn_native/BindGroup.h @@ -73,6 +73,7 @@ namespace dawn_native { private: BindGroupBase(DeviceBase* device, ObjectBase::ErrorTag tag); + void DeleteThis() override; Ref mLayout; BindGroupLayoutBase::BindingDataPointers mBindingData; diff --git a/src/dawn_native/RefCounted.cpp b/src/dawn_native/RefCounted.cpp index 3d8085c445..ceb9c75fc6 100644 --- a/src/dawn_native/RefCounted.cpp +++ b/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 diff --git a/src/dawn_native/RefCounted.h b/src/dawn_native/RefCounted.h index 7e5a1d84f8..3b3a55e3bb 100644 --- a/src/dawn_native/RefCounted.h +++ b/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; }; diff --git a/src/tests/end2end/BindGroupTests.cpp b/src/tests/end2end/BindGroupTests.cpp index 762d4318a9..60d7afc183 100644 --- a/src/tests/end2end/BindGroupTests.cpp +++ b/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 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());