Vulkan: Fix use-after-free with DynamicBuffer.

src/libANGLE/renderer/vulkan/TextureVk.cpp

@@ -174,6 +174,7 @@ vk::Error PixelBuffer::flushUpdatesToImage(RendererVk *renderer,
     }
 
     mSubresourceUpdates.clear();
+    mStagingBuffer.releaseRetainedBuffers(renderer);
 
     return vk::NoError();
 }

src/libANGLE/renderer/vulkan/VertexArrayVk.cpp

@@ -58,7 +58,6 @@ VertexArrayVk::~VertexArrayVk()
 void VertexArrayVk::destroy(const gl::Context *context)
 {
     VkDevice device = vk::GetImpl(context)->getRenderer()->getDevice();
-
     mDynamicVertexData.destroy(device);
     mDynamicIndexData.destroy(device);
     mLineLoopHelper.destroy(device);
@@ -108,6 +107,7 @@ gl::Error VertexArrayVk::streamVertexData(RendererVk *renderer,
     }
 
     ANGLE_TRY(mDynamicVertexData.flush(renderer->getDevice()));
+    mDynamicVertexData.releaseRetainedBuffers(renderer);
     return gl::NoError();
 }
 
@@ -139,7 +139,7 @@ gl::Error VertexArrayVk::streamIndexData(RendererVk *renderer,
         memcpy(dst, drawCallParams.indices(), amount);
     }
     ANGLE_TRY(mDynamicIndexData.flush(renderer->getDevice()));
-
+    mDynamicIndexData.releaseRetainedBuffers(renderer);
     mCurrentElementArrayBufferOffset = offset;
     return gl::NoError();
 }

src/libANGLE/renderer/vulkan/vk_helpers.cpp

@@ -7,6 +7,7 @@
 //   Helper utilitiy classes that manage Vulkan resources.
 
 #include "libANGLE/renderer/vulkan/vk_helpers.h"
+#include "libANGLE/renderer/vulkan/vk_utils.h"
 
 #include "libANGLE/renderer/vulkan/BufferVk.h"
 #include "libANGLE/renderer/vulkan/ContextVk.h"
@@ -145,9 +146,7 @@ Error DynamicBuffer::allocate(RendererVk *renderer,
             mMappedMemory = nullptr;
         }
 
-        Serial currentSerial = renderer->getCurrentQueueSerial();
-        renderer->releaseObject(currentSerial, &mBuffer);
-        renderer->releaseObject(currentSerial, &mMemory);
+        mRetainedBuffers.emplace_back(std::move(mBuffer), std::move(mMemory));
 
         VkBufferCreateInfo createInfo;
         createInfo.sType                 = VK_STRUCTURE_TYPE_BUFFER_CREATE_INFO;
@@ -216,8 +215,28 @@ void DynamicBuffer::release(RendererVk *renderer)
     renderer->releaseObject(currentSerial, &mMemory);
 }
 
