Skip to content

Commit

Permalink
Fix corrupted pNext chain in vkCreateDevice
Browse files Browse the repository at this point in the history 
When creating a device, the loader looks for the VkDeviceGroupCreateInfo
structure and replaces it with its own. This allows the loader to edit the
struct. However, to do this required editing the pNext chain. Because the
edited chain contained pointers to structures whose lifetimes end when the
vkCreateDevice function returns, the pNext chain is now corrupted.

This commit fixes that by storing a pointer to the user's
VkDeviceGroupCreateInfo and fixing up the pNext chain to use that instead.
  • Loading branch information
@charles-lunarg
charles-lunarg committed Jul 8, 2022
1 parent 0c7685b commit 57d5dd5
Show file tree
Hide file tree
Showing 2 changed files with 153 additions and 1 deletion.
23 changes: 22 additions & 1 deletion loader/loader.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4732,6 +4732,7 @@ VkResult loader_create_device_chain(const VkPhysicalDevice pd, const VkDeviceCre
VkLayerDeviceLink *layer_device_link_info;
VkLayerDeviceCreateInfo chain_info;
VkDeviceCreateInfo loader_create_info;
VkDeviceGroupDeviceCreateInfoKHR *original_device_group_create_info_struct = NULL;
VkResult res;

PFN_vkGetDeviceProcAddr fpGDPA = NULL, nextGDPA = loader_gpa_device_internal;
Expand Down Expand Up @@ -4770,6 +4771,8 @@ VkResult loader_create_device_chain(const VkPhysicalDevice pd, const VkDeviceCre
}
temp_struct->pPhysicalDevices = phys_dev_array;

original_device_group_create_info_struct = (VkDeviceGroupDeviceCreateInfoKHR *)pPrev->pNext;

// Replace the old struct in the pNext chain with this one.
pPrev->pNext = (VkBaseOutStructure *)temp_struct;
}
Expand Down Expand Up @@ -4921,6 +4924,24 @@ VkResult loader_create_device_chain(const VkPhysicalDevice pd, const VkDeviceCre
return res;
}
dev->chain_device = created_device;

// Because we changed the pNext chain to use our own VkDeviceGroupDeviceCreateInfoKHR, we need to fixup the chain to point
// back at the original VkDeviceGroupDeviceCreateInfoKHR.
VkBaseOutStructure *pNext = (VkBaseOutStructure *)loader_create_info.pNext;
VkBaseOutStructure *pPrev = (VkBaseOutStructure *)&loader_create_info;
while (NULL != pNext) {
if (VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO == pNext->sType) {
VkDeviceGroupDeviceCreateInfoKHR *cur_struct = (VkDeviceGroupDeviceCreateInfoKHR *)pNext;
if (0 < cur_struct->physicalDeviceCount && NULL != cur_struct->pPhysicalDevices) {
pPrev->pNext = (VkBaseOutStructure *)original_device_group_create_info_struct;
}
break;
}

pPrev = pNext;
pNext = pNext->pNext;
}

} else {
loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0,
"loader_create_device_chain: Failed to find \'vkCreateDevice\' in layers or ICD");
Expand Down Expand Up @@ -5459,7 +5480,7 @@ VKAPI_ATTR void VKAPI_CALL terminator_DestroyInstance(VkInstance instance, const
loader_destroy_generic_list(ptr_instance, (struct loader_generic_list *)&ptr_instance->ext_list);
if (NULL != ptr_instance->phys_devs_term) {
for (uint32_t i = 0; i < ptr_instance->phys_dev_count_term; i++) {
for (uint32_t j = i+1; j < ptr_instance->phys_dev_count_term; j++) {
for (uint32_t j = i + 1; j < ptr_instance->phys_dev_count_term; j++) {
if (ptr_instance->phys_devs_term[i] == ptr_instance->phys_devs_term[j]) {
ptr_instance->phys_devs_term[j] = NULL;
}
Expand Down
131 changes: 131 additions & 0 deletions tests/loader_regression_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1389,6 +1389,35 @@ TEST(EnumeratePhysicalDeviceGroups, OneCall) {
for (uint32_t dev = 0; dev < max_physical_device_count; ++dev) {
ASSERT_EQ(true, found[dev]);
}
for (auto& group : group_props) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
VkBaseInStructure spacer_structure{};
spacer_structure.sType = static_cast<VkStructureType>(100000);
spacer_structure.pNext = reinterpret_cast<const VkBaseInStructure*>(&group_info);
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &spacer_structure;
dev.CheckCreate(group.physicalDevices[0]);

// This convoluted logic makes sure that the pNext chain is unmolested after being passed into vkCreateDevice
// While not expected for applications to iterate over this chain, since it is const it is important to make sure
// that the chain didn't change somehow, and especially so that iterating it doesn't crash.
int count = 0;
const VkBaseInStructure* pNext = reinterpret_cast<const VkBaseInStructure*>(dev.create_info.dev.pNext);
while (pNext != nullptr) {
if (pNext->sType == VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO) {
ASSERT_EQ(&group_info, reinterpret_cast<const VkDeviceGroupDeviceCreateInfoKHR*>(pNext));
}
if (pNext->sType == 100000) {
ASSERT_EQ(&spacer_structure, pNext);
}
pNext = pNext->pNext;
count++;
}
ASSERT_EQ(count, 2);
}
}
driver.add_instance_extension({VK_KHR_DEVICE_GROUP_CREATION_EXTENSION_NAME});
// Extension
Expand Down Expand Up @@ -1428,6 +1457,18 @@ TEST(EnumeratePhysicalDeviceGroups, OneCall) {
for (uint32_t dev = 0; dev < max_physical_device_count; ++dev) {
ASSERT_EQ(true, found[dev]);
}
for (auto& group : group_props) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
VkBaseInStructure spacer_structure{};
spacer_structure.sType = static_cast<VkStructureType>(100000);
spacer_structure.pNext = reinterpret_cast<const VkBaseInStructure*>(&group_info);
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &spacer_structure;
dev.CheckCreate(group.physicalDevices[0]);
}
}
}

Expand Down Expand Up @@ -1484,6 +1525,15 @@ TEST(EnumeratePhysicalDeviceGroups, TwoCall) {
for (uint32_t dev = 0; dev < max_physical_device_count; ++dev) {
ASSERT_EQ(true, found[dev]);
}
for (auto& group : group_props) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}
driver.add_instance_extension({VK_KHR_DEVICE_GROUP_CREATION_EXTENSION_NAME});
// Extension
Expand Down Expand Up @@ -1526,6 +1576,15 @@ TEST(EnumeratePhysicalDeviceGroups, TwoCall) {
for (uint32_t dev = 0; dev < max_physical_device_count; ++dev) {
ASSERT_EQ(true, found[dev]);
}
for (auto& group : group_props) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}
}

Expand Down Expand Up @@ -1581,6 +1640,15 @@ TEST(EnumeratePhysicalDeviceGroups, TwoCallIncomplete) {
}
ASSERT_EQ(true, found);
}
for (auto& group : group_props) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}
driver.add_instance_extension({VK_KHR_DEVICE_GROUP_CREATION_EXTENSION_NAME});
// Extension
Expand Down Expand Up @@ -1623,6 +1691,15 @@ TEST(EnumeratePhysicalDeviceGroups, TwoCallIncomplete) {
}
ASSERT_EQ(true, found);
}
for (auto& group : group_props) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}
}

