From cb012d7068cf5e0effa23fc7c7194c1e6feb32b8 Mon Sep 17 00:00:00 2001 From: catbot Date: Wed, 17 Jun 2026 13:21:12 +0000 Subject: [PATCH] fix(device): fence-keyed deferred resource-deletion queue (#101) Since #40 dropped the per-frame vkQueueWaitIdle, frames are pipelined: a resource the CPU is done with may still be read by the GPU for up to numFrames-1 more frames. VulkanBuffer::Resize's destroy-and-recreate path was therefore a live use-after-free (#63), no longer masked by the wait-idle. Device gains a monotonic, frame-counter-keyed deletion queue: EnqueueDeletion tags {buffer, memory} with the current frameCounter; ReclaimDeletions (called per frame after the fence wait) frees entries once framesInFlight frames have elapsed; DrainDeletions frees everything after a wait-idle. VulkanBuffer::DeferredClear hands handles to the queue and nulls the handle, and Resize uses it instead of immediate Clear(). Window::Render sets framesInFlight at init, reclaims after the per-image fence wait, bumps Device::frameCounter once per frame, and drains on the resize / OUT_OF_DATE wait-idle paths. The destructor keeps immediate Clear() (callers destroying mid-flight remain responsible, unchanged). Adds the DeferredDeletion test: drives the retire timing on a real headless device with real buffers, stepping Device::frameCounter to pin the exact reclaim frame, asserting validation stays silent. Co-Authored-By: Claude Opus 4.8 --- implementations/Crafter.Graphics-Device.cpp | 35 ++++ implementations/Crafter.Graphics-Window.cpp | 20 ++ interfaces/Crafter.Graphics-Device.cppm | 38 ++++ interfaces/Crafter.Graphics-VulkanBuffer.cppm | 18 +- project.cpp | 31 ++++ tests/DeferredDeletion/main.cpp | 171 ++++++++++++++++++ 6 files changed, 312 insertions(+), 1 deletion(-) create mode 100644 tests/DeferredDeletion/main.cpp diff --git a/implementations/Crafter.Graphics-Device.cpp b/implementations/Crafter.Graphics-Device.cpp index cb09fe5..f96ddc8 100644 --- a/implementations/Crafter.Graphics-Device.cpp +++ b/implementations/Crafter.Graphics-Device.cpp @@ -1000,4 +1000,39 @@ bool Device::PreferDirectDeviceWrite(VkDeviceSize size) { // directWriteBudget caches the size-independent decision (CacheUploadStrategy): // 0 -> never direct, max -> ReBAR/UMA so any size, else the small-window cap. return directWriteBudget != 0 && size <= directWriteBudget; +} + +void Device::EnqueueDeletion(VkBuffer buffer, VkDeviceMemory memory) { + // Nothing to free for an already-null handle — avoids parking dead + // entries that ReclaimDeletions would have to skip over. + if (buffer == VK_NULL_HANDLE) return; + deletionQueue.push_back({ frameCounter, buffer, memory }); +} + +void Device::ReclaimDeletions() { + // Free every entry whose retire frame has been reached, compacting the + // survivors down in place. An entry tagged at frame F retires at + // F + framesInFlight: by the time the CPU has begun that many later + // frames (each gated by a per-image fence wait), single-queue submission + // order guarantees all GPU work from frame F is complete. + std::size_t kept = 0; + for (PendingDeletion& entry : deletionQueue) { + if (entry.retireAfter + framesInFlight <= frameCounter) { + vkDestroyBuffer(device, entry.buffer, nullptr); + vkFreeMemory(device, entry.memory, nullptr); + } else { + deletionQueue[kept++] = entry; + } + } + deletionQueue.resize(kept); +} + +void Device::DrainDeletions() { + // Unconditional: the caller has just wait-idled, so no in-flight GPU work + // can still reference any queued resource. + for (PendingDeletion& entry : deletionQueue) { + vkDestroyBuffer(device, entry.buffer, nullptr); + vkFreeMemory(device, entry.memory, nullptr); + } + deletionQueue.clear(); } \ No newline at end of file diff --git a/implementations/Crafter.Graphics-Window.cpp b/implementations/Crafter.Graphics-Window.cpp index 5138839..0afe9d8 100644 --- a/implementations/Crafter.Graphics-Window.cpp +++ b/implementations/Crafter.Graphics-Window.cpp @@ -452,6 +452,11 @@ Window::Window(std::uint32_t width, std::uint32_t height) : width(width), height Device::CheckVkResult(vkCreateSemaphore(Device::device, &semaphoreCreateInfo, nullptr, &imageAcquiredSemaphores[i])); } + // The deferred-deletion queue (issue #101) retires a resource after this + // many recorded frames — exactly the depth the per-frame fences (#40) + // allow in flight. Set it here, where numFrames is in scope. + Device::framesInFlight = numFrames; + // Per-frame info structs: everything that never varies between frames is // set here once. Render() patches the barriers' image/oldLayout and the // sync handles that follow currentBuffer/frameCounter (submitInfo's @@ -542,6 +547,9 @@ void Window::Resize(std::uint32_t newWidth, std::uint32_t newHeight) { // Caller (configure handler / WM_SIZE) runs between frames, but be // defensive: ensure no in-flight commands reference the old swapchain. Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + // The queue is idle, so every deferred resource is now safe to free + // regardless of its retire frame (issue #101). + Device::DrainDeletions(); #ifdef CRAFTER_GRAPHICS_WINDOW_WAYLAND if (wpViewport) { @@ -768,6 +776,7 @@ void Window::Render() { imageAcquired, (VkFence)nullptr, ¤tBuffer); if (acquire == VK_ERROR_OUT_OF_DATE_KHR) { Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + Device::DrainDeletions(); RecreateSwapchainAndImages(); onResize.Invoke(); acquire = vkAcquireNextImageKHR(Device::device, swapChain, UINT64_MAX, @@ -786,6 +795,12 @@ void Window::Render() { Device::CheckVkResult(vkWaitForFences(Device::device, 1, &waitFences[currentBuffer], VK_TRUE, UINT64_MAX)); Device::CheckVkResult(vkResetFences(Device::device, 1, &waitFences[currentBuffer])); + // Now that a frame's fence has been waited, free any deferred resources + // whose retire frame has been reached (issue #101). frameCounter is bumped + // at end of frame, so it still holds this frame's index here — exactly the + // value entries enqueued this frame are tagged with. + Device::ReclaimDeletions(); + Device::CheckVkResult(vkBeginCommandBuffer(drawCmdBuffers[currentBuffer], &cmdBufInfo)); // On an image's first use after (re)creating the swapchain it is still in @@ -902,6 +917,7 @@ void Window::Render() { // frame. The wait-idle here also re-settles every per-frame fence to a // signaled state, so the next Render()'s fence wait passes through. Device::CheckVkResult(vkQueueWaitIdle(Device::queue)); + Device::DrainDeletions(); RecreateSwapchainAndImages(); onResize.Invoke(); } else { @@ -911,6 +927,10 @@ void Window::Render() { // command-buffer reuse, so the CPU is free to acquire/record frame N+1 // while the GPU is still executing frame N. frameCounter++; + // Advance the global deferred-deletion clock too (issue #101). Kept + // separate from the per-window frameCounter above (which also keys the + // acquire-semaphore slot) so non-window code can enqueue against it. + Device::frameCounter++; } #ifdef CRAFTER_TIMING diff --git a/interfaces/Crafter.Graphics-Device.cppm b/interfaces/Crafter.Graphics-Device.cppm index e7530b9..95f7583 100644 --- a/interfaces/Crafter.Graphics-Device.cppm +++ b/interfaces/Crafter.Graphics-Device.cppm @@ -294,6 +294,44 @@ export namespace Crafter { inline static VkDeviceSize directWriteBudget = 0; static void CacheUploadStrategy(); + // ─── Fence-keyed deferred resource deletion (issue #101) ──────── + // Since #40 dropped the per-frame vkQueueWaitIdle, frames are + // pipelined: a buffer the CPU is "done" with may still be read by the + // GPU for up to framesInFlight-1 more frames. Destroying it + // immediately (as Clear() does) is then a use-after-free. Callers that + // free a buffer which may still be in flight — VulkanBuffer::Resize's + // reallocate path, VulkanBuffer::DeferredClear — hand the handles here + // instead, tagged with the current frameCounter; ReclaimDeletions frees + // an entry only once framesInFlight frames have elapsed, by which point + // single-queue submission order guarantees all GPU work that could + // reference it has completed. + struct PendingDeletion { + std::uint64_t retireAfter; // frameCounter value at enqueue time + VkBuffer buffer; + VkDeviceMemory memory; + }; + // Monotonic recorded-frame index, bumped once per Window::Render. Not + // the same as Window::frameCounter (which is per-window and also drives + // the acquire-semaphore slot) — this one is global so Mesh/Device code + // can enqueue too, not just the window loop. + inline static std::uint64_t frameCounter = 0; + // = Window::numFrames; set at window init. The number of frames that + // must elapse after an enqueue before the resource is safe to free. + // Zero until set, which only makes ReclaimDeletions more eager — and it + // is never called before a Window (which sets this) starts rendering. + inline static std::uint8_t framesInFlight = 0; + inline static std::vector deletionQueue; + // Tag {buffer, memory} for deletion after framesInFlight more frames. + // A VK_NULL_HANDLE buffer is ignored (nothing to free). + static void EnqueueDeletion(VkBuffer buffer, VkDeviceMemory memory); + // Free every entry whose retire frame has been reached + // (retireAfter + framesInFlight <= frameCounter). Call once per frame + // after waiting that frame's fence. + static void ReclaimDeletions(); + // Free every queued entry unconditionally. Call only after a wait-idle + // (resize / teardown), when no in-flight GPU work can reference them. + static void DrainDeletions(); + // ─── Wayland key repeat ──────────────────────────────────────── // TickKeyRepeats fires onRawKeyDown / onRawKeyHold / onTextInput on // the focused window for whichever key is currently repeating. diff --git a/interfaces/Crafter.Graphics-VulkanBuffer.cppm b/interfaces/Crafter.Graphics-VulkanBuffer.cppm index a130c1d..a499062 100644 --- a/interfaces/Crafter.Graphics-VulkanBuffer.cppm +++ b/interfaces/Crafter.Graphics-VulkanBuffer.cppm @@ -155,6 +155,18 @@ namespace Crafter { buffer = VK_NULL_HANDLE; } + // Like Clear(), but hands the destroy+free to Device's fence-keyed + // deletion queue instead of doing it immediately. Use when the buffer + // may still be read by an in-flight frame's GPU work — destroying it + // now would be a use-after-free, since frames are pipelined up to + // framesInFlight deep (issue #101). The handle is nulled immediately so + // this VulkanBuffer no longer owns it; vkFreeMemory (deferred) implicitly + // unmaps mapped memory, so no explicit vkUnmapMemory is needed here. + void DeferredClear() { + Device::EnqueueDeletion(buffer, memory); + buffer = VK_NULL_HANDLE; + } + void Resize(VkBufferUsageFlags2 usageFlags, VkMemoryPropertyFlags memoryPropertyFlags, std::uint32_t count, VkMemoryPropertyFlags preferredPropertyFlags = 0) { // Reuse the existing allocation in place when the request still fits // and the fixed-at-create properties match: usage flags are @@ -172,8 +184,12 @@ namespace Crafter { size = requestedSize; return; } + // Defer the old allocation's destruction: an in-flight frame may + // still reference it (the #63 hazard, no longer masked by a + // per-frame wait-idle since #40). DeferredClear nulls the handle, + // so the Create below starts from a clean slate. if(buffer != VK_NULL_HANDLE) { - Clear(); + DeferredClear(); } Create(usageFlags, memoryPropertyFlags, count, preferredPropertyFlags); } diff --git a/project.cpp b/project.cpp index a08ce34..879108f 100644 --- a/project.cpp +++ b/project.cpp @@ -586,6 +586,37 @@ extern "C" Configuration CrafterBuildProject(std::span a rc.GetInterfacesAndImplementations(ifaces, resizeImpls); cfg.tests.push_back(std::move(resizeTest)); + // Issue #101: fence-keyed deferred resource-deletion queue. Since #40 + // dropped the per-frame wait-idle, destroying a buffer the GPU may + // still read (Resize's reallocate path) is a use-after-free. + // VulkanBuffer::DeferredClear / Resize now hand handles to Device's + // queue, which ReclaimDeletions frees only after framesInFlight frames + // and DrainDeletions frees on a wait-idle. The retire timing is driven + // on a real headless device (real buffers so the frees execute and the + // validation layer can object) by stepping Device::frameCounter — no + // swapchain/window needed, so it shares the native build settings. + Test deferredTest; + Configuration& dc = deferredTest.config; + dc.path = cfg.path; + dc.name = "DeferredDeletion"; + dc.outputName = "DeferredDeletion"; + dc.type = ConfigurationType::Executable; + dc.target = cfg.target; + dc.march = cfg.march; + dc.mtune = cfg.mtune; + dc.debug = cfg.debug; + dc.sysroot = cfg.sysroot; + dc.dependencies = cfg.dependencies; + dc.externalDependencies = cfg.externalDependencies; + dc.compileFlags = cfg.compileFlags; + dc.linkFlags = cfg.linkFlags; + dc.defines = cfg.defines; + dc.cFiles = cfg.cFiles; + std::vector deferredImpls(impls.begin(), impls.end()); + deferredImpls.emplace_back("tests/DeferredDeletion/main"); + dc.GetInterfacesAndImplementations(ifaces, deferredImpls); + cfg.tests.push_back(std::move(deferredTest)); + // Issue #89: Device::PreferDirectDeviceWrite chooses the upload strategy // for a CPU-written, GPU-read buffer — direct HOST_VISIBLE|DEVICE_LOCAL // map+write on ReBAR/UMA vs. staged-into-pure-DEVICE_LOCAL on a small diff --git a/tests/DeferredDeletion/main.cpp b/tests/DeferredDeletion/main.cpp new file mode 100644 index 0000000..49e8b18 --- /dev/null +++ b/tests/DeferredDeletion/main.cpp @@ -0,0 +1,171 @@ +/* +Crafter®.Graphics +Copyright (C) 2026 Catcrafts® +catcrafts.net + +This library is free software; you can redistribute it and/or +modify it under the terms of the GNU Lesser General Public +License version 3.0 as published by the Free Software Foundation; + +This library is distributed in the hope that it will be useful, +but WITHOUT ANY WARRANTY; without even the implied warranty of +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +Lesser General Public License for more details. + +You should have received a copy of the GNU Lesser General Public +License along with this library; if not, write to the Free Software +Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA +*/ + +// Issue #101: fence-keyed deferred resource-deletion queue. Since #40 dropped +// the per-frame vkQueueWaitIdle, a buffer the CPU is done with may still be +// read by the GPU for up to framesInFlight-1 more frames — so destroying it +// immediately (VulkanBuffer::Clear / the old Resize path) is a use-after-free. +// VulkanBuffer::DeferredClear() / Resize() now hand the handles to Device's +// deletion queue, tagged with the current frameCounter; ReclaimDeletions frees +// an entry only once framesInFlight frames have elapsed, and DrainDeletions +// (called after a wait-idle) frees everything unconditionally. +// +// The retire-timing is the load-bearing logic: free too early and it's the +// very UAF the queue exists to prevent; free too late and resources leak. This +// test drives it on a real headless Vulkan device (real VkBuffer/VkDeviceMemory +// so the frees actually execute and the validation layer can object), manually +// stepping Device::frameCounter to pin down the exact frame an entry is +// reclaimed on. It needs no swapchain/window — the queue itself is window-free. + +#include "vulkan/vulkan.h" +#include + +import Crafter.Graphics; +import std; + +using namespace Crafter; + +namespace { + +int failures = 0; + +void Check(bool ok, std::string_view what) { + std::println("{} {}", ok ? "PASS" : "FAIL", what); + if (!ok) ++failures; +} + +constexpr VkBufferUsageFlags2 USAGE = + VK_BUFFER_USAGE_2_TRANSFER_SRC_BIT | VK_BUFFER_USAGE_2_SHADER_DEVICE_ADDRESS_BIT; +constexpr auto DEVICE_LOCAL = VK_MEMORY_PROPERTY_DEVICE_LOCAL_BIT; + +} // namespace + +int main() { + Device::Initialize(); + + // No Window here, so set the in-flight depth ourselves (Window::Render + // normally does this at init). Use the real window depth so the timing + // matches production. + Device::framesInFlight = Window::numFrames; + Device::frameCounter = 0; + Device::deletionQueue.clear(); + + // ── DeferredClear enqueues, nulls the handle, and retires after exactly + // framesInFlight frames ────────────────────────────────────────────── + { + VulkanBuffer buf; + buf.Create(USAGE, DEVICE_LOCAL, 16); + Check(buf.buffer != VK_NULL_HANDLE, "Create produced a real buffer handle"); + + buf.DeferredClear(); + Check(buf.buffer == VK_NULL_HANDLE, + "DeferredClear nulls the handle so the buffer no longer owns it"); + Check(Device::deletionQueue.size() == 1, + "DeferredClear enqueues exactly one pending deletion"); + + // Walk right up to the retire frame: still queued at every frame before + // frameCounter reaches retireAfter + framesInFlight. + for (std::uint64_t f = 0; f < Window::numFrames; ++f) { + Device::frameCounter = f; + Device::ReclaimDeletions(); + Check(Device::deletionQueue.size() == 1, + std::format("entry survives ReclaimDeletions at frame {} " + "(< retire frame {})", f, Window::numFrames)); + } + + // At retireAfter(0) + framesInFlight it is finally freed. + Device::frameCounter = Window::numFrames; + Device::ReclaimDeletions(); + Check(Device::deletionQueue.empty(), + std::format("entry is reclaimed once frameCounter reaches {}", + Window::numFrames)); + } + + // ── Resize's reallocate path defers the OLD allocation, not the new one ── + { + Device::frameCounter = 100; + Device::deletionQueue.clear(); + + VulkanBuffer buf; + buf.Create(USAGE, DEVICE_LOCAL, 4); + VkBuffer original = buf.buffer; + + // Grow past capacity → forces destroy+recreate, which must DEFER the old + // buffer (the #63 UAF this whole queue exists to close). + buf.Resize(USAGE, DEVICE_LOCAL, 64); + Check(buf.buffer != VK_NULL_HANDLE && buf.buffer != original, + "Resize past capacity allocates a fresh buffer"); + Check(Device::deletionQueue.size() == 1 + && Device::deletionQueue.front().buffer == original, + "Resize defers the OLD allocation to the deletion queue"); + Check(Device::deletionQueue.front().retireAfter == 100, + "deferred entry is tagged with the frameCounter at enqueue time"); + + // Not yet due (tagged at 100, retires at 100 + framesInFlight). + Device::frameCounter = 100 + Window::numFrames - 1; + Device::ReclaimDeletions(); + Check(Device::deletionQueue.size() == 1, + "old allocation is not freed before its retire frame"); + + Device::frameCounter = 100 + Window::numFrames; + Device::ReclaimDeletions(); + Check(Device::deletionQueue.empty(), + "old allocation is freed on its retire frame"); + + buf.Clear(); // immediate cleanup of the live buffer (no GPU work pending) + } + + // ── DrainDeletions frees everything regardless of retire frame ─────────── + { + Device::frameCounter = 0; + Device::deletionQueue.clear(); + + VulkanBuffer a; + VulkanBuffer b; + a.Create(USAGE, DEVICE_LOCAL, 8); + b.Create(USAGE, DEVICE_LOCAL, 8); + a.DeferredClear(); + b.DeferredClear(); + Check(Device::deletionQueue.size() == 2, "two entries queued"); + + // Far from any retire frame — ReclaimDeletions would free nothing here. + Device::DrainDeletions(); + Check(Device::deletionQueue.empty(), + "DrainDeletions frees every queued entry unconditionally"); + } + + // ── EnqueueDeletion ignores a null handle (nothing to free) ────────────── + { + Device::deletionQueue.clear(); + Device::EnqueueDeletion(VK_NULL_HANDLE, VK_NULL_HANDLE); + Check(Device::deletionQueue.empty(), + "EnqueueDeletion drops a VK_NULL_HANDLE buffer"); + } + + Check(Device::validationErrorCount == 0, + std::format("no Vulkan validation errors across the run ({} seen)", + Device::validationErrorCount)); + + if (failures != 0) { + std::println("{} check(s) failed", failures); + return EXIT_FAILURE; + } + std::println("all checks passed"); + return EXIT_SUCCESS; +} -- 2.54.0