From dd2e70c0799b302bc07b86f759afe9c0aaeda941 Mon Sep 17 00:00:00 2001 From: Chris Forbes Date: Wed, 6 Apr 2016 20:49:02 +1200 Subject: [PATCH] loader: Don't scribble on caller memory in CreateInstance,CreateDevice expand_... / unexpand_... scribbled on both the CreateInfo struct and the list of layer strings, and then unscribbled them on the way back out. This is a lousy thing to do, and just blows up if the memory isn't writable (which it needn't be, given the API takes ptrs to const). Instead, copy the *CreateInfo into a shadow struct on the stack, and be careful in expand_layer_names never to scribble on the caller's layer names array. V2: slight tweak (missed initializer) Signed-off-by: Chris Forbes --- loader/loader.c | 115 ++++++++++++++++++---------------------------------- loader/loader.h | 16 ++++---- loader/trampoline.c | 87 ++++++++++++--------------------------- 3 files changed, 71 insertions(+), 147 deletions(-) diff --git a/loader/loader.c b/loader/loader.c index f45bfce..c81aeab 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -1722,6 +1722,7 @@ static bool loader_find_layer_name_array( return false; } + /** * Searches through an array of layer names (ppp_layer_names) looking for a * layer key_name. @@ -1734,99 +1735,61 @@ static bool loader_find_layer_name_array( * @param ppp_layer_names */ void loader_expand_layer_names( - const struct loader_instance *inst, const char *key_name, + const struct loader_instance *inst, + const char *key_name, uint32_t expand_count, const char expand_names[][VK_MAX_EXTENSION_NAME_SIZE], - uint32_t *layer_count, char ***ppp_layer_names) { - char **pp_layer_names, **pp_src_layers = *ppp_layer_names; + uint32_t *layer_count, char const * const **ppp_layer_names) { - if (!loader_find_layer_name(key_name, *layer_count, - (const char **)pp_src_layers)) - return; // didn't find the key_name in the list + char const * const *pp_src_layers = *ppp_layer_names; - // since the total number of layers may expand, allocate new memory for the - // array of pointers - pp_layer_names = - loader_heap_alloc(inst, (expand_count + *layer_count) * sizeof(char *), - VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); + if (!loader_find_layer_name(key_name, *layer_count, (char const **)pp_src_layers)) + return; // didn't find the key_name in the list. loader_log(inst, VK_DEBUG_REPORT_INFORMATION_BIT_EXT, 0, "Found meta layer %s, replacing with actual layer group", key_name); - // In place removal of any expand_names found in layer_name (remove - // duplicates) - // Also remove the key_name - uint32_t src_idx, dst_idx, cnt = *layer_count; - for (src_idx = 0; src_idx < *layer_count; src_idx++) { - if (loader_find_layer_name_array(pp_src_layers[src_idx], expand_count, - expand_names)) { - pp_src_layers[src_idx] = NULL; - cnt--; - } else if (!strcmp(pp_src_layers[src_idx], key_name)) { - pp_src_layers[src_idx] = NULL; - cnt--; - } - pp_layer_names[src_idx] = pp_src_layers[src_idx]; - } - for (dst_idx = 0; dst_idx < cnt; dst_idx++) { - if (pp_layer_names[dst_idx] == NULL) { - src_idx = dst_idx + 1; - while (src_idx < *layer_count && pp_src_layers[src_idx] == NULL) - src_idx++; - if (src_idx < *layer_count && pp_src_layers[src_idx] != NULL) - pp_layer_names[dst_idx] = pp_src_layers[src_idx]; + + char const **pp_dst_layers = loader_heap_alloc(inst, (expand_count + *layer_count - 1) * sizeof(char const *), + VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); + + // copy layers from src to dst, stripping key_name and anything in + // expand_names. + uint32_t src_index, dst_index = 0; + for (src_index = 0; src_index < *layer_count; src_index++) { + if (loader_find_layer_name_array(pp_src_layers[src_index], expand_count, expand_names) || + !strcmp(pp_src_layers[src_index], key_name)) { + continue; } + + pp_dst_layers[dst_index++] = pp_src_layers[src_index]; } - // Add the expand_names to layer_names - src_idx = 0; - for (dst_idx = cnt; dst_idx < cnt + expand_count; dst_idx++) { - pp_layer_names[dst_idx] = (char *)&expand_names[src_idx++][0]; + // append expand_names. + for (src_index = 0; src_index < expand_count; src_index++) { + pp_dst_layers[dst_index++] = expand_names[src_index]; } - *layer_count = expand_count + cnt; - *ppp_layer_names = pp_layer_names; - return; + + *ppp_layer_names = pp_dst_layers; + *layer_count = dst_index; } -/** - * Restores the layer name list and count into the pCreatInfo structure. - * If is_device == tru then pCreateInfo is a device structure else an instance - * structure. - * @param layer_count - * @param layer_names - * @param pCreateInfo - */ -void loader_unexpand_dev_layer_names(const struct loader_instance *inst, - uint32_t layer_count, char **layer_names, - char **layer_ptr, - const VkDeviceCreateInfo *pCreateInfo) { - uint32_t *p_cnt = (uint32_t *)&pCreateInfo->enabledLayerCount; - *p_cnt = layer_count; - - char ***p_ptr = (char ***)&pCreateInfo->ppEnabledLayerNames; - if ((char **)pCreateInfo->ppEnabledLayerNames != layer_ptr) - loader_heap_free(inst, (void *)pCreateInfo->ppEnabledLayerNames); - *p_ptr = layer_ptr; - for (uint32_t i = 0; i < layer_count; i++) { - char **pp_str = (char **)&pCreateInfo->ppEnabledLayerNames[i]; - *pp_str = layer_names[i]; + +void loader_delete_shadow_dev_layer_names(const struct loader_instance *inst, + const VkDeviceCreateInfo *orig, + VkDeviceCreateInfo *ours) { + /* Free the layer names array iff we had to reallocate it */ + if (orig->ppEnabledLayerNames != ours->ppEnabledLayerNames) { + loader_heap_free(inst, (void *)ours->ppEnabledLayerNames); } } -void loader_unexpand_inst_layer_names(const struct loader_instance *inst, - uint32_t layer_count, char **layer_names, - char **layer_ptr, - const VkInstanceCreateInfo *pCreateInfo) { - uint32_t *p_cnt = (uint32_t *)&pCreateInfo->enabledLayerCount; - *p_cnt = layer_count; - - char ***p_ptr = (char ***)&pCreateInfo->ppEnabledLayerNames; - if ((char **)pCreateInfo->ppEnabledLayerNames != layer_ptr) - loader_heap_free(inst, (void *)pCreateInfo->ppEnabledLayerNames); - *p_ptr = layer_ptr; - for (uint32_t i = 0; i < layer_count; i++) { - char **pp_str = (char **)&pCreateInfo->ppEnabledLayerNames[i]; - *pp_str = layer_names[i]; +void loader_delete_shadow_inst_layer_names(const struct loader_instance *inst, + const VkInstanceCreateInfo *orig, + VkInstanceCreateInfo *ours) { + /* Free the layer names array iff we had to reallocate it */ + if (orig->ppEnabledLayerNames != ours->ppEnabledLayerNames) { + loader_heap_free(inst, (void *)ours->ppEnabledLayerNames); } } diff --git a/loader/loader.h b/loader/loader.h index 84f4d6f..6c192a2 100644 --- a/loader/loader.h +++ b/loader/loader.h @@ -478,15 +478,13 @@ void loader_expand_layer_names( const struct loader_instance *inst, const char *key_name, uint32_t expand_count, const char expand_names[][VK_MAX_EXTENSION_NAME_SIZE], - uint32_t *layer_count, char ***ppp_layer_names); -void loader_unexpand_dev_layer_names(const struct loader_instance *inst, - uint32_t layer_count, char **layer_names, - char **layer_ptr, - const VkDeviceCreateInfo *pCreateInfo); -void loader_unexpand_inst_layer_names(const struct loader_instance *inst, - uint32_t layer_count, char **layer_names, - char **layer_ptr, - const VkInstanceCreateInfo *pCreateInfo); + uint32_t *layer_count, char const * const **ppp_layer_names); +void loader_delete_shadow_dev_layer_names(const struct loader_instance *inst, + const VkDeviceCreateInfo *orig, + VkDeviceCreateInfo *ours); +void loader_delete_shadow_inst_layer_names(const struct loader_instance *inst, + const VkInstanceCreateInfo *orig, + VkInstanceCreateInfo *ours); void loader_add_to_layer_list(const struct loader_instance *inst, struct loader_layer_list *list, uint32_t prop_list_count, diff --git a/loader/trampoline.c b/loader/trampoline.c index d1c02f0..1c998ca 100644 --- a/loader/trampoline.c +++ b/loader/trampoline.c @@ -345,21 +345,12 @@ vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, } /* convert any meta layers to the actual layers makes a copy of layer name*/ - uint32_t saved_layer_count = pCreateInfo->enabledLayerCount; - char **saved_layer_names; - char **saved_layer_ptr; - saved_layer_names = - loader_stack_alloc(sizeof(char *) * pCreateInfo->enabledLayerCount); - for (uint32_t i = 0; i < saved_layer_count; i++) { - saved_layer_names[i] = (char *)pCreateInfo->ppEnabledLayerNames[i]; - } - saved_layer_ptr = (char **)pCreateInfo->ppEnabledLayerNames; - + VkInstanceCreateInfo ici = *pCreateInfo; loader_expand_layer_names( ptr_instance, std_validation_str, sizeof(std_validation_names) / sizeof(std_validation_names[0]), - std_validation_names, (uint32_t *)&pCreateInfo->enabledLayerCount, - (char ***)&pCreateInfo->ppEnabledLayerNames); + std_validation_names, &ici.enabledLayerCount, + &ici.ppEnabledLayerNames); /* Scan/discover all ICD libraries */ memset(&ptr_instance->icd_libs, 0, sizeof(ptr_instance->icd_libs)); @@ -370,11 +361,9 @@ vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, ptr_instance, &ptr_instance->icd_libs, &ptr_instance->ext_list); res = loader_validate_instance_extensions( ptr_instance, &ptr_instance->ext_list, - &ptr_instance->instance_layer_list, pCreateInfo); + &ptr_instance->instance_layer_list, &ici); if (res != VK_SUCCESS) { - loader_unexpand_inst_layer_names(ptr_instance, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_inst_layer_names(ptr_instance, pCreateInfo, &ici); loader_delete_layer_properties(ptr_instance, &ptr_instance->device_layer_list); loader_delete_layer_properties(ptr_instance, @@ -399,9 +388,7 @@ vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, loader_heap_alloc(ptr_instance, sizeof(VkLayerInstanceDispatchTable), VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE); if (ptr_instance->disp == NULL) { - loader_unexpand_inst_layer_names(ptr_instance, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_inst_layer_names(ptr_instance, pCreateInfo, &ici); loader_delete_layer_properties(ptr_instance, &ptr_instance->device_layer_list); loader_delete_layer_properties(ptr_instance, @@ -426,12 +413,10 @@ vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, loader.instances = ptr_instance; /* activate any layers on instance chain */ - res = loader_enable_instance_layers(ptr_instance, pCreateInfo, + res = loader_enable_instance_layers(ptr_instance, &ici, &ptr_instance->instance_layer_list); if (res != VK_SUCCESS) { - loader_unexpand_inst_layer_names(ptr_instance, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_inst_layer_names(ptr_instance, pCreateInfo, &ici); loader_delete_layer_properties(ptr_instance, &ptr_instance->device_layer_list); loader_delete_layer_properties(ptr_instance, @@ -455,12 +440,12 @@ vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, } created_instance = (VkInstance)ptr_instance; - res = loader_create_instance_chain(pCreateInfo, pAllocator, ptr_instance, + res = loader_create_instance_chain(&ici, pAllocator, ptr_instance, &created_instance); if (res == VK_SUCCESS) { - wsi_create_instance(ptr_instance, pCreateInfo); - debug_report_create_instance(ptr_instance, pCreateInfo); + wsi_create_instance(ptr_instance, &ici); + debug_report_create_instance(ptr_instance, &ici); *pInstance = created_instance; @@ -480,9 +465,7 @@ vkCreateInstance(const VkInstanceCreateInfo *pCreateInfo, pAllocator, ptr_instance->num_tmp_callbacks, ptr_instance->tmp_callbacks); - loader_unexpand_inst_layer_names(ptr_instance, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_inst_layer_names(ptr_instance, pCreateInfo, &ici); loader_platform_thread_unlock_mutex(&loader_lock); return res; } @@ -699,40 +682,27 @@ vkCreateDevice(VkPhysicalDevice physicalDevice, } /* convert any meta layers to the actual layers makes a copy of layer name*/ - uint32_t saved_layer_count = pCreateInfo->enabledLayerCount; - char **saved_layer_names; - char **saved_layer_ptr; - saved_layer_names = - loader_stack_alloc(sizeof(char *) * pCreateInfo->enabledLayerCount); - for (uint32_t i = 0; i < saved_layer_count; i++) { - saved_layer_names[i] = (char *)pCreateInfo->ppEnabledLayerNames[i]; - } - saved_layer_ptr = (char **)pCreateInfo->ppEnabledLayerNames; - + VkDeviceCreateInfo dci = *pCreateInfo; loader_expand_layer_names( inst, std_validation_str, sizeof(std_validation_names) / sizeof(std_validation_names[0]), - std_validation_names, (uint32_t *)&pCreateInfo->enabledLayerCount, - (char ***)&pCreateInfo->ppEnabledLayerNames); + std_validation_names, &dci.enabledLayerCount, + &dci.ppEnabledLayerNames); /* fetch a list of all layers activated, explicit and implicit */ res = loader_enable_device_layers(inst, &activated_layer_list, - pCreateInfo, &inst->device_layer_list); + &dci, &inst->device_layer_list); if (res != VK_SUCCESS) { - loader_unexpand_dev_layer_names(inst, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_dev_layer_names(inst, pCreateInfo, &dci); loader_platform_thread_unlock_mutex(&loader_lock); return res; } /* make sure requested extensions to be enabled are supported */ res = loader_validate_device_extensions(phys_dev, &activated_layer_list, - &icd_exts, pCreateInfo); + &icd_exts, &dci); if (res != VK_SUCCESS) { - loader_unexpand_dev_layer_names(inst, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_dev_layer_names(inst, pCreateInfo, &dci); loader_destroy_generic_list( inst, (struct loader_generic_list *)&activated_layer_list); loader_platform_thread_unlock_mutex(&loader_lock); @@ -741,9 +711,7 @@ vkCreateDevice(VkPhysicalDevice physicalDevice, dev = loader_create_logical_device(inst); if (dev == NULL) { - loader_unexpand_dev_layer_names(inst, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_dev_layer_names(inst, pCreateInfo, &dci); loader_destroy_generic_list( inst, (struct loader_generic_list *)&activated_layer_list); loader_platform_thread_unlock_mutex(&loader_lock); @@ -759,21 +727,17 @@ vkCreateDevice(VkPhysicalDevice physicalDevice, /* activate any layers on device chain which terminates with device*/ res = loader_enable_device_layers(inst, &dev->activated_layer_list, - pCreateInfo, &inst->device_layer_list); + &dci, &inst->device_layer_list); if (res != VK_SUCCESS) { - loader_unexpand_dev_layer_names(inst, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_dev_layer_names(inst, pCreateInfo, &dci); loader_platform_thread_unlock_mutex(&loader_lock); return res; } - res = loader_create_device_chain(phys_dev, pCreateInfo, pAllocator, inst, + res = loader_create_device_chain(phys_dev, &dci, pAllocator, inst, dev); if (res != VK_SUCCESS) { - loader_unexpand_dev_layer_names(inst, saved_layer_count, - saved_layer_names, saved_layer_ptr, - pCreateInfo); + loader_delete_shadow_dev_layer_names(inst, pCreateInfo, &dci); loader_platform_thread_unlock_mutex(&loader_lock); return res; } @@ -790,8 +754,7 @@ vkCreateDevice(VkPhysicalDevice physicalDevice, &dev->loader_dispatch, dev->loader_dispatch.core_dispatch.GetDeviceProcAddr, *pDevice); - loader_unexpand_dev_layer_names(inst, saved_layer_count, saved_layer_names, - saved_layer_ptr, pCreateInfo); + loader_delete_shadow_dev_layer_names(inst, pCreateInfo, &dci); loader_platform_thread_unlock_mutex(&loader_lock); return res; -- 2.7.4