From adf0a470cec7b1bfd671cfa2da43713a7a68c20d Mon Sep 17 00:00:00 2001 From: Charles Giessen Date: Thu, 28 Sep 2023 13:08:46 -0600 Subject: [PATCH] Fix problems detected by CodeQL static analysis 3 types of issues were found: * Incorrect format specifiers * Not providing required format specifies * Using alloca in a loop There are multiple instances of alloca in loops, but to fix them would require significant refactoring. This commit includes 1 move of alloca inside a for loop to the outside, but this is because the logic was redoing work in a for loop that could have been done once at the start of the function. --- loader/cJSON.c | 2 +- loader/loader.c | 5 +++-- loader/settings.c | 44 +++++++++++++++++++++++--------------------- loader/settings.h | 2 +- 4 files changed, 28 insertions(+), 25 deletions(-) diff --git a/loader/cJSON.c b/loader/cJSON.c index 4fb9585c8..47faa8357 100644 --- a/loader/cJSON.c +++ b/loader/cJSON.c @@ -1271,7 +1271,7 @@ VkResult loader_get_json(const struct loader_instance *inst, const char *filenam json_buf = (char *)loader_instance_heap_calloc(inst, len + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); if (json_buf == NULL) { loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0, - "loader_get_json: Failed to allocate space for JSON file %s buffer of length %d", filename, len); + "loader_get_json: Failed to allocate space for JSON file %s buffer of length %lu", filename, len); res = VK_ERROR_OUT_OF_HOST_MEMORY; goto out; } diff --git a/loader/loader.c b/loader/loader.c index 313dbcdb6..109697149 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -1495,7 +1495,8 @@ VkResult loader_add_direct_driver(const struct loader_instance *inst, uint32_t i void *new_ptr = loader_instance_heap_realloc(inst, icd_tramp_list->scanned_list, icd_tramp_list->capacity, icd_tramp_list->capacity * 2, VK_SYSTEM_ALLOCATION_SCOPE_INSTANCE); if (NULL == new_ptr) { - loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0, "loader_add_direct_driver: Realloc failed on icd library list for ICD %s"); + loader_log(inst, VULKAN_LOADER_ERROR_BIT, 0, + "loader_add_direct_driver: Realloc failed on icd library list for ICD index %u", index); return VK_ERROR_OUT_OF_HOST_MEMORY; } icd_tramp_list->scanned_list = new_ptr; @@ -5210,7 +5211,7 @@ VKAPI_ATTR VkResult VKAPI_CALL terminator_CreateInstance(const VkInstanceCreateI "#LLP_LAYER_21)"); } else if (LOADER_MAGIC_NUMBER != ptr_instance->magic) { loader_log(ptr_instance, VULKAN_LOADER_WARN_BIT, 0, - "terminator_CreateInstance: Instance pointer (%p) has invalid MAGIC value 0x%08x. Instance value possibly " + "terminator_CreateInstance: Instance pointer (%p) has invalid MAGIC value 0x%08lx. Instance value possibly " "corrupted by active layer (Policy #LLP_LAYER_21). ", ptr_instance, ptr_instance->magic); } diff --git a/loader/settings.c b/loader/settings.c index 861376675..04f4f3081 100644 --- a/loader/settings.c +++ b/loader/settings.c @@ -711,15 +711,20 @@ VkResult combine_settings_layers_with_regular_layers(const struct loader_instanc } VkResult enable_correct_layers_from_settings(const struct loader_instance* inst, const struct loader_envvar_all_filters* filters, - uint32_t name_count, const char* const* names, + uint32_t app_enabled_name_count, const char* const* app_enabled_names, const struct loader_layer_list* instance_layers, struct loader_pointer_layer_list* target_layer_list, struct loader_pointer_layer_list* activated_layer_list) { VkResult res = VK_SUCCESS; char* vk_instance_layers_env = loader_getenv(ENABLED_LAYERS_ENV, inst); + size_t vk_instance_layers_env_len = 0; + char* vk_instance_layers_env_copy = NULL; if (vk_instance_layers_env != NULL) { - loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0, "env var \'%s\' defined and adding layers \"%s\"", - ENABLED_LAYERS_ENV, names); + vk_instance_layers_env_len = strlen(vk_instance_layers_env) + 1; + vk_instance_layers_env_copy = loader_stack_alloc(vk_instance_layers_env_len); + + loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0, "env var \'%s\' defined and adding layers: %s", + ENABLED_LAYERS_ENV, vk_instance_layers_env); } for (uint32_t i = 0; i < instance_layers->count; i++) { bool enable_layer = false; @@ -745,30 +750,27 @@ VkResult enable_correct_layers_from_settings(const struct loader_instance* inst, enable_layer = true; } - if (!enable_layer && vk_instance_layers_env) { - size_t vk_instance_layers_env_len = strlen(vk_instance_layers_env) + 1; - char* name = loader_stack_alloc(vk_instance_layers_env_len); - if (name != NULL) { - loader_strncpy(name, vk_instance_layers_env_len, vk_instance_layers_env, vk_instance_layers_env_len); - // First look for the old-fashion layers forced on with VK_INSTANCE_LAYERS - while (name && *name) { - char* next = loader_get_next_path(name); - - if (strlen(name) > 0) { - if (0 == strcmp(name, props->info.layerName)) { - enable_layer = true; - break; - } - name = next; - } + // First look for the old-fashion layers forced on with VK_INSTANCE_LAYERS + if (!enable_layer && vk_instance_layers_env && vk_instance_layers_env_copy && vk_instance_layers_env_len > 0) { + // Copy the env-var on each iteration, so that loader_get_next_path can correctly find the separators + // This solution only needs one stack allocation ahead of time rather than an allocation per layer in the env-var + loader_strncpy(vk_instance_layers_env_copy, vk_instance_layers_env_len, vk_instance_layers_env, + vk_instance_layers_env_len); + + while (vk_instance_layers_env_copy && *vk_instance_layers_env_copy) { + char* next = loader_get_next_path(vk_instance_layers_env_copy); + if (0 == strcmp(vk_instance_layers_env_copy, props->info.layerName)) { + enable_layer = true; + break; } + vk_instance_layers_env_copy = next; } } // Check if it should be enabled by the application if (!enable_layer) { - for (uint32_t j = 0; j < name_count; j++) { - if (strcmp(props->info.layerName, names[j]) == 0) { + for (uint32_t j = 0; j < app_enabled_name_count; j++) { + if (strcmp(props->info.layerName, app_enabled_names[j]) == 0) { enable_layer = true; break; } diff --git a/loader/settings.h b/loader/settings.h index 1e9dbf0a9..d3cd8b7cc 100644 --- a/loader/settings.h +++ b/loader/settings.h @@ -108,7 +108,7 @@ VkResult combine_settings_layers_with_regular_layers(const struct loader_instanc // Fill out activated_layer_list with the layers that should be activated, based on environment variables, VkInstanceCreateInfo, and // the settings VkResult enable_correct_layers_from_settings(const struct loader_instance* inst, const struct loader_envvar_all_filters* filters, - uint32_t name_count, const char* const* names, + uint32_t app_enabled_name_count, const char* const* app_enabled_names, const struct loader_layer_list* instance_layers, struct loader_pointer_layer_list* target_layer_list, struct loader_pointer_layer_list* activated_layer_list);