From d6cfb20a4e373b9eb48e0058f93d3f76c546c622 Mon Sep 17 00:00:00 2001 From: Aizal Khan Date: Wed, 8 Jul 2026 18:55:04 +0530 Subject: [PATCH] gate debug create rollback icd destroy loop on reserved index --- loader/debug_utils.c | 4 +- tests/loader_alloc_callback_tests.cpp | 75 +++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 2 deletions(-) diff --git a/loader/debug_utils.c b/loader/debug_utils.c index dcefaefdc..0af7dd377 100644 --- a/loader/debug_utils.c +++ b/loader/debug_utils.c @@ -238,7 +238,7 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDebugUtilsMessengerEXT(VkInstanc // Roll back on errors if (VK_SUCCESS != res) { - if (pNextIndex) { + if (index_reserved) { for (struct loader_icd_term *icd_term = inst->icd_terms; icd_term; icd_term = icd_term->next) { if (icd_term->debug_utils_messenger_list.list && icd_term->debug_utils_messenger_list.list[next_index] && NULL != icd_term->dispatch.DestroyDebugUtilsMessengerEXT) { @@ -518,7 +518,7 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateDebugReportCallbackEXT(VkInstanc // Roll back on errors if (VK_SUCCESS != res) { - if (pNextIndex) { + if (index_reserved) { for (struct loader_icd_term *icd_term = inst->icd_terms; icd_term; icd_term = icd_term->next) { if (icd_term->debug_report_callback_list.list && icd_term->debug_report_callback_list.list[next_index] && NULL != icd_term->dispatch.DestroyDebugReportCallbackEXT) { diff --git a/tests/loader_alloc_callback_tests.cpp b/tests/loader_alloc_callback_tests.cpp index d2aaf8c81..309e16bfa 100644 --- a/tests/loader_alloc_callback_tests.cpp +++ b/tests/loader_alloc_callback_tests.cpp @@ -52,6 +52,7 @@ class MemoryTracker { const static size_t UNKNOWN_ALLOCATION = std::numeric_limits::max(); size_t allocation_count = 0; size_t call_count = 0; + bool fail_next_growing_reallocation = false; std::unordered_map allocations; void* allocate(size_t size, size_t alignment, VkSystemAllocationScope alloc_scope) { @@ -82,6 +83,10 @@ class MemoryTracker { size_t original_size = elem->second.requested_size_bytes; // We only care about the case where realloc is used to increase the size + if (size >= original_size && fail_next_growing_reallocation) { + fail_next_growing_reallocation = false; + return nullptr; + } if (size >= original_size && settings.should_fail_after_set_number_of_calls && call_count == settings.fail_after_calls) return nullptr; call_count++; @@ -157,6 +162,13 @@ class MemoryTracker { bool empty() noexcept { return allocation_count == 0; } + // Arm a single failure for the next reallocation that grows an existing block. Used to force the used-object + // list resize inside loader_get_next_available_entry to fail without disturbing prior allocations. + void arm_next_growing_reallocation_failure() noexcept { + std::lock_guard lg(main_mutex); + fail_next_growing_reallocation = true; + } + // Static callbacks static VKAPI_ATTR void* VKAPI_CALL public_allocation(void* pUserData, size_t size, size_t alignment, VkSystemAllocationScope allocationScope) noexcept { @@ -506,6 +518,69 @@ TEST(Allocation, CreateSurfaceIntentionalAllocFail) { } } +static VKAPI_ATTR VkBool32 VKAPI_CALL noop_debug_utils_callback(VkDebugUtilsMessageSeverityFlagBitsEXT, + VkDebugUtilsMessageTypeFlagsEXT, + const VkDebugUtilsMessengerCallbackDataEXT*, void*) { + return VK_FALSE; +} + +// When a debug messenger create can't reserve a slot (the used-object status list is full and its resize is +// refused), the error rollback used to be gated on the pNextIndex allocation instead of on a slot actually +// being reserved. With next_index left at 0 the rollback destroyed the ICD handle of the messenger already +// living in slot 0 - a driver-side double free once the application later destroyed that live messenger. +TEST(Allocation, DebugUtilsMessengerReservationFailKeepsLiveMessenger) { + FrameworkEnvironment env{}; + env.add_icd(TEST_ICD_PATH_VERSION_2) + .set_min_icd_interface_version(5) + .add_instance_extension(VK_EXT_DEBUG_UTILS_EXTENSION_NAME) + .add_physical_device({}); + + MemoryTracker tracker{}; + + VkInstance instance = VK_NULL_HANDLE; + InstanceCreateInfo inst_create_info{}; + inst_create_info.add_extension(VK_EXT_DEBUG_UTILS_EXTENSION_NAME); + ASSERT_EQ(VK_SUCCESS, env.vulkan_functions.vkCreateInstance(inst_create_info.get(), tracker.get(), &instance)); + + PFN_vkCreateDebugUtilsMessengerEXT create_messenger = reinterpret_cast( + env.vulkan_functions.vkGetInstanceProcAddr(instance, "vkCreateDebugUtilsMessengerEXT")); + PFN_vkDestroyDebugUtilsMessengerEXT destroy_messenger = reinterpret_cast( + env.vulkan_functions.vkGetInstanceProcAddr(instance, "vkDestroyDebugUtilsMessengerEXT")); + ASSERT_NE(create_messenger, nullptr); + ASSERT_NE(destroy_messenger, nullptr); + + VkDebugUtilsMessengerCreateInfoEXT messenger_info{VK_STRUCTURE_TYPE_DEBUG_UTILS_MESSENGER_CREATE_INFO_EXT}; + messenger_info.messageSeverity = VK_DEBUG_UTILS_MESSAGE_SEVERITY_ERROR_BIT_EXT; + messenger_info.messageType = VK_DEBUG_UTILS_MESSAGE_TYPE_GENERAL_BIT_EXT; + messenger_info.pfnUserCallback = noop_debug_utils_callback; + + // The used-object status list starts with room for 32 entries, so filling it makes the next reservation grow it. + std::vector messengers; + for (uint32_t i = 0; i < 32; i++) { + VkDebugUtilsMessengerEXT messenger = VK_NULL_HANDLE; + ASSERT_EQ(VK_SUCCESS, create_messenger(instance, &messenger_info, nullptr, &messenger)); + messengers.push_back(messenger); + } + + auto& icd_details = env.get_test_icd(0).created_instance_details.begin()->second; + ASSERT_EQ(icd_details.messenger_handles.size(), 32u); + + // Make the used-object list resize fail so the reservation fails after pNextIndex is already allocated. + tracker.arm_next_growing_reallocation_failure(); + VkDebugUtilsMessengerEXT failed_messenger = VK_NULL_HANDLE; + ASSERT_EQ(VK_ERROR_OUT_OF_HOST_MEMORY, create_messenger(instance, &messenger_info, nullptr, &failed_messenger)); + + // No slot was reserved, so the still-live messenger occupying slot 0 must remain untouched in the ICD. + EXPECT_EQ(icd_details.messenger_handles.size(), 32u); + + for (auto messenger : messengers) { + destroy_messenger(instance, messenger, nullptr); + } + EXPECT_EQ(icd_details.messenger_handles.size(), 0u); + env.vulkan_functions.vkDestroyInstance(instance, tracker.get()); + ASSERT_TRUE(tracker.empty()); +} + // Test failure during vkCreateInstance to make sure we don't leak memory if // one of the out-of-memory conditions trigger. TEST(Allocation, CreateInstanceIntentionalAllocFailWithSettingsFilePresent) {