Add a test for creating a 0-sized buffer.
This is valid in WebGPU but causes validation errors in backends.
Also make it an OOM error on Metal to request a buffer close to
UINT32_MAX size because it would truncate the size, and could lead to
OOBs.
Bug: chromium:1069076
Change-Id: Ib961cb236cb7cabc0ae21203bf1d72ba82a56272
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/21060
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn_native/d3d12/BufferD3D12.cpp b/src/dawn_native/d3d12/BufferD3D12.cpp
index 63a1f82..fa68ef7 100644
--- a/src/dawn_native/d3d12/BufferD3D12.cpp
+++ b/src/dawn_native/d3d12/BufferD3D12.cpp
@@ -84,7 +84,9 @@
D3D12_RESOURCE_DESC resourceDescriptor;
resourceDescriptor.Dimension = D3D12_RESOURCE_DIMENSION_BUFFER;
resourceDescriptor.Alignment = 0;
- resourceDescriptor.Width = GetSize();
+ // TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead
+ // of creating a new 4-byte buffer?
+ resourceDescriptor.Width = std::max(GetSize(), uint64_t(4u));
resourceDescriptor.Height = 1;
resourceDescriptor.DepthOrArraySize = 1;
resourceDescriptor.MipLevels = 1;
diff --git a/src/dawn_native/metal/BufferMTL.h b/src/dawn_native/metal/BufferMTL.h
index effd077..081503b 100644
--- a/src/dawn_native/metal/BufferMTL.h
+++ b/src/dawn_native/metal/BufferMTL.h
@@ -26,13 +26,14 @@
class Buffer : public BufferBase {
public:
- Buffer(Device* device, const BufferDescriptor* descriptor);
-
+ static ResultOrError<Buffer*> Create(Device* device, const BufferDescriptor* descriptor);
id<MTLBuffer> GetMTLBuffer() const;
void OnMapCommandSerialFinished(uint32_t mapSerial, bool isWrite);
private:
+ using BufferBase::BufferBase;
+ MaybeError Initialize();
~Buffer() override;
// Dawn API
MaybeError MapReadAsyncImpl(uint32_t serial) override;
diff --git a/src/dawn_native/metal/BufferMTL.mm b/src/dawn_native/metal/BufferMTL.mm
index c61b029..5b57778 100644
--- a/src/dawn_native/metal/BufferMTL.mm
+++ b/src/dawn_native/metal/BufferMTL.mm
@@ -17,13 +17,21 @@
#include "common/Math.h"
#include "dawn_native/metal/DeviceMTL.h"
+#include <limits>
+
namespace dawn_native { namespace metal {
// The size of uniform buffer and storage buffer need to be aligned to 16 bytes which is the
// largest alignment of supported data types
static constexpr uint32_t kMinUniformOrStorageBufferAlignment = 16u;
- Buffer::Buffer(Device* device, const BufferDescriptor* descriptor)
- : BufferBase(device, descriptor) {
+ // static
+ ResultOrError<Buffer*> Buffer::Create(Device* device, const BufferDescriptor* descriptor) {
+ Ref<Buffer> buffer = AcquireRef(new Buffer(device, descriptor));
+ DAWN_TRY(buffer->Initialize());
+ return buffer.Detach();
+ }
+
+ MaybeError Buffer::Initialize() {
MTLResourceOptions storageMode;
if (GetUsage() & (wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite)) {
storageMode = MTLResourceStorageModeShared;
@@ -31,7 +39,14 @@
storageMode = MTLResourceStorageModePrivate;
}
- uint32_t currentSize = GetSize();
+ if (GetSize() >
+ std::numeric_limits<uint64_t>::max() - kMinUniformOrStorageBufferAlignment) {
+ return DAWN_OUT_OF_MEMORY_ERROR("Buffer allocation is too large");
+ }
+
+ // TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead
+ // of creating a new 4-byte buffer?
+ uint32_t currentSize = std::max(GetSize(), uint64_t(4u));
// Metal validation layer requires the size of uniform buffer and storage buffer to be no
// less than the size of the buffer block defined in shader, and the overall size of the
// buffer must be aligned to the largest alignment of its members.
@@ -39,7 +54,9 @@
currentSize = Align(currentSize, kMinUniformOrStorageBufferAlignment);
}
- mMtlBuffer = [device->GetMTLDevice() newBufferWithLength:currentSize options:storageMode];
+ mMtlBuffer = [ToBackend(GetDevice())->GetMTLDevice() newBufferWithLength:currentSize
+ options:storageMode];
+ return {};
}
Buffer::~Buffer() {
diff --git a/src/dawn_native/metal/DeviceMTL.mm b/src/dawn_native/metal/DeviceMTL.mm
index ba24d06..905832e 100644
--- a/src/dawn_native/metal/DeviceMTL.mm
+++ b/src/dawn_native/metal/DeviceMTL.mm
@@ -110,7 +110,7 @@
return new BindGroupLayout(this, descriptor);
}
ResultOrError<BufferBase*> Device::CreateBufferImpl(const BufferDescriptor* descriptor) {
- return new Buffer(this, descriptor);
+ return Buffer::Create(this, descriptor);
}
CommandBufferBase* Device::CreateCommandBuffer(CommandEncoder* encoder,
const CommandBufferDescriptor* descriptor) {
diff --git a/src/dawn_native/vulkan/BufferVk.cpp b/src/dawn_native/vulkan/BufferVk.cpp
index 55e15cc..4bc09e0 100644
--- a/src/dawn_native/vulkan/BufferVk.cpp
+++ b/src/dawn_native/vulkan/BufferVk.cpp
@@ -137,7 +137,9 @@
createInfo.sType = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO;
createInfo.pNext = nullptr;
createInfo.flags = 0;
- createInfo.size = GetSize();
+ // TODO(cwallez@chromium.org): Have a global "zero" buffer that can do everything instead
+ // of creating a new 4-byte buffer?
+ createInfo.size = std::max(GetSize(), uint64_t(4u));
// Add CopyDst for non-mappable buffer initialization in CreateBufferMapped
// and robust resource initialization.
createInfo.usage = VulkanBufferUsage(GetUsage() | wgpu::BufferUsage::CopyDst);
diff --git a/src/tests/end2end/BufferTests.cpp b/src/tests/end2end/BufferTests.cpp
index d00aa74..04af878 100644
--- a/src/tests/end2end/BufferTests.cpp
+++ b/src/tests/end2end/BufferTests.cpp
@@ -581,3 +581,18 @@
MetalBackend(),
OpenGLBackend(),
VulkanBackend());
+
+class BufferTests : public DawnTest {};
+
+TEST_P(BufferTests, ZeroSizedBuffer) {
+ wgpu::BufferDescriptor desc;
+ desc.size = 0;
+ desc.usage = wgpu::BufferUsage::CopyDst;
+ device.CreateBuffer(&desc);
+}
+
+DAWN_INSTANTIATE_TEST(BufferTests,
+ D3D12Backend(),
+ MetalBackend(),
+ OpenGLBackend(),
+ VulkanBackend());
