Fix vkCreateSharedSwapchainsKHR not unwrapping handles correctly

The loader would unwrap the first surface and use that for all created swapchains rather than unwrap each surface handle individually for each VkSwapchainCreateInfoKHR struct.
KhronosGroup · Aug 20, 2024 · 4388753 · 4388753
1 parent 4c34d97
commit 4388753
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 19 deletions.
diff --git a/loader/wsi.c b/loader/wsi.c
@@ -2078,31 +2078,32 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateSharedSwapchainsKHR(VkDevice dev
                    "[VUID-vkCreateSharedSwapchainsKHR-device-parameter]");
         abort(); /* Intentionally fail so user can correct issue. */
     }
+    if (NULL != icd_term->surface_list.list) {
+        loader_log(NULL, VULKAN_LOADER_ERROR_BIT, 0,
+                   "vkCreateSharedSwapchainsKHR Terminator: No VkSurfaceKHR objects were created, indicating an application "
+                   "bug. Returning VK_SUCCESS. ");
+        return VK_SUCCESS;
+    }
     if (NULL == dev->loader_dispatch.extension_terminator_dispatch.CreateSharedSwapchainsKHR) {
         loader_log(NULL, VULKAN_LOADER_ERROR_BIT, 0,
-                   "vkCreateSharedSwapchainsKHR: Driver's function pointer was NULL, returning VK_SUCCESS. Was the "
+                   "vkCreateSharedSwapchainsKHR Terminator: Driver's function pointer was NULL, returning VK_SUCCESS. Was the "
                    "VK_KHR_display_swapchain extension enabled?");
         return VK_SUCCESS;
     }
-    VkIcdSurface *icd_surface = (VkIcdSurface *)(uintptr_t)pCreateInfos->surface;
-    if (NULL != icd_term->surface_list.list &&
-        icd_term->surface_list.capacity > icd_surface->surface_index * sizeof(VkSurfaceKHR) &&
-        icd_term->surface_list.list[icd_surface->surface_index]) {
-        // We found the ICD, and there is an ICD KHR surface
-        // associated with it, so copy the CreateInfo struct
-        // and point it at the ICD's surface.
-        VkSwapchainCreateInfoKHR *pCreateCopy = loader_stack_alloc(sizeof(VkSwapchainCreateInfoKHR) * swapchainCount);
-        if (NULL == pCreateCopy) {
-            return VK_ERROR_OUT_OF_HOST_MEMORY;
-        }
-        memcpy(pCreateCopy, pCreateInfos, sizeof(VkSwapchainCreateInfoKHR) * swapchainCount);
-        for (uint32_t sc = 0; sc < swapchainCount; sc++) {
+
+    VkSwapchainCreateInfoKHR *pCreateCopy = loader_stack_alloc(sizeof(VkSwapchainCreateInfoKHR) * swapchainCount);
+    if (NULL == pCreateCopy) {
+        return VK_ERROR_OUT_OF_HOST_MEMORY;
+    }
+    memcpy(pCreateCopy, pCreateInfos, sizeof(VkSwapchainCreateInfoKHR) * swapchainCount);
+    for (uint32_t sc = 0; sc < swapchainCount; sc++) {
+        VkIcdSurface *icd_surface = (VkIcdSurface *)(uintptr_t)pCreateCopy[sc].surface;
+        if (icd_term->surface_list.capacity > icd_surface->surface_index * sizeof(VkSurfaceKHR) &&
+            icd_term->surface_list.list[icd_surface->surface_index]) {
             pCreateCopy[sc].surface = icd_term->surface_list.list[icd_surface->surface_index];
         }
-        return dev->loader_dispatch.extension_terminator_dispatch.CreateSharedSwapchainsKHR(device, swapchainCount, pCreateCopy,
-                                                                                            pAllocator, pSwapchains);
     }
-    return dev->loader_dispatch.extension_terminator_dispatch.CreateSharedSwapchainsKHR(device, swapchainCount, pCreateInfos,
+    return dev->loader_dispatch.extension_terminator_dispatch.CreateSharedSwapchainsKHR(device, swapchainCount, pCreateCopy,
                                                                                         pAllocator, pSwapchains);
 }
 

diff --git a/tests/framework/icd/test_icd.cpp b/tests/framework/icd/test_icd.cpp
@@ -936,10 +936,15 @@ VKAPI_ATTR VkResult VKAPI_CALL test_vkGetPhysicalDeviceSurfaceFormats2KHR(VkPhys
 }
 // VK_KHR_display_swapchain
 VKAPI_ATTR VkResult VKAPI_CALL test_vkCreateSharedSwapchainsKHR([[maybe_unused]] VkDevice device, uint32_t swapchainCount,
-                                                                [[maybe_unused]] const VkSwapchainCreateInfoKHR* pCreateInfos,
+                                                                const VkSwapchainCreateInfoKHR* pCreateInfos,
                                                                 [[maybe_unused]] const VkAllocationCallbacks* pAllocator,
                                                                 VkSwapchainKHR* pSwapchains) {
     for (uint32_t i = 0; i < swapchainCount; i++) {
+        uint64_t surface_integer_value = from_nondispatch_handle(pCreateInfos[i].surface);
+        auto found_iter = std::find(icd.surface_handles.begin(), icd.surface_handles.end(), surface_integer_value);
+        if (found_iter == icd.surface_handles.end()) {
+            return VK_ERROR_INITIALIZATION_FAILED;
+        }
         common_nondispatch_handle_creation(icd.swapchain_handles, &pSwapchains[i]);
     }
     return VK_SUCCESS;

diff --git a/tests/loader_get_proc_addr_tests.cpp b/tests/loader_get_proc_addr_tests.cpp
@@ -213,6 +213,9 @@ TEST(GetDeviceProcAddr, SwapchainFuncsWithTerminator) {
     VkSurfaceKHR surface{};
     ASSERT_EQ(VK_SUCCESS, create_surface(inst, surface));
 
+    VkSurfaceKHR surface2{};
+    ASSERT_EQ(VK_SUCCESS, create_surface(inst, surface2));
+
     DebugUtilsWrapper log{inst};
     ASSERT_EQ(VK_SUCCESS, CreateDebugUtilsMessenger(log));
     auto phys_dev = inst.GetPhysDev();
@@ -286,9 +289,15 @@ TEST(GetDeviceProcAddr, SwapchainFuncsWithTerminator) {
         VkDeviceGroupPresentModeFlagsKHR modes{};
         GetDeviceGroupSurfacePresentModesKHR(dev.dev, surface, &modes);
 
-        CreateSharedSwapchainsKHR(dev.dev, 1, &info, nullptr, &swapchain);
+        std::array<VkSwapchainCreateInfoKHR, 2> infos{};
+        infos[0] = info;
+        infos[1].sType = VK_STRUCTURE_TYPE_SWAPCHAIN_CREATE_INFO_KHR;
+        infos[1].surface = surface2;
+
+        ASSERT_EQ(VK_SUCCESS, CreateSharedSwapchainsKHR(dev.dev, 2, infos.data(), nullptr, &swapchain));
     }
     env.vulkan_functions.vkDestroySurfaceKHR(inst.inst, surface, nullptr);
+    env.vulkan_functions.vkDestroySurfaceKHR(inst.inst, surface2, nullptr);
 }
 
 // Verify that the various ways to get vkGetDeviceProcAddr return the same value