+void DynamicBuffer::releaseRetainedBuffers(RendererVk *renderer)
+{
+    for (BufferAndMemory &toFree : mRetainedBuffers)
+    {
+        Serial currentSerial = renderer->getCurrentQueueSerial();
+        renderer->releaseObject(currentSerial, &toFree.buffer);
+        renderer->releaseObject(currentSerial, &toFree.memory);
+    }
+
+    mRetainedBuffers.clear();
+}
+
 void DynamicBuffer::destroy(VkDevice device)
 {
+    for (BufferAndMemory &toFree : mRetainedBuffers)
+    {
+        toFree.buffer.destroy(device);
+        toFree.memory.destroy(device);
+    }
+
+    mRetainedBuffers.clear();
+
     mAlignment = 0;
     mBuffer.destroy(device);
     mMemory.destroy(device);
@@ -351,6 +370,8 @@ gl::Error LineLoopHelper::getIndexBufferForDrawArrays(RendererVk *renderer,
     uint32_t *indices    = nullptr;
     size_t allocateBytes = sizeof(uint32_t) * (drawCallParams.vertexCount() + 1);
     uint32_t offset      = 0;
+
+    mDynamicIndexBuffer.releaseRetainedBuffers(renderer);
     ANGLE_TRY(mDynamicIndexBuffer.allocate(renderer, allocateBytes,
                                            reinterpret_cast<uint8_t **>(&indices), bufferHandleOut,
                                            &offset, nullptr));
@@ -387,6 +408,8 @@ gl::Error LineLoopHelper::getIndexBufferForElementArrayBuffer(RendererVk *render
 
     auto unitSize = (indexType == VK_INDEX_TYPE_UINT16 ? sizeof(uint16_t) : sizeof(uint32_t));
     size_t allocateBytes = unitSize * (indexCount + 1);
+
+    mDynamicIndexBuffer.releaseRetainedBuffers(renderer);
     ANGLE_TRY(mDynamicIndexBuffer.allocate(renderer, allocateBytes,
                                            reinterpret_cast<uint8_t **>(&indices), bufferHandleOut,
                                            &destinationOffset, nullptr));

src/libANGLE/renderer/vulkan/vk_helpers.h

@@ -55,6 +55,9 @@ class DynamicBuffer : angle::NonCopyable
     // This releases resources when they might currently be in use.
     void release(RendererVk *renderer);
 
+    // This releases all the buffers that have been allocated since this was last called.
+    void releaseRetainedBuffers(RendererVk *renderer);
+
     // This frees resources immediately.
     void destroy(VkDevice device);
 
@@ -73,6 +76,8 @@ class DynamicBuffer : angle::NonCopyable
     size_t mSize;
     size_t mAlignment;
     uint8_t *mMappedMemory;
+
+    std::vector<BufferAndMemory> mRetainedBuffers;
 };
 
 // Uses DescriptorPool to allocate descriptor sets as needed. If the descriptor pool

src/libANGLE/renderer/vulkan/vk_utils.cpp

@@ -313,6 +313,25 @@ bool Error::isError() const
     return (mResult != VK_SUCCESS);
 }
 
+BufferAndMemory::BufferAndMemory() = default;
+
+BufferAndMemory::BufferAndMemory(Buffer &&buffer, DeviceMemory &&deviceMemory)
+    : buffer(std::move(buffer)), memory(std::move(deviceMemory))
+{
+}
+
+BufferAndMemory::BufferAndMemory(BufferAndMemory &&other)
+    : buffer(std::move(other.buffer)), memory(std::move(other.memory))
+{
+}
+
+BufferAndMemory &BufferAndMemory::operator=(BufferAndMemory &&other)
+{
+    buffer = std::move(other.buffer);
+    memory = std::move(other.memory);
+    return *this;
+}
+
 // CommandPool implementation.
 CommandPool::CommandPool()
 {

src/libANGLE/renderer/vulkan/vk_utils.h

@@ -640,8 +640,13 @@ Error AllocateBufferMemory(RendererVk *renderer,
                            DeviceMemory *deviceMemoryOut,
                            size_t *requiredSizeOut);
 
-struct BufferAndMemory final : private angle::NonCopyable
+struct BufferAndMemory final : angle::NonCopyable
 {
+    BufferAndMemory();
+    BufferAndMemory(Buffer &&buffer, DeviceMemory &&deviceMemory);
+    BufferAndMemory(BufferAndMemory &&other);
+    BufferAndMemory &operator=(BufferAndMemory &&other);
+
     Buffer buffer;
     DeviceMemory memory;
 };

src/tests/deqp_support/deqp_gles2_test_expectations.txt

@@ -190,7 +190,6 @@
 2161 VULKAN : dEQP-GLES2.functional.buffer.* = SKIP
 2161 VULKAN : dEQP-GLES2.functional.light_amount.* = SKIP
 2161 VULKAN : dEQP-GLES2.functional.shaders.* = SKIP
-2161 VULKAN : dEQP-GLES2.functional.texture.filtering.cube.* = SKIP
 2161 VULKAN : dEQP-GLES2.functional.texture.mipmap.2d.generate.* = SKIP
 2161 VULKAN : dEQP-GLES2.functional.texture.mipmap.cube.generate.* = SKIP
 2161 VULKAN : dEQP-GLES2.functional.texture.specification.basic_copytex* = SKIP

src/tests/gl_tests/StateChangeTest.cpp

@@ -1576,6 +1576,60 @@ TEST_P(SimpleStateChangeTest, DeleteFramebufferInUse)
     ASSERT_GL_NO_ERROR();
 }
 
+// This test was made to reproduce a specific issue with our Vulkan backend where were releasing
+// buffers too early. The test has 2 textures, we first create a texture and update it with
+// multiple updates, but we don't use it right away, we instead draw using another texture
+// then we bind the first texture and draw with it.
+TEST_P(SimpleStateChangeTest, DynamicAllocationOfMemoryForTextures)
+{
+    constexpr int kSize = 64;
+
+    GLuint program = get2DTexturedQuadProgram();
+    glUseProgram(program);
+
+    std::vector<GLColor> greenPixels(kSize * kSize, GLColor::green);
+    std::vector<GLColor> redPixels(kSize * kSize, GLColor::red);
+    GLTexture texture1;
+    glBindTexture(GL_TEXTURE_2D, texture1);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, kSize, kSize, 0, GL_RGBA, GL_UNSIGNED_BYTE, nullptr);
+
+    for (int i = 0; i < 100; i++)
+    {
+        // We do this a lot of time to make sure we use multiple buffers in the vulkan backend.
+        glTexSubImage2D(GL_TEXTURE_2D, 0, 0, 0, kSize, kSize, GL_RGBA, GL_UNSIGNED_BYTE,
+                        greenPixels.data());
+    }
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+
+    ASSERT_GL_NO_ERROR();
+
+    GLTexture texture2;
+    glBindTexture(GL_TEXTURE_2D, texture2);
+    glTexImage2D(GL_TEXTURE_2D, 0, GL_RGBA, 2, 2, 0, GL_RGBA, GL_UNSIGNED_BYTE, redPixels.data());
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
+    glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
+
+    // Setup the vertex array to draw a quad.
+    GLint positionLocation = glGetAttribLocation(program, "position");
+    setupQuadVertexBuffer(1.0f, 1.0f);
+    glVertexAttribPointer(positionLocation, 3, GL_FLOAT, GL_FALSE, 0, 0);
+    glEnableVertexAttribArray(positionLocation);
+
+    // Draw quad with texture 2 while texture 1 has "staged" changes that have not been flushed yet.
+    glBindTexture(GL_TEXTURE_2D, texture2);
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    ASSERT_GL_NO_ERROR();
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::red);
+
+    // If we now try to draw with texture1, we should trigger the issue.
+    glBindTexture(GL_TEXTURE_2D, texture1);
+    glDrawArrays(GL_TRIANGLES, 0, 6);
+    ASSERT_GL_NO_ERROR();
+
+    EXPECT_PIXEL_COLOR_EQ(0, 0, GLColor::green);
+}
+
 // Tests deleting a Framebuffer that is in use.
 TEST_P(SimpleStateChangeTest, RedefineFramebufferInUse)
 {