terminator_GetPhysicalDeviceSurfaceSupportKHR bug when multiple ICDs

[jjulianoatnv]

By code inspection of terminator_GetPhysicalDeviceSurfaceSupportKHR(), it appears that this function might not behave as intended when there are multiple ICDs. There is a potential bug that is caused by a flaw in the loader code.

Background:

• An ICD terminator has an optional array of pointers named surface_list. (This should be named surface_array, right?)
• A surface_list has a capacity. Some array elements may have nullptr value. Other array elements may have non-null value. When there is a non-null value an ICD terminator's surface_list, this value was returned by that ICD as output from one of the surface factory Vulkan commands.
• The index into a surface_list array is icd_surface->surface_index. For a given surface index, that index in one ICD's surface_list array may have the value nullptr while a different ICD may have a non-null value.

The Vulkan command that's implemented by terminator_GetPhysicalDeviceSurfaceSupportKHR():

• Has a dispatchable object that is a VkPhysicalDevice. icd_term is that physical device's ICD terminator.
• Has a non-dispatchable object surface that is typecast to the loader-internal value icd_surface.

The tail of terminator_GetPhysicalDeviceSurfaceSupportKHR() contains the following code:

    VkIcdSurface *icd_surface = (VkIcdSurface *)(uintptr_t)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]) {
        return icd_term->dispatch.GetPhysicalDeviceSurfaceSupportKHR(
            phys_dev_term->phys_dev, queueFamilyIndex, icd_term->surface_list.list[icd_surface->surface_index], pSupported);
    }

    return icd_term->dispatch.GetPhysicalDeviceSurfaceSupportKHR(phys_dev_term->phys_dev, queueFamilyIndex, surface, pSupported);

When an ICD's surface_list array has nullptr at the array index surface_index, then the body of the if-statement is not executed. Execution falls to the last line. The last line passes surface into the ICD. The value surface is a typecast pointer to a heap-allocated struct that was created by the Loader, not by this ICD.

• There seems to be an implicit expectation that the ICD returns an error indicating that surface is not a valid handle.
• It is possible that the value of surface aliases a surface handle that was created by this ICD. In that case, the ICD attempts to operate on the wrong surface object.

To fix this, I think the last line that calls the ICD with the loader's surface handle should be replaced. The replacement should skip calling into the ICD, and should simply return an error.

[jjulianoatnv]

This issue is related to #1541