Skip to content

Commit 6fd1b13

Browse files
committed
gfx/vulkan: fix VkDeviceMemory leak + spec violations in create/destroy paths
Four issues found in the same audit pass over gfx/drivers/vulkan.c, all in the create-with-old / destroy-buffer ownership protocol. None of them are the trigger for the iOS materialui content-close abort that prompted the audit (the font teardown does not go through the patched paths), but they are real and worth fixing on their own. * vulkan_create_texture: VkDeviceMemory leak on alloc failure (gfx/drivers/vulkan.c) When called with a non-NULL `old`, the function destroys old's view/image/buffer at the top of the `if (old)` block, then either pilfers old->memory or allocates fresh memory for tex.memory. The trailing `if (old)` cleanup is responsible for freeing old->memory in the no-pilfer path (skipped by the pilfer branch via old->memory = VK_NULL_HANDLE). The vkAllocateMemory failure branch returns early before reaching that cleanup. At return, old->view/image/buffer are dangling but zeroed by the caller's `*old = tex` overwrite, while old->memory still holds the original live VkDeviceMemory and is now unreachable. Pure leak per failed call; reachable from every call site that passes a real `old` (vk_per_frame texture realloc on resize, MUI/RGUI set_texture, readback staging realloc). Inline an equivalent cleanup before the early return: unmap if mapped, free old->memory, zero old. Mirrors the unmap-before-free the pilfer branch already does on line 1277. * vulkan_create_texture: free-while-mapped on no-pilfer cleanup (gfx/drivers/vulkan.c) The trailing `if (old)` cleanup was calling vkFreeMemory on old->memory without unmapping first, even though the pilfer branch a few lines up does unmap. vkFreeMemory implicitly unmaps so this is not a spec violation, but MoltenVK has historically handled free-while-mapped poorly (KhronosGroup/MoltenVK#957) and the asymmetry with the pilfer branch is gratuitous. Add the `if (old->mapped) vkUnmapMemory(...)` guard before the free. * vulkan_destroy_buffer: VUID-vkFreeMemory-memory-00677 violation (gfx/drivers/vulkan.c) Order was: vkUnmapMemory (unconditional) -> vkFreeMemory -> vkDestroyBuffer. Two problems. First, the memory is freed while buffer->buffer is still bound to it, which the spec forbids per VUID-vkFreeMemory-memory-00677 -- vulkan_destroy_texture (line 974) gets this right; this function was the outlier. Second, vkUnmapMemory was unconditional, but vulkan_create_buffer leaves buffer->mapped == NULL when its initial vkMapMemory fails, so a destroy on a never-mapped buffer would unmap memory that is not currently mapped. Fix the order to unmap (gated on buffer->mapped) -> destroy buffer -> free memory, with NULL-handle guards on the destroys for symmetry with vulkan_destroy_texture. * vulkan_init_quad_ibo: same VUID violation in vkMapMemory failure path (gfx/drivers/vulkan.c) Identical wrong order in the IBO-init map-failure cleanup -- frees vk->quad_ibo.memory before destroying vk->quad_ibo.buffer that is still bound to it. Swap the two calls. vulkan_deinit_quad_ibo itself was already correct.
1 parent 46ba4c1 commit 6fd1b13

1 file changed

Lines changed: 40 additions & 4 deletions

File tree

gfx/drivers/vulkan.c

Lines changed: 40 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1289,6 +1289,20 @@ static struct vk_texture vulkan_create_texture(vk_t *vk,
12891289
vkDestroyImage(device, tex.image, NULL);
12901290
if (tex.buffer)
12911291
vkDestroyBuffer(device, tex.buffer, NULL);
1292+
/* The `if (old)` cleanup further below is skipped by this
1293+
* early return, but old->view/image/buffer were already
1294+
* destroyed at the top of the `if (old)` block above and
1295+
* old->memory still owns a live VkDeviceMemory. Free it
1296+
* here, otherwise it leaks across the call. Mirrors the
1297+
* unmap-before-free done in the pilfer branch. */
1298+
if (old)
1299+
{
1300+
if (old->mapped)
1301+
vkUnmapMemory(device, old->memory);
1302+
if (old->memory != VK_NULL_HANDLE)
1303+
vkFreeMemory(device, old->memory, NULL);
1304+
memset(old, 0, sizeof(*old));
1305+
}
12921306
memset(&tex, 0, sizeof(tex));
12931307
return tex;
12941308
}
@@ -1299,8 +1313,18 @@ static struct vk_texture vulkan_create_texture(vk_t *vk,
12991313

13001314
if (old)
13011315
{
1316+
/* old->view/image/buffer were already destroyed at the top of
1317+
* the `if (old)` block above. In the no-pilfer path we did not
1318+
* take ownership of old->memory, so it still owns a live
1319+
* VkDeviceMemory; free it here. Unmap first to mirror the
1320+
* pilfer branch and to avoid free-while-mapped, which the spec
1321+
* permits but MoltenVK has historically handled poorly. */
13021322
if (old->memory != VK_NULL_HANDLE)
1323+
{
1324+
if (old->mapped)
1325+
vkUnmapMemory(device, old->memory);
13031326
vkFreeMemory(device, old->memory, NULL);
1327+
}
13041328
memset(old, 0, sizeof(*old));
13051329
}
13061330

@@ -3867,10 +3891,20 @@ static void vulkan_init_samplers(vk_t *vk)
38673891

38683892
static void vulkan_destroy_buffer(VkDevice device, struct vk_buffer *buffer)
38693893
{
3870-
vkUnmapMemory(device, buffer->memory);
3871-
vkFreeMemory(device, buffer->memory, NULL);
3894+
/* Order: unmap (only if mapped) -> destroy buffer -> free memory.
3895+
* vkFreeMemory must not be called while a VkBuffer is still bound
3896+
* to the memory (VUID-vkFreeMemory-memory-00677), and vkUnmapMemory
3897+
* must not be called on memory that is not currently mapped.
3898+
* vulkan_create_buffer leaves buffer->mapped == NULL when the
3899+
* initial vkMapMemory fails, so the mapped check is required. */
3900+
if (buffer->mapped)
3901+
vkUnmapMemory(device, buffer->memory);
38723902

3873-
vkDestroyBuffer(device, buffer->buffer, NULL);
3903+
if (buffer->buffer != VK_NULL_HANDLE)
3904+
vkDestroyBuffer(device, buffer->buffer, NULL);
3905+
3906+
if (buffer->memory != VK_NULL_HANDLE)
3907+
vkFreeMemory(device, buffer->memory, NULL);
38743908

38753909
memset(buffer, 0, sizeof(*buffer));
38763910
}
@@ -4483,8 +4517,10 @@ static void vulkan_init_quad_ibo(vk_t *vk, unsigned max_quads)
44834517
if (res != VK_SUCCESS || !mapped)
44844518
{
44854519
RARCH_ERR("[Vulkan] Failed to map quad IBO memory (VkResult: %d).\n", res);
4486-
vkFreeMemory(device, vk->quad_ibo.memory, NULL);
4520+
/* Destroy the buffer before freeing the memory it is bound to
4521+
* (VUID-vkFreeMemory-memory-00677). */
44874522
vkDestroyBuffer(device, vk->quad_ibo.buffer, NULL);
4523+
vkFreeMemory(device, vk->quad_ibo.memory, NULL);
44884524
vk->quad_ibo.buffer = VK_NULL_HANDLE;
44894525
vk->quad_ibo.memory = VK_NULL_HANDLE;
44904526
vk->quad_ibo.num_quads = 0;

0 commit comments

Comments
 (0)