[infra] Fence-keyed deferred resource-deletion queue (UAF since #40; unblocks #63/#66) #101

New issue

Closed

opened 2026-06-16 20:47:49 +02:00 by jorijnvdgraaf · 0 comments

jorijnvdgraaf commented

2026-06-16 20:47:49 +02:00

Owner

Subsystem: Device / VulkanBuffer — resource lifetime
Impact: Correctness (live UAF since #40 landed) + enabler · Effort: Medium
Prerequisite-for / unblocks: #63 (Resize destroys old buffer immediately), #66 (scratch release), #62/#67 (pool recycling)

Problem

Since #40 (per-frame fences, no per-frame vkQueueWaitIdle) landed, frames are pipelined: a resource the CPU is "done" with may still be read by the GPU for up to numFrames-1 more frames. But VulkanBuffer::Clear()/Resize() (interfaces/Crafter.Graphics-VulkanBuffer.cppm:123-153) destroy + free immediately. Resize's destroy-and-recreate path is now a live use-after-free whenever an in-flight frame still references the old buffer (the #63 hazard, no longer masked by wait-idle).

Fix — fence-keyed deferred deletion queue

Device (all-inline static singleton) gains:

struct PendingDeletion { std::uint64_t retireAfter; VkBuffer buffer; VkDeviceMemory memory; };
inline static std::uint64_t frameCounter = 0;   // monotonic recorded-frame index
inline static std::uint8_t  framesInFlight = 0;  // = Window::numFrames, set at init
inline static std::vector<PendingDeletion> deletionQueue;
static void EnqueueDeletion(VkBuffer, VkDeviceMemory);  // tags with frameCounter
static void ReclaimDeletions();   // frees entries where retireAfter + framesInFlight <= frameCounter
static void DrainDeletions();     // frees all; call after wait-idle / teardown

Window::Render: after the existing vkWaitForFences(waitFences[currentBuffer]) (Window.cpp:786) call Device::ReclaimDeletions(); bump frameCounter once per frame; set framesInFlight at init; call DrainDeletions() on the resize/teardown wait-idle paths.
VulkanBuffer::DeferredClear() hands {buffer, memory} to EnqueueDeletion and nulls the handle (mapped memory is implicitly unmapped by vkFreeMemory). Resize() uses DeferredClear() instead of Clear().
Monotonic keying (not per-slot buckets) so Mesh/Device code can enqueue too, not just the Window loop.

Decisions

Destructor stays immediate (~VulkanBuffer keeps Clear()); only Resize() + explicit DeferredClear() defer. Callers destroying objects mid-flight remain responsible (unchanged from today).
First change scope: the queue + reclaim hook + DeferredClear, and convert Resize to use it (closes #63). #66 scratch release is a follow-up on top.

Correctness

An entry tagged at frame F is freed only once the fence for frame F has been waited (at F+framesInFlight); single-queue submission order then guarantees all <=F GPU work is complete. The one-shot immediate submits that still vkQueueWaitIdle (Window.cpp:1077/1102/1267) are synchronous and may keep immediate Clear().

**Subsystem:** Device / VulkanBuffer — resource lifetime **Impact:** Correctness (live UAF since #40 landed) + enabler · **Effort:** Medium **Prerequisite-for / unblocks:** #63 (Resize destroys old buffer immediately), #66 (scratch release), #62/#67 (pool recycling) ### Problem Since #40 (per-frame fences, no per-frame `vkQueueWaitIdle`) landed, frames are pipelined: a resource the CPU is "done" with may still be read by the GPU for up to `numFrames-1` more frames. But `VulkanBuffer::Clear()`/`Resize()` (`interfaces/Crafter.Graphics-VulkanBuffer.cppm:123-153`) destroy + free **immediately**. `Resize`'s destroy-and-recreate path is now a live use-after-free whenever an in-flight frame still references the old buffer (the #63 hazard, no longer masked by wait-idle). ### Fix — fence-keyed deferred deletion queue `Device` (all-`inline static` singleton) gains: ```cpp struct PendingDeletion { std::uint64_t retireAfter; VkBuffer buffer; VkDeviceMemory memory; }; inline static std::uint64_t frameCounter = 0; // monotonic recorded-frame index inline static std::uint8_t framesInFlight = 0; // = Window::numFrames, set at init inline static std::vector<PendingDeletion> deletionQueue; static void EnqueueDeletion(VkBuffer, VkDeviceMemory); // tags with frameCounter static void ReclaimDeletions(); // frees entries where retireAfter + framesInFlight <= frameCounter static void DrainDeletions(); // frees all; call after wait-idle / teardown ``` - `Window::Render`: after the existing `vkWaitForFences(waitFences[currentBuffer])` (Window.cpp:786) call `Device::ReclaimDeletions()`; bump `frameCounter` once per frame; set `framesInFlight` at init; call `DrainDeletions()` on the resize/teardown wait-idle paths. - `VulkanBuffer::DeferredClear()` hands `{buffer, memory}` to `EnqueueDeletion` and nulls the handle (mapped memory is implicitly unmapped by `vkFreeMemory`). `Resize()` uses `DeferredClear()` instead of `Clear()`. - Monotonic keying (not per-slot buckets) so Mesh/Device code can enqueue too, not just the Window loop. ### Decisions - **Destructor stays immediate** (`~VulkanBuffer` keeps `Clear()`); only `Resize()` + explicit `DeferredClear()` defer. Callers destroying objects mid-flight remain responsible (unchanged from today). - **First change scope:** the queue + reclaim hook + `DeferredClear`, and convert `Resize` to use it (closes #63). #66 scratch release is a follow-up on top. ### Correctness An entry tagged at frame F is freed only once the fence for frame F has been waited (at F+framesInFlight); single-queue submission order then guarantees all <=F GPU work is complete. The one-shot immediate submits that still `vkQueueWaitIdle` (Window.cpp:1077/1102/1267) are synchronous and may keep immediate `Clear()`.

jorijnvdgraaf added the

claude:ready

label

2026-06-17 15:10:48 +02:00