From 4171134daf9dbcef5652582fcd637312f8e5ac4a Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Wed, 7 Oct 2020 17:19:53 +0000 Subject: [PATCH] Buffer: Validate the offset is aligned to 8 This is to match the upstream WebGPU spec. Bug: dawn:445 Change-Id: I1a511ed9a2a04c7b95368ce724d69c128158f097 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/29360 Reviewed-by: Stephen White Reviewed-by: Austin Eng Commit-Queue: Corentin Wallez --- src/dawn_native/Buffer.cpp | 8 ++- src/dawn_wire/client/Buffer.cpp | 4 ++ src/tests/end2end/BufferTests.cpp | 51 ++++++++++--------- src/tests/end2end/BufferZeroInitTests.cpp | 4 +- .../validation/BufferValidationTests.cpp | 45 ++++++++-------- 5 files changed, 63 insertions(+), 49 deletions(-) diff --git a/src/dawn_native/Buffer.cpp b/src/dawn_native/Buffer.cpp index 3b2b750ffc..519b0e7d75 100644 --- a/src/dawn_native/Buffer.cpp +++ b/src/dawn_native/Buffer.cpp @@ -403,8 +403,8 @@ namespace dawn_native { *status = WGPUBufferMapAsyncStatus_Error; DAWN_TRY(GetDevice()->ValidateObject(this)); - if (offset % 4 != 0) { - return DAWN_VALIDATION_ERROR("offset must be a multiple of 4"); + if (offset % 8 != 0) { + return DAWN_VALIDATION_ERROR("offset must be a multiple of 8"); } if (size % 4 != 0) { @@ -448,6 +448,10 @@ namespace dawn_native { } bool BufferBase::CanGetMappedRange(bool writable, size_t offset, size_t size) const { + if (offset % 8 != 0 || size % 4 != 0) { + return false; + } + if (size > mMapSize || offset < mMapOffset) { return false; } diff --git a/src/dawn_wire/client/Buffer.cpp b/src/dawn_wire/client/Buffer.cpp index 4515caf0ed..d752831e5d 100644 --- a/src/dawn_wire/client/Buffer.cpp +++ b/src/dawn_wire/client/Buffer.cpp @@ -366,6 +366,10 @@ namespace dawn_wire { namespace client { } bool Buffer::CheckGetMappedRangeOffsetSize(size_t offset, size_t size) const { + if (offset % 8 != 0 || size % 4 != 0) { + return false; + } + if (size > mMapSize || offset < mMapOffset) { return false; } diff --git a/src/tests/end2end/BufferTests.cpp b/src/tests/end2end/BufferTests.cpp index 5d806246a9..339f4cc721 100644 --- a/src/tests/end2end/BufferTests.cpp +++ b/src/tests/end2end/BufferTests.cpp @@ -84,13 +84,13 @@ TEST_P(BufferMappingTests, MapRead_ZeroSized) { // Test map-reading with a non-zero offset TEST_P(BufferMappingTests, MapRead_NonZeroOffset) { - wgpu::Buffer buffer = CreateMapReadBuffer(8); + wgpu::Buffer buffer = CreateMapReadBuffer(12); - uint32_t myData[2] = {0x01020304, 0x05060708}; + uint32_t myData[3] = {0x01020304, 0x05060708, 0x090A0B0C}; queue.WriteBuffer(buffer, 0, &myData, sizeof(myData)); - MapAsyncAndWait(buffer, wgpu::MapMode::Read, 4, 4); - ASSERT_EQ(myData[1], *static_cast(buffer.GetConstMappedRange(4))); + MapAsyncAndWait(buffer, wgpu::MapMode::Read, 8, 4); + ASSERT_EQ(myData[2], *static_cast(buffer.GetConstMappedRange(8))); buffer.Unmap(); } @@ -133,23 +133,27 @@ TEST_P(BufferMappingTests, MapRead_Large) { 0, memcmp(buffer.GetConstMappedRange(8, kByteSize - 8), myData.data() + 2, kByteSize - 8)); buffer.Unmap(); - MapAsyncAndWait(buffer, wgpu::MapMode::Read, 12, kByteSize - 12); + MapAsyncAndWait(buffer, wgpu::MapMode::Read, 16, kByteSize - 16); + // Size is too big. EXPECT_EQ(nullptr, buffer.GetConstMappedRange(16, kByteSize - 12)); + // Offset defaults to 0 which is less than 16 EXPECT_EQ(nullptr, buffer.GetConstMappedRange()); + // Offset less than 8 is less than 16 EXPECT_EQ(nullptr, buffer.GetConstMappedRange(8)); - EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(12), myData.data() + 3, kByteSize - 12)); - EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(20), myData.data() + 5, kByteSize - 20)); - EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(20, kByteSize - 24), myData.data() + 5, - kByteSize - 44)); + + // Test a couple values. + EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(16), myData.data() + 4, kByteSize - 16)); + EXPECT_EQ(0, memcmp(buffer.GetConstMappedRange(24), myData.data() + 6, kByteSize - 24)); + buffer.Unmap(); } // Test that GetConstMappedRange works inside map-read callback TEST_P(BufferMappingTests, MapRead_InCallback) { - constexpr size_t kBufferSize = 8; + constexpr size_t kBufferSize = 12; wgpu::Buffer buffer = CreateMapReadBuffer(kBufferSize); - uint32_t myData[2] = {0x01020304, 0x05060708}; + uint32_t myData[3] = {0x01020304, 0x05060708, 0x090A0B0C}; constexpr size_t kSize = sizeof(myData); queue.WriteBuffer(buffer, 0, &myData, kSize); @@ -170,8 +174,8 @@ TEST_P(BufferMappingTests, MapRead_InCallback) { CheckMapping(user->buffer.GetConstMappedRange(), user->expected, kSize); CheckMapping(user->buffer.GetConstMappedRange(0, kSize), user->expected, kSize); - CheckMapping(user->buffer.GetConstMappedRange(4, 4), - static_cast(user->expected) + 1, sizeof(uint32_t)); + CheckMapping(user->buffer.GetConstMappedRange(8, 4), + static_cast(user->expected) + 2, sizeof(uint32_t)); user->buffer.Unmap(); } @@ -224,14 +228,14 @@ TEST_P(BufferMappingTests, MapWrite_ZeroSized) { // Test map-writing with a non-zero offset. TEST_P(BufferMappingTests, MapWrite_NonZeroOffset) { - wgpu::Buffer buffer = CreateMapWriteBuffer(8); + wgpu::Buffer buffer = CreateMapWriteBuffer(12); uint32_t myData = 2934875; - MapAsyncAndWait(buffer, wgpu::MapMode::Write, 4, 4); - memcpy(buffer.GetMappedRange(4), &myData, sizeof(myData)); + MapAsyncAndWait(buffer, wgpu::MapMode::Write, 8, 4); + memcpy(buffer.GetMappedRange(8), &myData, sizeof(myData)); buffer.Unmap(); - EXPECT_BUFFER_U32_EQ(myData, buffer, 4); + EXPECT_BUFFER_U32_EQ(myData, buffer, 8); } // Map, write and unmap twice. Test that both of these two iterations work. @@ -264,15 +268,14 @@ TEST_P(BufferMappingTests, MapWrite_Large) { myData.push_back(i); } - MapAsyncAndWait(buffer, wgpu::MapMode::Write, 12, kByteSize - 20); + MapAsyncAndWait(buffer, wgpu::MapMode::Write, 16, kByteSize - 20); EXPECT_EQ(nullptr, buffer.GetMappedRange()); EXPECT_EQ(nullptr, buffer.GetMappedRange(0)); EXPECT_EQ(nullptr, buffer.GetMappedRange(8)); EXPECT_EQ(nullptr, buffer.GetMappedRange(16, kByteSize - 8)); - EXPECT_EQ(nullptr, buffer.GetMappedRange(16, kByteSize - 20)); - memcpy(buffer.GetMappedRange(12), myData.data(), kByteSize - 20); + memcpy(buffer.GetMappedRange(16), myData.data(), kByteSize - 20); buffer.Unmap(); - EXPECT_BUFFER_U32_RANGE_EQ(myData.data(), buffer, 12, kDataSize - 5); + EXPECT_BUFFER_U32_RANGE_EQ(myData.data(), buffer, 16, kDataSize - 5); } // Stress test mapping many buffers. @@ -329,7 +332,7 @@ TEST_P(BufferMappingTests, OffsetNotUpdatedOnError) { // Map the buffer but do not wait on the result yet. bool done = false; buffer.MapAsync( - wgpu::MapMode::Read, 4, 4, + wgpu::MapMode::Read, 8, 4, [](WGPUBufferMapAsyncStatus status, void* userdata) { ASSERT_EQ(WGPUBufferMapAsyncStatus_Success, status); *static_cast(userdata) = true; @@ -338,14 +341,14 @@ TEST_P(BufferMappingTests, OffsetNotUpdatedOnError) { // Call MapAsync another time, it is an error because the buffer is already being mapped so // mMapOffset is not updated. - ASSERT_DEVICE_ERROR(buffer.MapAsync(wgpu::MapMode::Read, 8, 4, nullptr, nullptr)); + ASSERT_DEVICE_ERROR(buffer.MapAsync(wgpu::MapMode::Read, 0, 4, nullptr, nullptr)); while (!done) { WaitABit(); } // mMapOffset has not been updated so it should still be 4, which is data[1] - ASSERT_EQ(0, memcmp(buffer.GetConstMappedRange(4), &data[1], sizeof(uint32_t))); + ASSERT_EQ(0, memcmp(buffer.GetConstMappedRange(8), &data[2], sizeof(uint32_t))); } // Test that Get(Const)MappedRange work inside map-write callback. diff --git a/src/tests/end2end/BufferZeroInitTests.cpp b/src/tests/end2end/BufferZeroInitTests.cpp index 4304053374..c98b9e6f36 100644 --- a/src/tests/end2end/BufferZeroInitTests.cpp +++ b/src/tests/end2end/BufferZeroInitTests.cpp @@ -707,7 +707,7 @@ TEST_P(BufferZeroInitTest, MapAsync_Read) { { wgpu::Buffer buffer = CreateBuffer(kBufferSize, kBufferUsage); - constexpr uint64_t kOffset = 4u; + constexpr uint64_t kOffset = 8u; constexpr uint64_t kSize = 8u; EXPECT_LAZY_CLEAR(1u, MapAsyncAndWait(buffer, kMapMode, kOffset, kSize)); @@ -753,7 +753,7 @@ TEST_P(BufferZeroInitTest, MapAsync_Write) { { wgpu::Buffer buffer = CreateBuffer(kBufferSize, kBufferUsage); - constexpr uint64_t kOffset = 4u; + constexpr uint64_t kOffset = 8u; constexpr uint64_t kSize = 8u; EXPECT_LAZY_CLEAR(1u, MapAsyncAndWait(buffer, kMapMode, kOffset, kSize)); buffer.Unmap(); diff --git a/src/tests/unittests/validation/BufferValidationTests.cpp b/src/tests/unittests/validation/BufferValidationTests.cpp index d0179d699e..c895b1b002 100644 --- a/src/tests/unittests/validation/BufferValidationTests.cpp +++ b/src/tests/unittests/validation/BufferValidationTests.cpp @@ -171,24 +171,24 @@ TEST_F(BufferValidationTest, MapAsync_ErrorBuffer) { // Test map async with an invalid offset and size alignment. TEST_F(BufferValidationTest, MapAsync_OffsetSizeAlignment) { - // Control case, both aligned to 4 is ok. + // Control case, offset aligned to 8 and size to 4 is valid { - wgpu::Buffer buffer = CreateMapReadBuffer(8); - buffer.MapAsync(wgpu::MapMode::Read, 4, 4, nullptr, nullptr); + wgpu::Buffer buffer = CreateMapReadBuffer(12); + buffer.MapAsync(wgpu::MapMode::Read, 8, 4, nullptr, nullptr); } { - wgpu::Buffer buffer = CreateMapWriteBuffer(8); - buffer.MapAsync(wgpu::MapMode::Write, 4, 4, nullptr, nullptr); + wgpu::Buffer buffer = CreateMapWriteBuffer(12); + buffer.MapAsync(wgpu::MapMode::Write, 8, 4, nullptr, nullptr); } - // Error case, offset aligned to 2 is an error. + // Error case, offset aligned to 4 is an error. { - wgpu::Buffer buffer = CreateMapReadBuffer(8); - AssertMapAsyncError(buffer, wgpu::MapMode::Read, 2, 4); + wgpu::Buffer buffer = CreateMapReadBuffer(12); + AssertMapAsyncError(buffer, wgpu::MapMode::Read, 4, 4); } { - wgpu::Buffer buffer = CreateMapWriteBuffer(8); - AssertMapAsyncError(buffer, wgpu::MapMode::Write, 2, 4); + wgpu::Buffer buffer = CreateMapWriteBuffer(12); + AssertMapAsyncError(buffer, wgpu::MapMode::Write, 4, 4); } // Error case, size aligned to 2 is an error. @@ -212,8 +212,8 @@ TEST_F(BufferValidationTest, MapAsync_OffsetSizeOOB) { // Valid case: range in the middle of the buffer is ok. { - wgpu::Buffer buffer = CreateMapReadBuffer(12); - buffer.MapAsync(wgpu::MapMode::Read, 4, 4, nullptr, nullptr); + wgpu::Buffer buffer = CreateMapReadBuffer(16); + buffer.MapAsync(wgpu::MapMode::Read, 8, 4, nullptr, nullptr); } // Valid case: empty range at the end of the buffer is ok. @@ -224,8 +224,8 @@ TEST_F(BufferValidationTest, MapAsync_OffsetSizeOOB) { // Error case, offset is larger than the buffer size (even if size is 0). { - wgpu::Buffer buffer = CreateMapReadBuffer(8); - AssertMapAsyncError(buffer, wgpu::MapMode::Read, 12, 0); + wgpu::Buffer buffer = CreateMapReadBuffer(12); + AssertMapAsyncError(buffer, wgpu::MapMode::Read, 16, 0); } // Error case, offset + size is larger than the buffer @@ -237,8 +237,8 @@ TEST_F(BufferValidationTest, MapAsync_OffsetSizeOOB) { // Error case, offset + size is larger than the buffer, overflow case. { wgpu::Buffer buffer = CreateMapReadBuffer(12); - AssertMapAsyncError(buffer, wgpu::MapMode::Read, 4, - std::numeric_limits::max() & ~size_t(3)); + AssertMapAsyncError(buffer, wgpu::MapMode::Read, 8, + std::numeric_limits::max() & ~size_t(7)); } } @@ -831,9 +831,9 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { // Valid case: range in the middle is ok. { - wgpu::Buffer buffer = CreateMapWriteBuffer(12); - buffer.MapAsync(wgpu::MapMode::Write, 0, 12, nullptr, nullptr); - EXPECT_NE(buffer.GetMappedRange(4, 4), nullptr); + wgpu::Buffer buffer = CreateMapWriteBuffer(16); + buffer.MapAsync(wgpu::MapMode::Write, 0, 16, nullptr, nullptr); + EXPECT_NE(buffer.GetMappedRange(8, 4), nullptr); } // Error case: offset is larger than the mapped range (even with size = 0) @@ -841,6 +841,7 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { wgpu::Buffer buffer = CreateMapWriteBuffer(8); buffer.MapAsync(wgpu::MapMode::Write, 0, 8, nullptr, nullptr); EXPECT_EQ(buffer.GetMappedRange(9, 0), nullptr); + EXPECT_EQ(buffer.GetMappedRange(16, 0), nullptr); } // Error case: offset + size is larger than the mapped range @@ -848,6 +849,7 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { wgpu::Buffer buffer = CreateMapWriteBuffer(12); buffer.MapAsync(wgpu::MapMode::Write, 0, 12, nullptr, nullptr); EXPECT_EQ(buffer.GetMappedRange(8, 5), nullptr); + EXPECT_EQ(buffer.GetMappedRange(8, 8), nullptr); } // Error case: offset + size is larger than the mapped range, overflow case @@ -860,7 +862,8 @@ TEST_F(BufferValidationTest, GetMappedRange_OffsetSizeOOB) { // Error case: offset is before the start of the range { wgpu::Buffer buffer = CreateMapWriteBuffer(12); - buffer.MapAsync(wgpu::MapMode::Write, 4, 4, nullptr, nullptr); - EXPECT_EQ(buffer.GetMappedRange(3, 4), nullptr); + buffer.MapAsync(wgpu::MapMode::Write, 8, 4, nullptr, nullptr); + EXPECT_EQ(buffer.GetMappedRange(7, 4), nullptr); + EXPECT_EQ(buffer.GetMappedRange(0, 4), nullptr); } }