Repository navigation
initialise ext_count in linux_sort_physical_device_groups - #2082
Conversation
linux_sort_physical_device_groups passes &ext_count to the driver's vkEnumerateDeviceExtensionProperties count query without initialising it and without checking the result. A driver that fails the query before writing the count leaves ext_count indeterminate, and the loader then branches on it, sizes a heap allocation from it and passes it back to the driver as the capacity of the second query. The sibling linux_read_sorted_physical_devices already initialises its ext_count to 0, so a failed query is skipped there. Do the same here. Add a test ICD knob so vkEnumerateDeviceExtensionProperties can fail without writing its outputs, and a device group sorting test that runs through the failure path. Valgrind reports the uninitialised read at loader_linux.c:413 on the unpatched loader and is clean afterwards.
|
User aizu-m not on autobuild list. Waiting for curator authorization before starting CI build. |
1 similar comment
|
User aizu-m not on autobuild list. Waiting for curator authorization before starting CI build. |
|
CI Vulkan-Loader build queued with queue ID 170422. |
| uint32_t ext_count; | ||
| uint32_t ext_count = 0; | ||
| icd_term->dispatch.EnumerateDeviceExtensionProperties( | ||
| sorted_group_term[group].internal_device_info[gpu].physical_device, NULL, &ext_count, NULL); |
There was a problem hiding this comment.
Not a request, just a comment about pre-existing code, but it certainly is bad form to not check the return value of vkEnumerateDeviceExtensionProperties, so thats something to fix in the future.
There was a problem hiding this comment.
Agreed, the ignored VkResult is the real smell there. Zeroing the count only papers over it. I will look at checking it properly in a follow-up. Thanks for merging this.
|
CI Vulkan-Loader build # 3909 running. |
|
CI Vulkan-Loader build # 3909 passed. |
Valgrind, Linux Debug build, running the new test against the unpatched loader:
Found while reading the two Linux sorting functions after the alloca removal and the initialiser sweep in 470f90e, which touched the
dev_propsline two above this one.linux_read_sorted_physical_devicesinitialises itsext_countto 0.linux_sort_physical_device_groupsdeclares it uninitialised and ignores the result of the count query. When the driver fails that query before writing the count (VK_ERROR_OUT_OF_HOST_MEMORY is a permitted return), the loader branches on whatever was on the stack, sizes theext_propsallocation from it and hands it to the driver as the capacity of the second query. Initialise it to 0 like the sibling, so a failed query skips the extension scan.The test ICD gains
enum_device_extension_properties_return_code, which makesvkEnumerateDeviceExtensionPropertiesreturn the given code without writing its outputs.SortedPhysicalDevices.DeviceGroupsSortedExtensionQueryFailsdrives a two device group through that path. It only reports the bug under Valgrind or MemorySanitizer, since an ordinary build just sees whatever value the stack held. With the fix Valgrind is clean. Full suite green under ASAN/UBSAN on Linux and macOS, clang-format 16 clean.