From 7f8c91cddeba24c993c7dfcc04171fc36e940a0b Mon Sep 17 00:00:00 2001 From: Aleksi Sapon Date: Tue, 30 Mar 2021 13:06:17 +0000 Subject: [PATCH] Fix crash from async callback calling Queue::Submit An async callback which calls Queue::Submit will cause reentrance in QueueBase::Tick and CreatePipelineAsyncTracker::Tick, which invalidates the task queue being used by the original call, and leads to a crash from an invalid pointer. The Tick functions should remove the tasks from the queues before the callbacks are called, so invalidation doesn't cause a crash. Bug: dawn:729 Change-Id: I0d952d51040a3d1a475767400de3333a8b9b0821 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/45900 Commit-Queue: Corentin Wallez Reviewed-by: Austin Eng --- .../CreatePipelineAsyncTracker.cpp | 12 ++- src/dawn_native/Queue.cpp | 12 ++- src/dawn_native/null/DeviceNull.cpp | 8 +- .../validation/QueueSubmitValidationTests.cpp | 92 +++++++++++++++++++ 4 files changed, 121 insertions(+), 3 deletions(-) diff --git a/src/dawn_native/CreatePipelineAsyncTracker.cpp b/src/dawn_native/CreatePipelineAsyncTracker.cpp index 7239774168..2228111484 100644 --- a/src/dawn_native/CreatePipelineAsyncTracker.cpp +++ b/src/dawn_native/CreatePipelineAsyncTracker.cpp @@ -98,10 +98,20 @@ namespace dawn_native { } void CreatePipelineAsyncTracker::Tick(ExecutionSerial finishedSerial) { + // If a user calls Queue::Submit inside Create*PipelineAsync, then the device will be + // ticked, which in turns ticks the tracker, causing reentrance here. To prevent the + // reentrant call from invalidating mCreatePipelineAsyncTasksInFlight while in use by the + // first call, we remove the tasks to finish from the queue, update + // mCreatePipelineAsyncTasksInFlight, then run the callbacks. + std::vector> tasks; for (auto& task : mCreatePipelineAsyncTasksInFlight.IterateUpTo(finishedSerial)) { - task->Finish(WGPUCreatePipelineAsyncStatus_Success); + tasks.push_back(std::move(task)); } mCreatePipelineAsyncTasksInFlight.ClearUpTo(finishedSerial); + + for (auto& task : tasks) { + task->Finish(WGPUCreatePipelineAsyncStatus_Success); + } } void CreatePipelineAsyncTracker::ClearForShutDown() { diff --git a/src/dawn_native/Queue.cpp b/src/dawn_native/Queue.cpp index a62db1ba41..d319b64503 100644 --- a/src/dawn_native/Queue.cpp +++ b/src/dawn_native/Queue.cpp @@ -223,10 +223,20 @@ namespace dawn_native { } void QueueBase::Tick(ExecutionSerial finishedSerial) { + // If a user calls Queue::Submit inside a task, for example in a Buffer::MapAsync callback, + // then the device will be ticked, which in turns ticks the queue, causing reentrance here. + // To prevent the reentrant call from invalidating mTasksInFlight while in use by the first + // call, we remove the tasks to finish from the queue, update mTasksInFlight, then run the + // callbacks. + std::vector> tasks; for (auto& task : mTasksInFlight.IterateUpTo(finishedSerial)) { - task->Finish(); + tasks.push_back(std::move(task)); } mTasksInFlight.ClearUpTo(finishedSerial); + + for (auto& task : tasks) { + task->Finish(); + } } void QueueBase::HandleDeviceLoss() { diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 3ac5ac8c29..23dbe2032f 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -334,7 +334,13 @@ namespace dawn_native { namespace null { } MaybeError Queue::SubmitImpl(uint32_t, CommandBufferBase* const*) { - ToBackend(GetDevice())->SubmitPendingOperations(); + Device* device = ToBackend(GetDevice()); + + // The Vulkan, D3D12 and Metal implementation all tick the device here, + // for testing purposes we should also tick in the null implementation. + DAWN_TRY(device->Tick()); + + device->SubmitPendingOperations(); return {}; } diff --git a/src/tests/unittests/validation/QueueSubmitValidationTests.cpp b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp index ed7fd7d9ef..06b3a41a49 100644 --- a/src/tests/unittests/validation/QueueSubmitValidationTests.cpp +++ b/src/tests/unittests/validation/QueueSubmitValidationTests.cpp @@ -14,6 +14,7 @@ #include "tests/unittests/validation/ValidationTest.h" +#include "utils/ComboRenderPipelineDescriptor.h" #include "utils/WGPUHelpers.h" namespace { @@ -129,4 +130,95 @@ namespace { ASSERT_DEVICE_ERROR(queue.Submit(1, &commands)); } + // Test that submitting in a buffer mapping callback doesn't cause re-entrance problems. + TEST_F(QueueSubmitValidationTest, SubmitInBufferMapCallback) { + // Create a buffer for mapping, to run our callback. + wgpu::BufferDescriptor descriptor; + descriptor.size = 4; + descriptor.usage = wgpu::BufferUsage::MapWrite; + wgpu::Buffer buffer = device.CreateBuffer(&descriptor); + + struct CallbackData { + wgpu::Device device; + wgpu::Buffer buffer; + } callbackData = {device, buffer}; + + const auto callback = [](WGPUBufferMapAsyncStatus status, void* userdata) { + CallbackData* data = reinterpret_cast(userdata); + + data->buffer.Unmap(); + + wgpu::Queue queue = data->device.GetQueue(); + queue.Submit(0, nullptr); + }; + + buffer.MapAsync(wgpu::MapMode::Write, 0, descriptor.size, callback, &callbackData); + + WaitForAllOperations(device); + } + + // Test that submitting in a render pipeline creation callback doesn't cause re-entrance + // problems. + TEST_F(QueueSubmitValidationTest, SubmitInCreateRenderPipelineAsyncCallback) { + struct CallbackData { + wgpu::Device device; + } callbackData = {device}; + + const auto callback = [](WGPUCreatePipelineAsyncStatus status, WGPURenderPipeline pipeline, + char const* message, void* userdata) { + CallbackData* data = reinterpret_cast(userdata); + + wgpuRenderPipelineRelease(pipeline); + + wgpu::Queue queue = data->device.GetQueue(); + queue.Submit(0, nullptr); + }; + + wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"( + [[builtin(position)]] var Position : vec4; + [[stage(vertex)]] fn main() -> void { + Position = vec4(0.0, 0.0, 0.0, 1.0); + })"); + + wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, R"( + [[location(0)]] var fragColor : vec4; + [[stage(fragment)]] fn main() -> void { + fragColor = vec4(0.0, 1.0, 0.0, 1.0); + })"); + + utils::ComboRenderPipelineDescriptor2 descriptor; + descriptor.vertex.module = vsModule; + descriptor.cFragment.module = fsModule; + device.CreateRenderPipelineAsync(&descriptor, callback, &callbackData); + + WaitForAllOperations(device); + } + + // Test that submitting in a compute pipeline creation callback doesn't cause re-entrance + // problems. + TEST_F(QueueSubmitValidationTest, SubmitInCreateComputePipelineAsyncCallback) { + struct CallbackData { + wgpu::Device device; + } callbackData = {device}; + + const auto callback = [](WGPUCreatePipelineAsyncStatus status, WGPUComputePipeline pipeline, + char const* message, void* userdata) { + CallbackData* data = reinterpret_cast(userdata); + + wgpuComputePipelineRelease(pipeline); + + wgpu::Queue queue = data->device.GetQueue(); + queue.Submit(0, nullptr); + }; + + wgpu::ComputePipelineDescriptor descriptor; + descriptor.computeStage.module = utils::CreateShaderModule(device, R"( + [[stage(compute)]] fn main() -> void { + })"); + descriptor.computeStage.entryPoint = "main"; + device.CreateComputePipelineAsync(&descriptor, callback, &callbackData); + + WaitForAllOperations(device); + } + } // anonymous namespace