From aaae3ffadafafe1fea4fa141420b939bc82e2934 Mon Sep 17 00:00:00 2001 From: Austin Eng Date: Mon, 6 Mar 2023 22:09:26 +0000 Subject: [PATCH] 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. Fixed: chromium:1421170 Change-Id: I452d80b2513a7726a003d44e2a7850292d798bb1 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/122580 Auto-Submit: Austin Eng Kokoro: Kokoro Commit-Queue: Austin Eng Reviewed-by: Loko Kung Reviewed-by: Corentin Wallez Reviewed-by: Peng Huang --- src/dawn/common/RefBase.h | 2 ++ src/dawn/native/vulkan/BufferVk.cpp | 10 +++++----- src/dawn/native/vulkan/BufferVk.h | 2 +- .../native/vulkan/CommandRecordingContext.h | 2 +- src/dawn/native/vulkan/DeviceVk.cpp | 2 +- src/dawn/tests/end2end/BufferTests.cpp | 18 ++++++++++++++++++ 6 files changed, 28 insertions(+), 8 deletions(-) diff --git a/src/dawn/common/RefBase.h b/src/dawn/common/RefBase.h index 8f06f19f15..1539547176 100644 --- a/src/dawn/common/RefBase.h +++ b/src/dawn/common/RefBase.h @@ -110,6 +110,8 @@ class RefBase { const T operator->() const { return mValue; } T operator->() { return mValue; } + bool operator<(const RefBase& 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 eeafa2273e..73150bcdf3 100644 --- a/src/dawn/native/vulkan/BufferVk.cpp +++ b/src/dawn/native/vulkan/BufferVk.cpp @@ -416,9 +416,8 @@ bool Buffer::EnsureDataInitializedAsDestination(CommandRecordingContext* recordi // static void Buffer::TransitionMappableBuffersEagerly(const VulkanFunctions& fn, CommandRecordingContext* recordingContext, - std::set buffers) { + const std::set>& buffers) { ASSERT(!buffers.empty()); - ASSERT(recordingContext->mappableBuffersForEagerTransition.empty()); VkPipelineStageFlags srcStages = 0; VkPipelineStageFlags dstStages = 0; @@ -426,7 +425,8 @@ void Buffer::TransitionMappableBuffersEagerly(const VulkanFunctions& fn, std::vector barriers; barriers.reserve(buffers.size()); - for (Buffer* buffer : buffers) { + size_t originalBufferCount = buffers.size(); + for (const Ref& buffer : buffers) { wgpu::BufferUsage mapUsage = buffer->GetUsage() & kMapUsages; ASSERT(mapUsage == wgpu::BufferUsage::MapRead || mapUsage == wgpu::BufferUsage::MapWrite); VkBufferMemoryBarrier barrier; @@ -435,9 +435,9 @@ void Buffer::TransitionMappableBuffersEagerly(const VulkanFunctions& fn, &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 c806b001f6..2c11e14aaf 100644 --- a/src/dawn/native/vulkan/BufferVk.h +++ b/src/dawn/native/vulkan/BufferVk.h @@ -58,7 +58,7 @@ class Buffer final : public BufferBase { static void TransitionMappableBuffersEagerly(const VulkanFunctions& fn, CommandRecordingContext* recordingContext, - std::set buffers); + const std::set>& buffers); private: ~Buffer() override; diff --git a/src/dawn/native/vulkan/CommandRecordingContext.h b/src/dawn/native/vulkan/CommandRecordingContext.h index a20db63c42..dc429b3606 100644 --- a/src/dawn/native/vulkan/CommandRecordingContext.h +++ b/src/dawn/native/vulkan/CommandRecordingContext.h @@ -50,7 +50,7 @@ struct CommandRecordingContext { // Mappable buffers which will be eagerly transitioned to usage MapRead or MapWrite after // VkSubmit. - std::set mappableBuffersForEagerTransition; + std::set> 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 370b3b593a..907ca38f41 100644 --- a/src/dawn/native/vulkan/DeviceVk.cpp +++ b/src/dawn/native/vulkan/DeviceVk.cpp @@ -310,7 +310,7 @@ MaybeError Device::SubmitPendingCommands() { 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 8e3f2a5e8c..b12ae0ea60 100644 --- a/src/dawn/tests/end2end/BufferTests.cpp +++ b/src/dawn/tests/end2end/BufferTests.cpp @@ -534,6 +534,24 @@ TEST_P(BufferMappingTests, MapWrite_ZeroSizedTwice) { 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(),