From f8a0f82fad3893c80fdc3219cb09900be02a1655 Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Thu, 10 Jun 2021 02:40:18 +0000 Subject: [PATCH] Move zero-size copy skips from the frontend to the backend Bug: chromium:1217741 Change-Id: Ib4884b5aa80124ed9da48262fcae11cf6d605034 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/53883 Commit-Queue: Austin Eng Reviewed-by: Corentin Wallez Reviewed-by: Jiawei Shao --- src/dawn_native/CommandEncoder.cpp | 101 ++++++++---------- src/dawn_native/d3d12/CommandBufferD3D12.cpp | 20 +++- src/dawn_native/metal/CommandBufferMTL.mm | 19 ++++ src/dawn_native/opengl/CommandBufferGL.cpp | 19 ++++ src/dawn_native/vulkan/CommandBufferVk.cpp | 19 ++++ .../CopyCommandsValidationTests.cpp | 16 +++ 6 files changed, 134 insertions(+), 60 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 8f9db690a0..7676a49c4c 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -633,16 +633,13 @@ namespace dawn_native { mTopLevelBuffers.insert(destination); } - // Skip noop copies. Some backends validation rules disallow them. - if (size != 0) { - CopyBufferToBufferCmd* copy = - allocator->Allocate(Command::CopyBufferToBuffer); - copy->source = source; - copy->sourceOffset = sourceOffset; - copy->destination = destination; - copy->destinationOffset = destinationOffset; - copy->size = size; - } + CopyBufferToBufferCmd* copy = + allocator->Allocate(Command::CopyBufferToBuffer); + copy->source = source; + copy->sourceOffset = sourceOffset; + copy->destination = destination; + copy->destinationOffset = destinationOffset; + copy->size = size; return {}; }); @@ -682,23 +679,18 @@ namespace dawn_native { ApplyDefaultTextureDataLayoutOptions(&srcLayout, blockInfo, *copySize); - // Skip noop copies. - if (copySize->width != 0 && copySize->height != 0 && - copySize->depthOrArrayLayers != 0) { - // Record the copy command. - CopyBufferToTextureCmd* copy = - allocator->Allocate(Command::CopyBufferToTexture); - copy->source.buffer = source->buffer; - copy->source.offset = srcLayout.offset; - copy->source.bytesPerRow = srcLayout.bytesPerRow; - copy->source.rowsPerImage = srcLayout.rowsPerImage; - copy->destination.texture = destination->texture; - copy->destination.origin = destination->origin; - copy->destination.mipLevel = destination->mipLevel; - copy->destination.aspect = - ConvertAspect(destination->texture->GetFormat(), destination->aspect); - copy->copySize = *copySize; - } + CopyBufferToTextureCmd* copy = + allocator->Allocate(Command::CopyBufferToTexture); + copy->source.buffer = source->buffer; + copy->source.offset = srcLayout.offset; + copy->source.bytesPerRow = srcLayout.bytesPerRow; + copy->source.rowsPerImage = srcLayout.rowsPerImage; + copy->destination.texture = destination->texture; + copy->destination.origin = destination->origin; + copy->destination.mipLevel = destination->mipLevel; + copy->destination.aspect = + ConvertAspect(destination->texture->GetFormat(), destination->aspect); + copy->copySize = *copySize; return {}; }); @@ -738,22 +730,17 @@ namespace dawn_native { ApplyDefaultTextureDataLayoutOptions(&dstLayout, blockInfo, *copySize); - // Skip noop copies. - if (copySize->width != 0 && copySize->height != 0 && - copySize->depthOrArrayLayers != 0) { - // Record the copy command. - CopyTextureToBufferCmd* copy = - allocator->Allocate(Command::CopyTextureToBuffer); - copy->source.texture = source->texture; - copy->source.origin = source->origin; - copy->source.mipLevel = source->mipLevel; - copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect); - copy->destination.buffer = destination->buffer; - copy->destination.offset = dstLayout.offset; - copy->destination.bytesPerRow = dstLayout.bytesPerRow; - copy->destination.rowsPerImage = dstLayout.rowsPerImage; - copy->copySize = *copySize; - } + CopyTextureToBufferCmd* copy = + allocator->Allocate(Command::CopyTextureToBuffer); + copy->source.texture = source->texture; + copy->source.origin = source->origin; + copy->source.mipLevel = source->mipLevel; + copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect); + copy->destination.buffer = destination->buffer; + copy->destination.offset = dstLayout.offset; + copy->destination.bytesPerRow = dstLayout.bytesPerRow; + copy->destination.rowsPerImage = dstLayout.rowsPerImage; + copy->copySize = *copySize; return {}; }); @@ -783,22 +770,18 @@ namespace dawn_native { mTopLevelTextures.insert(destination->texture); } - // Skip noop copies. - if (copySize->width != 0 && copySize->height != 0 && - copySize->depthOrArrayLayers != 0) { - CopyTextureToTextureCmd* copy = - allocator->Allocate(Command::CopyTextureToTexture); - copy->source.texture = source->texture; - copy->source.origin = source->origin; - copy->source.mipLevel = source->mipLevel; - copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect); - copy->destination.texture = destination->texture; - copy->destination.origin = destination->origin; - copy->destination.mipLevel = destination->mipLevel; - copy->destination.aspect = - ConvertAspect(destination->texture->GetFormat(), destination->aspect); - copy->copySize = *copySize; - } + CopyTextureToTextureCmd* copy = + allocator->Allocate(Command::CopyTextureToTexture); + copy->source.texture = source->texture; + copy->source.origin = source->origin; + copy->source.mipLevel = source->mipLevel; + copy->source.aspect = ConvertAspect(source->texture->GetFormat(), source->aspect); + copy->destination.texture = destination->texture; + copy->destination.origin = destination->origin; + copy->destination.mipLevel = destination->mipLevel; + copy->destination.aspect = + ConvertAspect(destination->texture->GetFormat(), destination->aspect); + copy->copySize = *copySize; return {}; }); diff --git a/src/dawn_native/d3d12/CommandBufferD3D12.cpp b/src/dawn_native/d3d12/CommandBufferD3D12.cpp index a2dca9ca00..29b3a98775 100644 --- a/src/dawn_native/d3d12/CommandBufferD3D12.cpp +++ b/src/dawn_native/d3d12/CommandBufferD3D12.cpp @@ -626,6 +626,10 @@ namespace dawn_native { namespace d3d12 { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = mCommands.NextCommand(); + if (copy->size == 0) { + // Skip no-op copies. + break; + } Buffer* srcBuffer = ToBackend(copy->source.Get()); Buffer* dstBuffer = ToBackend(copy->destination.Get()); @@ -646,6 +650,11 @@ namespace dawn_native { namespace d3d12 { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } Buffer* buffer = ToBackend(copy->source.buffer.Get()); Texture* texture = ToBackend(copy->destination.texture.Get()); @@ -676,6 +685,11 @@ namespace dawn_native { namespace d3d12 { case Command::CopyTextureToBuffer: { CopyTextureToBufferCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } Texture* texture = ToBackend(copy->source.texture.Get()); Buffer* buffer = ToBackend(copy->destination.buffer.Get()); @@ -700,7 +714,11 @@ namespace dawn_native { namespace d3d12 { case Command::CopyTextureToTexture: { CopyTextureToTextureCmd* copy = mCommands.NextCommand(); - + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } Texture* source = ToBackend(copy->source.texture.Get()); Texture* destination = ToBackend(copy->destination.texture.Get()); diff --git a/src/dawn_native/metal/CommandBufferMTL.mm b/src/dawn_native/metal/CommandBufferMTL.mm index a9f1896a94..b390184137 100644 --- a/src/dawn_native/metal/CommandBufferMTL.mm +++ b/src/dawn_native/metal/CommandBufferMTL.mm @@ -704,6 +704,10 @@ namespace dawn_native { namespace metal { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = mCommands.NextCommand(); + if (copy->size == 0) { + // Skip no-op copies. + break; + } ToBackend(copy->source)->EnsureDataInitialized(commandContext); ToBackend(copy->destination) @@ -721,6 +725,11 @@ namespace dawn_native { namespace metal { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } auto& src = copy->source; auto& dst = copy->destination; auto& copySize = copy->copySize; @@ -739,6 +748,11 @@ namespace dawn_native { namespace metal { case Command::CopyTextureToBuffer: { CopyTextureToBufferCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } auto& src = copy->source; auto& dst = copy->destination; auto& copySize = copy->copySize; @@ -814,6 +828,11 @@ namespace dawn_native { namespace metal { case Command::CopyTextureToTexture: { CopyTextureToTextureCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } Texture* srcTexture = ToBackend(copy->source.texture.Get()); Texture* dstTexture = ToBackend(copy->destination.texture.Get()); diff --git a/src/dawn_native/opengl/CommandBufferGL.cpp b/src/dawn_native/opengl/CommandBufferGL.cpp index 40eb26b249..085a8f015b 100644 --- a/src/dawn_native/opengl/CommandBufferGL.cpp +++ b/src/dawn_native/opengl/CommandBufferGL.cpp @@ -612,6 +612,10 @@ namespace dawn_native { namespace opengl { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = mCommands.NextCommand(); + if (copy->size == 0) { + // Skip no-op copies. + break; + } ToBackend(copy->source)->EnsureDataInitialized(); ToBackend(copy->destination) @@ -630,6 +634,11 @@ namespace dawn_native { namespace opengl { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } auto& src = copy->source; auto& dst = copy->destination; Buffer* buffer = ToBackend(src.buffer.Get()); @@ -664,6 +673,11 @@ namespace dawn_native { namespace opengl { case Command::CopyTextureToBuffer: { CopyTextureToBufferCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } auto& src = copy->source; auto& dst = copy->destination; auto& copySize = copy->copySize; @@ -771,6 +785,11 @@ namespace dawn_native { namespace opengl { case Command::CopyTextureToTexture: { CopyTextureToTextureCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } auto& src = copy->source; auto& dst = copy->destination; diff --git a/src/dawn_native/vulkan/CommandBufferVk.cpp b/src/dawn_native/vulkan/CommandBufferVk.cpp index 2383e90972..64090795ca 100644 --- a/src/dawn_native/vulkan/CommandBufferVk.cpp +++ b/src/dawn_native/vulkan/CommandBufferVk.cpp @@ -516,6 +516,10 @@ namespace dawn_native { namespace vulkan { switch (type) { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = mCommands.NextCommand(); + if (copy->size == 0) { + // Skip no-op copies. + break; + } Buffer* srcBuffer = ToBackend(copy->source.Get()); Buffer* dstBuffer = ToBackend(copy->destination.Get()); @@ -540,6 +544,11 @@ namespace dawn_native { namespace vulkan { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } auto& src = copy->source; auto& dst = copy->destination; @@ -578,6 +587,11 @@ namespace dawn_native { namespace vulkan { case Command::CopyTextureToBuffer: { CopyTextureToBufferCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } auto& src = copy->source; auto& dst = copy->destination; @@ -610,6 +624,11 @@ namespace dawn_native { namespace vulkan { case Command::CopyTextureToTexture: { CopyTextureToTextureCmd* copy = mCommands.NextCommand(); + if (copy->copySize.width == 0 || copy->copySize.height == 0 || + copy->copySize.depthOrArrayLayers == 0) { + // Skip no-op copies. + continue; + } TextureCopy& src = copy->source; TextureCopy& dst = copy->destination; SubresourceRange srcRange = GetSubresourcesAffectedByCopy(src, copy->copySize); diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp index dbbc3327c3..c75c50ddd6 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp @@ -226,6 +226,22 @@ TEST_F(CopyCommandTest_B2B, Success) { } } +// Test a successful B2B copy where the last external reference is dropped. +// This is a regression test for crbug.com/1217741 where submitting a command +// buffer with dropped resources when the copy size is 0 was a use-after-free. +TEST_F(CopyCommandTest_B2B, DroppedBuffer) { + wgpu::Buffer source = CreateBuffer(16, wgpu::BufferUsage::CopySrc); + wgpu::Buffer destination = CreateBuffer(16, wgpu::BufferUsage::CopyDst); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(source, 0, destination, 0, 0); + wgpu::CommandBuffer commandBuffer = encoder.Finish(); + + source = nullptr; + destination = nullptr; + device.GetQueue().Submit(1, &commandBuffer); +} + // Test B2B copies with OOB TEST_F(CopyCommandTest_B2B, OutOfBounds) { wgpu::Buffer source = CreateBuffer(16, wgpu::BufferUsage::CopySrc);