Fix dangling pointer for T2B if copy size is zero
T2B: remove the early return when copy size is zero.
Instead encode a CopyTextureToBufferCmd, which is
handled on the backends to no-op a zero sized copy.
Add regression unittests.
Bug: 405316877
Change-Id: Icdf8f49b18267fdbdc6f7957874653fcc5b390d0
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/232694
Auto-Submit: Shrek Shao <shrekshao@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Shrek Shao <shrekshao@google.com>
Reviewed-by: Quyen Le <lehoangquyen@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn/native/CommandEncoder.cpp b/src/dawn/native/CommandEncoder.cpp
index f89de32..a776905 100644
--- a/src/dawn/native/CommandEncoder.cpp
+++ b/src/dawn/native/CommandEncoder.cpp
@@ -940,7 +940,13 @@
bool ShouldUseTextureToBufferBlit(const DeviceBase* device,
const Format& format,
- const Aspect& aspect) {
+ const Aspect& aspect,
+ const Extent3D& copySize) {
+ // Noop copy, do not use blit.
+ if (copySize.width == 0 || copySize.height == 0 || copySize.depthOrArrayLayers == 0) {
+ return false;
+ }
+
// Snorm
if (format.IsSnorm() && device->IsToggleEnabled(Toggle::UseBlitForSnormTextureToBufferCopy)) {
return true;
@@ -1694,17 +1700,11 @@
TexelCopyBufferLayout dstLayout = destination->layout;
ApplyDefaultTexelCopyBufferLayoutOptions(&dstLayout, blockInfo, *copySize);
- if (copySize->width == 0 || copySize->height == 0 ||
- copySize->depthOrArrayLayers == 0) {
- // Noop copy but is valid, simply skip encoding any command.
- return {};
- }
-
auto format = source.texture->GetFormat();
auto aspect = ConvertAspect(format, source.aspect);
// Workaround to use compute pass to emulate texture to buffer copy
- if (ShouldUseTextureToBufferBlit(GetDevice(), format, aspect)) {
+ if (ShouldUseTextureToBufferBlit(GetDevice(), format, aspect, *copySize)) {
// This function might create new resources. Need to lock the Device.
// TODO(crbug.com/dawn/1618): In future, all temp resources should be created at
// Command Submit time, so the locking would be removed from here at that point.
diff --git a/src/dawn/tests/unittests/native/CommandBufferEncodingTests.cpp b/src/dawn/tests/unittests/native/CommandBufferEncodingTests.cpp
index 1ad37e0..1e68eaf 100644
--- a/src/dawn/tests/unittests/native/CommandBufferEncodingTests.cpp
+++ b/src/dawn/tests/unittests/native/CommandBufferEncodingTests.cpp
@@ -32,6 +32,7 @@
#include "dawn/native/Commands.h"
#include "dawn/native/ComputePassEncoder.h"
#include "dawn/tests/DawnNativeTest.h"
+#include "dawn/utils/TestUtils.h"
#include "dawn/utils/WGPUHelpers.h"
namespace dawn::native {
@@ -321,5 +322,91 @@
EXPECT_FALSE(stateTracker->HasPipeline());
}
+// Regression test for crbug.com/405316877.
+// Make sure a zero size no-op copy would not cause dereferencing to a dangling pointer,
+// if the buffer is destroyed immediately after the encoding.
+TEST_F(CommandBufferEncodingTests, DanglingDestroyedBufferPointerB2T) {
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ {
+ wgpu::Buffer buffer;
+ {
+ wgpu::BufferDescriptor descriptor = {};
+ descriptor.size = 1024;
+ descriptor.usage = wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst;
+ buffer = device.CreateBuffer(&descriptor);
+ }
+
+ constexpr wgpu::Extent3D kTextureSize = {4, 4, 1};
+ wgpu::Texture texture;
+ {
+ wgpu::TextureDescriptor descriptor;
+ descriptor.size = kTextureSize;
+ descriptor.format = wgpu::TextureFormat::RGBA8Unorm;
+ descriptor.usage = wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::CopySrc;
+ texture = device.CreateTexture(&descriptor);
+ }
+
+ wgpu::TexelCopyBufferInfo texelCopyBufferInfo = utils::CreateTexelCopyBufferInfo(
+ buffer, 0, kTextureBytesPerRowAlignment, kTextureSize.height);
+ wgpu::TexelCopyTextureInfo texelCopyTextureInfo =
+ utils::CreateTexelCopyTextureInfo(texture, 0, {0, 0, 0});
+ constexpr wgpu::Extent3D kCopySize = {0, 0, 0};
+ encoder.CopyBufferToTexture(&texelCopyBufferInfo, &texelCopyTextureInfo, &kCopySize);
+
+ // Destroy and release the buffer immediately.
+ buffer.Destroy();
+ }
+
+ wgpu::CommandBuffer commands = encoder.Finish();
+
+ device.PushErrorScope(wgpu::ErrorFilter::Validation);
+ device.GetQueue().Submit(1, &commands);
+ device.PopErrorScope(wgpu::CallbackMode::AllowProcessEvents,
+ [](wgpu::PopErrorScopeStatus, wgpu::ErrorType, wgpu::StringView) {});
+}
+
+// Regression test for crbug.com/405316877.
+// Make sure a zero size no-op copy would not cause dereferencing to a dangling pointer,
+// if the buffer is destroyed immediately after the encoding.
+TEST_F(CommandBufferEncodingTests, DanglingDestroyedBufferPointerT2B) {
+ wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+ {
+ wgpu::Buffer buffer;
+ {
+ wgpu::BufferDescriptor descriptor = {};
+ descriptor.size = 1024;
+ descriptor.usage = wgpu::BufferUsage::CopySrc | wgpu::BufferUsage::CopyDst;
+ buffer = device.CreateBuffer(&descriptor);
+ }
+
+ constexpr wgpu::Extent3D kTextureSize = {4, 4, 1};
+ wgpu::Texture texture;
+ {
+ wgpu::TextureDescriptor descriptor;
+ descriptor.size = kTextureSize;
+ descriptor.format = wgpu::TextureFormat::RGBA8Unorm;
+ descriptor.usage = wgpu::TextureUsage::CopyDst | wgpu::TextureUsage::CopySrc;
+ texture = device.CreateTexture(&descriptor);
+ }
+
+ wgpu::TexelCopyBufferInfo texelCopyBufferInfo = utils::CreateTexelCopyBufferInfo(
+ buffer, 0, kTextureBytesPerRowAlignment, kTextureSize.height);
+ wgpu::TexelCopyTextureInfo texelCopyTextureInfo =
+ utils::CreateTexelCopyTextureInfo(texture, 0, {0, 0, 0});
+ constexpr wgpu::Extent3D kCopySize = {0, 0, 0};
+ encoder.CopyTextureToBuffer(&texelCopyTextureInfo, &texelCopyBufferInfo, &kCopySize);
+
+ // Destroy and release the buffer immediately.
+ buffer.Destroy();
+ }
+
+ wgpu::CommandBuffer commands = encoder.Finish();
+
+ device.PushErrorScope(wgpu::ErrorFilter::Validation);
+ device.GetQueue().Submit(1, &commands);
+ device.PopErrorScope(wgpu::CallbackMode::AllowProcessEvents,
+ [](wgpu::PopErrorScopeStatus, wgpu::ErrorType, wgpu::StringView) {});
+}
+
} // anonymous namespace
} // namespace dawn::native
