KhronosGroup / KhronosGroup/Vulkan-Loader
terminator_GetPhysicalDeviceSurfaceSupportKHR bug when multiple ICDs
- 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
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