Fix potential memory leak in VULKAN_AcquireCommandBuffer() on error

[Sackzement]

Moving the commandBuffer NULL check above the VULKAN_INTERNAL_AcquireDescriptorSetCache() call.

VULKAN_INTERNAL_AcquireDescriptorSetCache() allocates memory or gets a descriptorSetCache from a pool and returns it, which would be unused if commandBuffer is NULL:

SDL/src/gpu/vulkan/SDL_gpu_vulkan.c

Lines 5018 to 5025 in fb1d1ab

	if (renderer->descriptorSetCachePoolCount == 0) {
	cache = SDL_malloc(sizeof(DescriptorSetCache));
	cache->poolCount = 0;
	cache->pools = NULL;
	} else {
	cache = renderer->descriptorSetCachePool[renderer->descriptorSetCachePoolCount - 1];
	renderer->descriptorSetCachePoolCount -= 1;
	}

Warning

SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:9557:16: warning: Potential leak of memory pointed to by 'descriptorSetCache' [clang-analyzer-unix.Malloc]
 9557 |         return NULL;
      |                ^
/usr/lib/clang/21/include/__stddef_null.h:26:22: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |                      ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:12993:9: note: Assuming 'result' is equal to XR_SUCCESS
 12993 |     if (result != XR_SUCCESS) return result;
       |         ^~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:12993:5: note: Taking false branch
 12993 |     if (result != XR_SUCCESS) return result;
       |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:12997:9: note: Assuming 'result' is equal to XR_SUCCESS
 12997 |     if (result != XR_SUCCESS) return result;
       |         ^~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:12997:5: note: Taking false branch
 12997 |     if (result != XR_SUCCESS) return result;
       |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13000:17: note: 'i' is < 'swapchainImageCount'
 13000 |     for (i = 0; i < swapchainImageCount; i++) swapchainImages[i].type = XR_TYPE_SWAPCHAIN_IMAGE_VULKAN2_KHR;
       |                 ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13000:5: note: Loop condition is true.  Entering loop body
 13000 |     for (i = 0; i < swapchainImageCount; i++) swapchainImages[i].type = XR_TYPE_SWAPCHAIN_IMAGE_VULKAN2_KHR;
       |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13000:17: note: Assuming 'i' is >= 'swapchainImageCount'
 13000 |     for (i = 0; i < swapchainImageCount; i++) swapchainImages[i].type = XR_TYPE_SWAPCHAIN_IMAGE_VULKAN2_KHR;
       |                 ^~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13000:5: note: Loop condition is false. Execution continues on line 13002
 13000 |     for (i = 0; i < swapchainImageCount; i++) swapchainImages[i].type = XR_TYPE_SWAPCHAIN_IMAGE_VULKAN2_KHR;
       |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13003:9: note: Assuming 'result' is equal to XR_SUCCESS
 13003 |     if (result != XR_SUCCESS) {
       |         ^~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13003:5: note: Taking false branch
 13003 |     if (result != XR_SUCCESS) {
       |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13010:26: note: 'idx' is < 'swapchainImageCount'
 13010 |     for (Uint32 idx = 0; idx < swapchainImageCount; idx++) {
       |                          ^~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13010:5: note: Loop condition is true.  Entering loop body
 13010 |     for (Uint32 idx = 0; idx < swapchainImageCount; idx++) {
       |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13028:21: note: Assuming 'i' is < field 'arraySize'
 13028 |         for (i = 0; i < createInfo.arraySize; i += 1) {
       |                     ^~~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13028:9: note: Loop condition is true.  Entering loop body
 13028 |         for (i = 0; i < createInfo.arraySize; i += 1) {
       |         ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13029:25: note: Assuming 'j' is < field 'mipCount'
 13029 |             for (j = 0; j < createInfo.mipCount; j += 1) {
       |                         ^~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13029:13: note: Loop condition is true.  Entering loop body
 13029 |             for (j = 0; j < createInfo.mipCount; j += 1) {
       |             ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13035:21: note: Assuming the condition is false
 13035 |                 if (createInfo.usageFlags & XR_SWAPCHAIN_USAGE_COLOR_ATTACHMENT_BIT) {
       |                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13035:17: note: Taking false branch
 13035 |                 if (createInfo.usageFlags & XR_SWAPCHAIN_USAGE_COLOR_ATTACHMENT_BIT) {
       |                 ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13029:25: note: Assuming 'j' is >= field 'mipCount'
 13029 |             for (j = 0; j < createInfo.mipCount; j += 1) {
       |                         ^~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13029:13: note: Loop condition is false. Execution continues on line 13028
 13029 |             for (j = 0; j < createInfo.mipCount; j += 1) {
       |             ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13028:21: note: Assuming 'i' is >= field 'arraySize'
 13028 |         for (i = 0; i < createInfo.arraySize; i += 1) {
       |                     ^~~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13028:9: note: Loop condition is false. Execution continues on line 13059
 13028 |         for (i = 0; i < createInfo.arraySize; i += 1) {
       |         ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:13059:76: note: Calling 'VULKAN_AcquireCommandBuffer'
 13059 |         VulkanCommandBuffer *barrierCommandBuffer = (VulkanCommandBuffer *)VULKAN_AcquireCommandBuffer((SDL_GPURenderer *)renderer);
       |                                                                            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:9552:9: note: Calling 'VULKAN_INTERNAL_AcquireDescriptorSetCache'
 9552 |         VULKAN_INTERNAL_AcquireDescriptorSetCache(renderer);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:5018:9: note: Assuming field 'descriptorSetCachePoolCount' is equal to 0
 5018 |     if (renderer->descriptorSetCachePoolCount == 0) {
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:5018:5: note: Taking true branch
 5018 |     if (renderer->descriptorSetCachePoolCount == 0) {
      |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:5019:17: note: Memory is allocated
 5019 |         cache = SDL_malloc(sizeof(DescriptorSetCache));
      |                 ^
SDL/include/SDL3/SDL_stdinc.h:6037:20: note: expanded from macro 'SDL_malloc'
 6037 | #define SDL_malloc malloc
      |                    ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:9552:9: note: Returned allocated memory
 9552 |         VULKAN_INTERNAL_AcquireDescriptorSetCache(renderer);
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:9556:9: note: 'commandBuffer' is equal to NULL
 9556 |     if (commandBuffer == NULL) {
      |         ^~~~~~~~~~~~~
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:9556:5: note: Taking true branch
 9556 |     if (commandBuffer == NULL) {
      |     ^
SDL/src/gpu/vulkan/SDL_gpu_vulkan.c:9557:16: note: Potential leak of memory pointed to by 'descriptorSetCache'
 9557 |         return NULL;
      |                ^
/usr/lib/clang/21/include/__stddef_null.h:26:22: note: expanded from macro 'NULL'
   26 | #define NULL ((void*)0)
      |                      ^

[slouken]

It's a good practice to check for allocation results immediately after allocation, but in this case I think checking after the natural unlock is harmless. @flibitijibibo, thoughts?

[Sackzement]

It's a good practice to check for allocation results immediately after allocation, but in this case I think checking after the natural unlock is harmless. @flibitijibibo, thoughts?

If the check is done after the natural unlock and commandBuffer is NULL, then the memory that was allocated inside VULKAN_INTERNAL_AcquireDescriptorSetCache() or the pool index which was advanced, would need to be freed/reverted to not have a leak / a marked but unused index.
Currently VULKAN_AcquireCommandBuffer() just returns NULL without freeing/reverting anything.

[flibitijibibo]

This makes sense - the only real functional change is preventing a leak, and the normal scenario won't behave any differently here.

[slouken]

Merged, thanks!