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 <chrisforbes@google.com>

diff --git a/loader/loader.c b/loader/loader.c
index f45bfce..c81aeab 100644 (file)
--- 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 (file)
--- 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 (file)
--- 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;