Buffer DestroyImpl check for buffer resource to avoid double free
Double free was happening if buffer.Destroy() was called right
before the buffer went out of scope since DestroyImpl wouldn't
check to see if the resource was already released.
Also refactor Destroy in Texture classes to match the pattern
of Buffer classes. Added validation of the texture object when
Destroy is called.
Bug: dawn:124, chromium:947323
Change-Id: I0e4a652ff5b86a151b4919c781c1dd385b4e3213
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/6060
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Natasha Lee <natlee@microsoft.com>
diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp
index 9cfcf03..74ac084 100644
--- a/src/dawn_native/Buffer.cpp
+++ b/src/dawn_native/Buffer.cpp
@@ -235,8 +235,7 @@
if (mState == BufferState::Mapped) {
Unmap();
}
- DestroyImpl();
- mState = BufferState::Destroyed;
+ DestroyInternal();
}
void BufferBase::Unmap() {
@@ -331,4 +330,11 @@
return {};
}
+ void BufferBase::DestroyInternal() {
+ if (mState != BufferState::Destroyed) {
+ DestroyImpl();
+ }
+ mState = BufferState::Destroyed;
+ }
+
} // namespace dawn_native
diff --git a/src/dawn_native/Buffer.h b/src/dawn_native/Buffer.h
index 7ad78a9..dbe6fe5 100644
--- a/src/dawn_native/Buffer.h
+++ b/src/dawn_native/Buffer.h
@@ -70,6 +70,8 @@
void* pointer,
uint32_t dataLength);
+ void DestroyInternal();
+
private:
virtual MaybeError SetSubDataImpl(uint32_t start, uint32_t count, const uint8_t* data);
virtual void MapReadAsyncImpl(uint32_t serial) = 0;
diff --git a/src/dawn_native/Texture.cpp b/src/dawn_native/Texture.cpp
index 573269c..9d584e8 100644
--- a/src/dawn_native/Texture.cpp
+++ b/src/dawn_native/Texture.cpp
@@ -376,14 +376,6 @@
return {};
}
- MaybeError TextureBase::ValidateCanCreateTextureViewNow() const {
- ASSERT(!IsError());
- if (mState == TextureState::Destroyed) {
- return DAWN_VALIDATION_ERROR("Destroyed texture used to create texture view");
- }
- return {};
- }
-
bool TextureBase::IsMultisampledTexture() const {
ASSERT(!IsError());
return mSampleCount > 1;
@@ -404,13 +396,26 @@
}
void TextureBase::Destroy() {
+ if (GetDevice()->ConsumedError(ValidateDestroy())) {
+ return;
+ }
+ ASSERT(!IsError());
+ DestroyInternal();
+ }
+
+ void TextureBase::DestroyImpl() {
+ }
+
+ void TextureBase::DestroyInternal() {
if (mState == TextureState::OwnedInternal) {
DestroyImpl();
}
mState = TextureState::Destroyed;
}
- void TextureBase::DestroyImpl() {
+ MaybeError TextureBase::ValidateDestroy() const {
+ DAWN_TRY(GetDevice()->ValidateObject(this));
+ return {};
}
// TextureViewBase
diff --git a/src/dawn_native/Texture.h b/src/dawn_native/Texture.h
index af91a63..15c3dcd 100644
--- a/src/dawn_native/Texture.h
+++ b/src/dawn_native/Texture.h
@@ -61,7 +61,6 @@
TextureState GetTextureState() const;
MaybeError ValidateCanUseInSubmitNow() const;
- MaybeError ValidateCanCreateTextureViewNow() const;
bool IsMultisampledTexture() const;
@@ -70,10 +69,14 @@
TextureViewBase* CreateTextureView(const TextureViewDescriptor* descriptor);
void Destroy();
+ protected:
+ void DestroyInternal();
+
private:
TextureBase(DeviceBase* device, ObjectBase::ErrorTag tag);
virtual void DestroyImpl();
+ MaybeError ValidateDestroy() const;
dawn::TextureDimension mDimension;
dawn::TextureFormat mFormat;
Extent3D mSize;
diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp
index 307118d..b0b5a1e 100644
--- a/src/dawn_native/d3d12/BufferD3D12.cpp
+++ b/src/dawn_native/d3d12/BufferD3D12.cpp
@@ -105,7 +105,7 @@
}
Buffer::~Buffer() {
- DestroyImpl();
+ DestroyInternal();
}
uint32_t Buffer::GetD3D12Size() const {
diff --git a/src/dawn_native/d3d12/TextureD3D12.cpp b/src/dawn_native/d3d12/TextureD3D12.cpp
index ecce6f2..63a0533 100644
--- a/src/dawn_native/d3d12/TextureD3D12.cpp
+++ b/src/dawn_native/d3d12/TextureD3D12.cpp
@@ -141,7 +141,7 @@
}
Texture::~Texture() {
- Destroy();
+ DestroyInternal();
}
void Texture::DestroyImpl() {
diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm
index 3e91b43..1d0828a 100644
--- a/src/dawn_native/metal/BufferMTL.mm
+++ b/src/dawn_native/metal/BufferMTL.mm
@@ -31,7 +31,7 @@
}
Buffer::~Buffer() {
- DestroyImpl();
+ DestroyInternal();
}
id<MTLBuffer> Buffer::GetMTLBuffer() {
diff --git a/src/dawn_native/metal/TextureMTL.mm b/src/dawn_native/metal/TextureMTL.mm
index b364d4a..3659240 100644
--- a/src/dawn_native/metal/TextureMTL.mm
+++ b/src/dawn_native/metal/TextureMTL.mm
@@ -219,7 +219,7 @@
}
Texture::~Texture() {
- Destroy();
+ DestroyInternal();
}
void Texture::DestroyImpl() {
diff --git a/src/dawn_native/opengl/BufferGL.cpp b/src/dawn_native/opengl/BufferGL.cpp
index 50e2d0b..225a583 100644
--- a/src/dawn_native/opengl/BufferGL.cpp
+++ b/src/dawn_native/opengl/BufferGL.cpp
@@ -28,7 +28,7 @@
}
Buffer::~Buffer() {
- DestroyImpl();
+ DestroyInternal();
}
GLuint Buffer::GetHandle() const {
diff --git a/src/dawn_native/opengl/TextureGL.cpp b/src/dawn_native/opengl/TextureGL.cpp
index bfef538..34fa9f3 100644
--- a/src/dawn_native/opengl/TextureGL.cpp
+++ b/src/dawn_native/opengl/TextureGL.cpp
@@ -177,7 +177,7 @@
}
Texture::~Texture() {
- Destroy();
+ DestroyInternal();
}
void Texture::DestroyImpl() {
diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp
index e38a716..137f1ad 100644
--- a/src/dawn_native/vulkan/BufferVk.cpp
+++ b/src/dawn_native/vulkan/BufferVk.cpp
@@ -137,7 +137,7 @@
}
Buffer::~Buffer() {
- DestroyImpl();
+ DestroyInternal();
}
void Buffer::OnMapReadCommandSerialFinished(uint32_t mapSerial, const void* data) {
diff --git a/src/dawn_native/vulkan/TextureVk.cpp b/src/dawn_native/vulkan/TextureVk.cpp
index 872393d..186ec14 100644
--- a/src/dawn_native/vulkan/TextureVk.cpp
+++ b/src/dawn_native/vulkan/TextureVk.cpp
@@ -297,7 +297,7 @@
}
Texture::~Texture() {
- Destroy();
+ DestroyInternal();
}
void Texture::DestroyImpl() {
