From d6eb2e75016b3e893b0260fa91be93e0d49c3c9b Mon Sep 17 00:00:00 2001 From: Jiawei Shao Date: Tue, 5 Mar 2019 00:03:28 +0000 Subject: [PATCH] Fix crashes when using error textures in copy commands This patch fixes a Dawn crash issue when using textures in error state in the copy commands of CommandEncoder. In Dawn's copy commands (copyBufferToTexture and CopyTextureToBuffer), we should check if the texture is in error state or not, or the assert ASSERT(!IsError()) in texture->GetFormat() will fail and a crash will occur. In current Dawn code the validations on the buffer and texture objects in the copy commands are executed in CommandEncoder::Finish(), which is too late for textures according to the previous investigation. This patch moves all these validations to the call of copy commands. The checks on buffers are also moved away to keep the consistency of the ones on textures. BUG=chromium:937628 TEST=dawn_unittests Change-Id: I0bc44e76262fba5927df20c6a7551b107bad5ca1 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/5240 Reviewed-by: Kai Ninomiya Commit-Queue: Jiawei Shao --- src/dawn_native/CommandEncoder.cpp | 26 +---- .../CopyCommandsValidationTests.cpp | 105 ++++++++++++++++++ 2 files changed, 111 insertions(+), 20 deletions(-) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 22f7fb8313..ef186a1b09 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -555,13 +555,11 @@ namespace dawn_native { return; } - if (source == nullptr) { - HandleError("Source cannot be null"); + if (ConsumedError(GetDevice()->ValidateObject(source))) { return; } - if (destination == nullptr) { - HandleError("Destination cannot be null"); + if (ConsumedError(GetDevice()->ValidateObject(destination))) { return; } @@ -582,13 +580,11 @@ namespace dawn_native { return; } - if (source->buffer == nullptr) { - HandleError("Buffer cannot be null"); + if (ConsumedError(GetDevice()->ValidateObject(source->buffer))) { return; } - if (destination->texture == nullptr) { - HandleError("Texture cannot be null"); + if (ConsumedError(GetDevice()->ValidateObject(destination->texture))) { return; } @@ -621,13 +617,11 @@ namespace dawn_native { return; } - if (source->texture == nullptr) { - HandleError("Texture cannot be null"); + if (ConsumedError(GetDevice()->ValidateObject(source->texture))) { return; } - if (destination->buffer == nullptr) { - HandleError("Buffer cannot be null"); + if (ConsumedError(GetDevice()->ValidateObject(destination->buffer))) { return; } @@ -721,8 +715,6 @@ namespace dawn_native { case Command::CopyBufferToBuffer: { CopyBufferToBufferCmd* copy = mIterator.NextCommand(); - DAWN_TRY(GetDevice()->ValidateObject(copy->source.buffer.Get())); - DAWN_TRY(GetDevice()->ValidateObject(copy->destination.buffer.Get())); DAWN_TRY(ValidateCopySizeFitsInBuffer(copy->source, copy->size)); DAWN_TRY(ValidateCopySizeFitsInBuffer(copy->destination, copy->size)); DAWN_TRY(ValidateB2BCopySizeAlignment(copy->size, copy->source.offset, @@ -740,9 +732,6 @@ namespace dawn_native { case Command::CopyBufferToTexture: { CopyBufferToTextureCmd* copy = mIterator.NextCommand(); - DAWN_TRY(GetDevice()->ValidateObject(copy->source.buffer.Get())); - DAWN_TRY(GetDevice()->ValidateObject(copy->destination.texture.Get())); - DAWN_TRY( ValidateTextureSampleCountInCopyCommands(copy->destination.texture.Get())); @@ -771,9 +760,6 @@ namespace dawn_native { case Command::CopyTextureToBuffer: { CopyTextureToBufferCmd* copy = mIterator.NextCommand(); - DAWN_TRY(GetDevice()->ValidateObject(copy->source.texture.Get())); - DAWN_TRY(GetDevice()->ValidateObject(copy->destination.buffer.Get())); - DAWN_TRY(ValidateTextureSampleCountInCopyCommands(copy->source.texture.Get())); uint32_t bufferCopySize = 0; diff --git a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp index a11731b962..dbb6836571 100644 --- a/src/tests/unittests/validation/CopyCommandsValidationTests.cpp +++ b/src/tests/unittests/validation/CopyCommandsValidationTests.cpp @@ -200,6 +200,29 @@ TEST_F(CopyCommandTest_B2B, UnalignedOffset) { } } +// Test B2B copies with buffers in error state cause errors. +TEST_F(CopyCommandTest_B2B, BuffersInErrorState) { + dawn::BufferDescriptor errorBufferDescriptor; + errorBufferDescriptor.size = 4; + errorBufferDescriptor.usage = dawn::BufferUsageBit::MapRead | dawn::BufferUsageBit::TransferSrc; + ASSERT_DEVICE_ERROR(dawn::Buffer errorBuffer = device.CreateBuffer(&errorBufferDescriptor)); + + constexpr uint32_t bufferSize = 4; + dawn::Buffer validBuffer = CreateBuffer(bufferSize, dawn::BufferUsageBit::TransferSrc); + + { + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(errorBuffer, 0, validBuffer, 0, 4); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + { + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(validBuffer, 0, errorBuffer, 0, 4); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +} + class CopyCommandTest_B2T : public CopyCommandTest { }; @@ -420,6 +443,47 @@ TEST_F(CopyCommandTest_B2T, CopyToMultisampledTexture) { {2, 2, 1}); } +// Test B2T copies with buffer or texture in error state causes errors. +TEST_F(CopyCommandTest_B2T, BufferOrTextureInErrorState) { + dawn::BufferDescriptor errorBufferDescriptor; + errorBufferDescriptor.size = 4; + errorBufferDescriptor.usage = dawn::BufferUsageBit::MapRead | dawn::BufferUsageBit::TransferSrc; + ASSERT_DEVICE_ERROR(dawn::Buffer errorBuffer = device.CreateBuffer(&errorBufferDescriptor)); + + dawn::TextureDescriptor errorTextureDescriptor; + errorTextureDescriptor.arrayLayerCount = 0; + ASSERT_DEVICE_ERROR(dawn::Texture errorTexture = device.CreateTexture(&errorTextureDescriptor)); + + dawn::BufferCopyView errorBufferCopyView = utils::CreateBufferCopyView(errorBuffer, 0, 0, 0); + dawn::TextureCopyView errorTextureCopyView = + utils::CreateTextureCopyView(errorTexture, 0, 0, {1, 1, 1}); + + dawn::Extent3D extent3D = {1, 1, 1}; + + { + dawn::Texture destination = + Create2DTexture(16, 16, 1, 1, dawn::TextureFormat::R8G8B8A8Unorm, + dawn::TextureUsageBit::TransferDst); + dawn::TextureCopyView textureCopyView = + utils::CreateTextureCopyView(destination, 0, 0, {1, 1, 1}); + + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToTexture(&errorBufferCopyView, &textureCopyView, &extent3D); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + { + uint32_t bufferSize = BufferSizeForTextureCopy(4, 4, 1); + dawn::Buffer source = CreateBuffer(bufferSize, dawn::BufferUsageBit::TransferSrc); + + dawn::BufferCopyView bufferCopyView = utils::CreateBufferCopyView(source, 0, 0, 0); + + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToTexture(&bufferCopyView, &errorTextureCopyView, &extent3D); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +} + class CopyCommandTest_T2B : public CopyCommandTest { }; @@ -633,3 +697,44 @@ TEST_F(CopyCommandTest_T2B, CopyFromMultisampledTexture) { TestT2BCopy(utils::Expectation::Failure, source, 0, 0, {0, 0, 0}, destination, 0, 256, 0, {2, 2, 1}); } + +// Test T2B copies with buffer or texture in error state cause errors. +TEST_F(CopyCommandTest_T2B, BufferOrTextureInErrorState) { + dawn::BufferDescriptor errorBufferDescriptor; + errorBufferDescriptor.size = 4; + errorBufferDescriptor.usage = dawn::BufferUsageBit::MapRead | dawn::BufferUsageBit::TransferSrc; + ASSERT_DEVICE_ERROR(dawn::Buffer errorBuffer = device.CreateBuffer(&errorBufferDescriptor)); + + dawn::TextureDescriptor errorTextureDescriptor; + errorTextureDescriptor.arrayLayerCount = 0; + ASSERT_DEVICE_ERROR(dawn::Texture errorTexture = device.CreateTexture(&errorTextureDescriptor)); + + dawn::BufferCopyView errorBufferCopyView = utils::CreateBufferCopyView(errorBuffer, 0, 0, 0); + dawn::TextureCopyView errorTextureCopyView = + utils::CreateTextureCopyView(errorTexture, 0, 0, {1, 1, 1}); + + dawn::Extent3D extent3D = {1, 1, 1}; + + { + uint32_t bufferSize = BufferSizeForTextureCopy(4, 4, 1); + dawn::Buffer source = CreateBuffer(bufferSize, dawn::BufferUsageBit::TransferSrc); + + dawn::BufferCopyView bufferCopyView = utils::CreateBufferCopyView(source, 0, 0, 0); + + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyTextureToBuffer(&errorTextureCopyView, &bufferCopyView, &extent3D); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } + + { + dawn::Texture destination = + Create2DTexture(16, 16, 1, 1, dawn::TextureFormat::R8G8B8A8Unorm, + dawn::TextureUsageBit::TransferDst); + dawn::TextureCopyView textureCopyView = + utils::CreateTextureCopyView(destination, 0, 0, {1, 1, 1}); + + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyTextureToBuffer(&textureCopyView, &errorBufferCopyView, &extent3D); + ASSERT_DEVICE_ERROR(encoder.Finish()); + } +}