KhronosGroup / KhronosGroup/Vulkan-Loader

terminator_GetPhysicalDeviceSurfaceSupportKHR bug when multiple ICDs

Open
#1,666 2 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
C
Stars
695
Forks
343
Avg merge
1d 1h
Merged PRs (30d)
17

Description

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.

Contributor guide

Open the contributing guide

Research direction

Start with terminator_GetPhysicalDeviceSurfaceSupportKHR() and the VkIcdSurface surface_list handling described in the issue. Reproduce the multiple-ICD case where the indexed entry is nullptr, then verify that the loader no longer passes its own surface handle into the ICD and instead returns the appropriate error.

Written by the indexing model from the issue text.

Assessment

Tech stack
c
Domain
computer-graphics
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
42/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.