Expand Down Expand Up @@ -1704,6 +1781,15 @@ TEST(EnumeratePhysicalDeviceGroups, TestCoreVersusExtensionSameReturns) {
}
}
}
for (auto& group : core_group_props) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}

// Start with 6 devices in 3 different groups, and then add a group,
Expand Down Expand Up @@ -1788,6 +1874,15 @@ TEST(EnumeratePhysicalDeviceGroups, CallThriceAddGroupInBetween) {
}
}
}
for (auto& group : group_props_after) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}

// Start with 7 devices in 4 different groups, and then remove a group,
Expand Down Expand Up @@ -1864,6 +1959,15 @@ TEST(EnumeratePhysicalDeviceGroups, CallTwiceRemoveGroupInBetween) {
}
}
}
for (auto& group : group_props_after) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}

// Start with 6 devices in 3 different groups, and then add a device to the middle group,
Expand Down Expand Up @@ -1939,6 +2043,15 @@ TEST(EnumeratePhysicalDeviceGroups, CallTwiceAddDeviceInBetween) {
}
}
}
for (auto& group : group_props_after) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}

// Start with 6 devices in 3 different groups, and then remove a device to the middle group,
Expand Down Expand Up @@ -2025,6 +2138,15 @@ TEST(EnumeratePhysicalDeviceGroups, CallTwiceRemoveDeviceInBetween) {
}
}
}
for (auto& group : group_props_after) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}

// Start with 9 devices but only some in 3 different groups, add and remove
Expand Down Expand Up @@ -2132,6 +2254,15 @@ TEST(EnumeratePhysicalDeviceGroups, MultipleAddRemoves) {
for (uint32_t group = 0; group < before_group_count; ++group) {
ASSERT_EQ(group_props_after_add_device[group].physicalDeviceCount, after_add_dev_expected_counts[group]);
}
for (auto& group : group_props_after_add_device) {
VkDeviceGroupDeviceCreateInfoKHR group_info{};
group_info.sType = VK_STRUCTURE_TYPE_DEVICE_GROUP_DEVICE_CREATE_INFO;
group_info.physicalDeviceCount = group.physicalDeviceCount;
group_info.pPhysicalDevices = &group.physicalDevices[0];
DeviceWrapper dev{inst};
dev.create_info.dev.pNext = &group_info;
dev.CheckCreate(group.physicalDevices[0]);
}
}

// Fill in random but valid data into the device properties struct for the current physical device
Expand Down

0 comments on commit 57d5dd5

Please sign in to comment.