Vulkan: make mappableBuffersForEagerTransition ref buffers
There are some scenarios where buffers can be used in pending
commands that are not retained by a command buffer.
They must be retained in the set of mappable buffers for eager
transition to prevent a use-after-free violation.
No-Try: true
Fixed: chromium:1421170
Change-Id: I452d80b2513a7726a003d44e2a7850292d798bb1
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/122580
Auto-Submit: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Austin Eng <enga@chromium.org>
Reviewed-by: Loko Kung <lokokung@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
(cherry picked from commit aaae3ffadafafe1fea4fa141420b939bc82e2934)
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/123040
Kokoro: Austin Eng <enga@chromium.org>
diff --git a/src/dawn/common/RefBase.h b/src/dawn/common/RefBase.h
index 8f06f19..1539547 100644
--- a/src/dawn/common/RefBase.h
+++ b/src/dawn/common/RefBase.h
@@ -110,6 +110,8 @@
const T operator->() const { return mValue; }
T operator->() { return mValue; }
+ bool operator<(const RefBase<T, Traits>& other) const { return mValue < other.mValue; }
+
// Smart pointer methods.
const T& Get() const { return mValue; }
T& Get() { return mValue; }
diff --git a/src/dawn/native/vulkan/BufferVk.cpp b/src/dawn/native/vulkan/BufferVk.cpp
index eeafa22..73150bc 100644
--- a/src/dawn/native/vulkan/BufferVk.cpp
+++ b/src/dawn/native/vulkan/BufferVk.cpp
@@ -416,9 +416,8 @@
// static
void Buffer::TransitionMappableBuffersEagerly(const VulkanFunctions& fn,
CommandRecordingContext* recordingContext,
- std::set<Buffer*> buffers) {
+ const std::set<Ref<Buffer>>& buffers) {
ASSERT(!buffers.empty());
- ASSERT(recordingContext->mappableBuffersForEagerTransition.empty());
VkPipelineStageFlags srcStages = 0;
VkPipelineStageFlags dstStages = 0;
@@ -426,7 +425,8 @@
std::vector<VkBufferMemoryBarrier> barriers;
barriers.reserve(buffers.size());
- for (Buffer* buffer : buffers) {
+ size_t originalBufferCount = buffers.size();
+ for (const Ref<Buffer>& buffer : buffers) {
wgpu::BufferUsage mapUsage = buffer->GetUsage() & kMapUsages;
ASSERT(mapUsage == wgpu::BufferUsage::MapRead || mapUsage == wgpu::BufferUsage::MapWrite);
VkBufferMemoryBarrier barrier;
@@ -435,9 +435,9 @@
&srcStages, &dstStages)) {
barriers.push_back(barrier);
}
- // TrackUsageAndGetResourceBarrier() should not modify recordingContext for map usages.
- ASSERT(recordingContext->mappableBuffersForEagerTransition.empty());
}
+ // TrackUsageAndGetResourceBarrier() should not modify recordingContext for map usages.
+ ASSERT(buffers.size() == originalBufferCount);
if (barriers.empty()) {
return;
diff --git a/src/dawn/native/vulkan/BufferVk.h b/src/dawn/native/vulkan/BufferVk.h
index c806b00..2c11e14 100644
--- a/src/dawn/native/vulkan/BufferVk.h
+++ b/src/dawn/native/vulkan/BufferVk.h
@@ -58,7 +58,7 @@
static void TransitionMappableBuffersEagerly(const VulkanFunctions& fn,
CommandRecordingContext* recordingContext,
- std::set<Buffer*> buffers);
+ const std::set<Ref<Buffer>>& buffers);
private:
~Buffer() override;
diff --git a/src/dawn/native/vulkan/CommandRecordingContext.h b/src/dawn/native/vulkan/CommandRecordingContext.h
index a20db63..dc429b3 100644
--- a/src/dawn/native/vulkan/CommandRecordingContext.h
+++ b/src/dawn/native/vulkan/CommandRecordingContext.h
@@ -50,7 +50,7 @@
// Mappable buffers which will be eagerly transitioned to usage MapRead or MapWrite after
// VkSubmit.
- std::set<Buffer*> mappableBuffersForEagerTransition;
+ std::set<Ref<Buffer>> mappableBuffersForEagerTransition;
// For Device state tracking only.
VkCommandPool commandPool = VK_NULL_HANDLE;
diff --git a/src/dawn/native/vulkan/DeviceVk.cpp b/src/dawn/native/vulkan/DeviceVk.cpp
index 9375be3..b3e5994 100644
--- a/src/dawn/native/vulkan/DeviceVk.cpp
+++ b/src/dawn/native/vulkan/DeviceVk.cpp
@@ -321,7 +321,7 @@
if (!mRecordingContext.mappableBuffersForEagerTransition.empty()) {
// Transition mappable buffers back to map usages with the submit.
Buffer::TransitionMappableBuffersEagerly(
- fn, &mRecordingContext, std::move(mRecordingContext.mappableBuffersForEagerTransition));
+ fn, &mRecordingContext, mRecordingContext.mappableBuffersForEagerTransition);
}
ScopedSignalSemaphore scopedSignalSemaphore(this, VK_NULL_HANDLE);
diff --git a/src/dawn/tests/end2end/BufferTests.cpp b/src/dawn/tests/end2end/BufferTests.cpp
index 8e3f2a5..b12ae0e 100644
--- a/src/dawn/tests/end2end/BufferTests.cpp
+++ b/src/dawn/tests/end2end/BufferTests.cpp
@@ -534,6 +534,24 @@
MapAsyncAndWait(buffer, wgpu::MapMode::Write, 0, wgpu::kWholeMapSize);
}
+// Regression test for crbug.com/1421170 where dropping a buffer which needs
+// padding bytes initialization resulted in a use-after-free.
+TEST_P(BufferMappingTests, RegressChromium1421170) {
+ // Create a mappable buffer of size 7. It will be internally
+ // aligned such that the padding bytes need to be zero initialized.
+ wgpu::BufferDescriptor descriptor;
+ descriptor.size = 7;
+ descriptor.usage = wgpu::BufferUsage::MapWrite;
+ descriptor.mappedAtCreation = false;
+ wgpu::Buffer buffer = device.CreateBuffer(&descriptor);
+
+ // Drop the buffer. The pending commands to zero initialize the
+ // padding bytes should stay valid.
+ buffer = nullptr;
+ // Flush pending commands.
+ device.Tick();
+}
+
DAWN_INSTANTIATE_TEST(BufferMappingTests,
D3D12Backend(),
MetalBackend(),
