From 3382accd2328be9bb5c0d3caff6bca4250028633 Mon Sep 17 00:00:00 2001 From: Charles Giessen Date: Mon, 29 May 2023 17:06:06 -0600 Subject: [PATCH] Fix issues found with clang-tidy clang-tidy found numerous issues, roughly divided into three categories: * Unused variables * strcpy is unsafe * NULL dereference due to missing NULL check --- loader/cJSON.c | 14 +++---- loader/loader.c | 79 ++++++++++++++++++++---------------- loader/settings.c | 10 +++-- loader/unknown_function_handling.c | 1 - tests/framework/test_environment.cpp | 1 + 5 files changed, 57 insertions(+), 48 deletions(-) diff --git a/loader/cJSON.c b/loader/cJSON.c index 2323e9b..99b3e5d 100644 --- a/loader/cJSON.c +++ b/loader/cJSON.c @@ -182,7 +182,7 @@ char *print_number(cJSON *item, printbuffer *p) { str = ensure(item->pAllocator, p, str_buf_size); else str = (char *)cJSON_malloc(item->pAllocator, str_buf_size); - if (str) strcpy(str, "0"); + if (str) strncpy(str, "0", str_buf_size); } else if (fabs(((double)item->valueint) - d) <= DBL_EPSILON && d <= INT_MAX && d >= INT_MIN) { str_buf_size = 21; /* 2^64+1 can be represented in 21 chars. */ if (p) @@ -361,7 +361,7 @@ char *print_string_ptr(const VkAllocationCallbacks *pAllocator, const char *str, if (!out) return 0; ptr2 = out; // *ptr2++ = '\"'; // Modified to not put quotes around the string - strcpy(ptr2, str); + strncpy(ptr2, str, out_buf_size); // ptr2[len] = '\"'; // Modified to not put quotes around the string ptr2[len] = 0; // ptr2[len + 1] = 0; // Modified to not put quotes around the string return out; @@ -374,7 +374,7 @@ char *print_string_ptr(const VkAllocationCallbacks *pAllocator, const char *str, else out = (char *)cJSON_malloc_instance_scope(pAllocator, out_buf_size); if (!out) return 0; - strcpy(out, "\"\""); + strncpy(out, "\"\"", out_buf_size); return out; } ptr = str; @@ -538,17 +538,17 @@ char *print_value(cJSON *item, int depth, int fmt, printbuffer *p) { switch ((item->type) & 255) { case cJSON_NULL: { out = ensure(item->pAllocator, p, 5); - if (out) strcpy(out, "null"); + if (out) strncpy(out, "null", 5); break; } case cJSON_False: { out = ensure(item->pAllocator, p, 6); - if (out) strcpy(out, "false"); + if (out) strncpy(out, "false", 6); break; } case cJSON_True: { out = ensure(item->pAllocator, p, 5); - if (out) strcpy(out, "true"); + if (out) strncpy(out, "true", 5); break; } case cJSON_Number: @@ -642,7 +642,7 @@ char *print_array(cJSON *item, int depth, int fmt, printbuffer *p) { out = ensure(item->pAllocator, p, 3); else out = (char *)cJSON_malloc(item->pAllocator, 3); - if (out) strcpy(out, "[]"); + if (out) strncpy(out, "[]", 3); return out; } diff --git a/loader/loader.c b/loader/loader.c index 1d87c41..39bdedc 100644 --- a/loader/loader.c +++ b/loader/loader.c @@ -1024,7 +1024,7 @@ bool loader_implicit_layer_is_enabled(const struct loader_instance *inst, const loader_log(inst, VULKAN_LOADER_WARN_BIT | VULKAN_LOADER_LAYER_BIT, 0, "Implicit layer \"%s\" forced disabled because name matches filter of env var \'%s\'.", prop->info.layerName, VK_LAYERS_DISABLE_ENV_VAR); - return false; + return enable; } // The disable_environment has priority over everything else. If it is defined, the layer is always @@ -1428,7 +1428,7 @@ bool loader_get_icd_interface_version(PFN_vkNegotiateLoaderICDInterfaceVersion f } void loader_scanned_icd_clear(const struct loader_instance *inst, struct loader_icd_tramp_list *icd_tramp_list) { - if (0 != icd_tramp_list->capacity) { + if (0 != icd_tramp_list->capacity && icd_tramp_list->scanned_list) { for (uint32_t i = 0; i < icd_tramp_list->count; i++) { if (icd_tramp_list->scanned_list[i].handle) { loader_platform_close_library(icd_tramp_list->scanned_list[i].handle); @@ -2038,8 +2038,9 @@ out: // @return - A string in out_fullpath of either the full path or file. void loader_get_fullpath(const char *file, const char *in_dirs, size_t out_size, char *out_fullpath) { if (!loader_platform_is_path(file) && *in_dirs) { - char *dirs_copy = loader_stack_alloc(strlen(in_dirs) + 1); - strcpy(dirs_copy, in_dirs); + size_t dirs_copy_len = strlen(in_dirs) + 1; + char *dirs_copy = loader_stack_alloc(dirs_copy_len); + strncpy(dirs_copy, in_dirs, dirs_copy_len); // find if file exists after prepending paths in given list // for (dir = dirs_copy; *dir && (next_dir = loader_get_next_path(dir)); dir = next_dir) { @@ -2134,30 +2135,33 @@ bool update_meta_layer_extensions_from_component_layers(const struct loader_inst struct loader_layer_properties *comp_prop = loader_find_layer_property(prop->component_layer_names.list[comp_layer], instance_layers); - for (uint32_t ext = 0; ext < comp_prop->instance_extension_list.count; ext++) { - loader_log(inst, VULKAN_LOADER_DEBUG_BIT, 0, "Meta-layer %s component layer %s adding instance extension %s", - prop->info.layerName, prop->component_layer_names.list[comp_layer], - comp_prop->instance_extension_list.list[ext].extensionName); - - if (!has_vk_extension_property(&comp_prop->instance_extension_list.list[ext], &prop->instance_extension_list)) { - res = - loader_add_to_ext_list(inst, &prop->instance_extension_list, 1, &comp_prop->instance_extension_list.list[ext]); - if (VK_ERROR_OUT_OF_HOST_MEMORY == res) { - return res; + if (NULL != comp_prop->instance_extension_list.list) { + for (uint32_t ext = 0; ext < comp_prop->instance_extension_list.count; ext++) { + loader_log(inst, VULKAN_LOADER_DEBUG_BIT, 0, "Meta-layer %s component layer %s adding instance extension %s", + prop->info.layerName, prop->component_layer_names.list[comp_layer], + comp_prop->instance_extension_list.list[ext].extensionName); + + if (!has_vk_extension_property(&comp_prop->instance_extension_list.list[ext], &prop->instance_extension_list)) { + res = loader_add_to_ext_list(inst, &prop->instance_extension_list, 1, + &comp_prop->instance_extension_list.list[ext]); + if (VK_ERROR_OUT_OF_HOST_MEMORY == res) { + return res; + } } } } + if (NULL != comp_prop->device_extension_list.list) { + for (uint32_t ext = 0; ext < comp_prop->device_extension_list.count; ext++) { + loader_log(inst, VULKAN_LOADER_DEBUG_BIT, 0, "Meta-layer %s component layer %s adding device extension %s", + prop->info.layerName, prop->component_layer_names.list[comp_layer], + comp_prop->device_extension_list.list[ext].props.extensionName); - for (uint32_t ext = 0; ext < comp_prop->device_extension_list.count; ext++) { - loader_log(inst, VULKAN_LOADER_DEBUG_BIT, 0, "Meta-layer %s component layer %s adding device extension %s", - prop->info.layerName, prop->component_layer_names.list[comp_layer], - comp_prop->device_extension_list.list[ext].props.extensionName); - - if (!has_vk_dev_ext_property(&comp_prop->device_extension_list.list[ext].props, &prop->device_extension_list)) { - loader_add_to_dev_ext_list(inst, &prop->device_extension_list, &comp_prop->device_extension_list.list[ext].props, - NULL); - if (VK_ERROR_OUT_OF_HOST_MEMORY == res) { - return res; + if (!has_vk_dev_ext_property(&comp_prop->device_extension_list.list[ext].props, &prop->device_extension_list)) { + loader_add_to_dev_ext_list(inst, &prop->device_extension_list, + &comp_prop->device_extension_list.list[ext].props, NULL); + if (VK_ERROR_OUT_OF_HOST_MEMORY == res) { + return res; + } } } } @@ -2918,7 +2922,7 @@ VkResult add_data_files(const struct loader_instance *inst, char *search_path, s loader_log(inst, VULKAN_LOADER_DEBUG_BIT, 0, "add_data_files: Path to %s too long", cur_file); continue; } - strcpy(temp_path, cur_file); + strncpy(temp_path, cur_file, str_len); name = temp_path; #else #warning add_data_files must define relative path copy for this platform @@ -3028,25 +3032,25 @@ VkResult read_data_files_in_search_paths(const struct loader_instance *inst, enu if (home != NULL) { if (NULL == xdg_config_home || '\0' == xdg_config_home[0]) { const char config_suffix[] = "/.config"; - default_config_home = - loader_instance_heap_alloc(inst, strlen(home) + strlen(config_suffix) + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); + size_t default_config_home_len = strlen(home) + sizeof(config_suffix) + 1; + default_config_home = loader_instance_heap_calloc(inst, default_config_home_len, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); if (default_config_home == NULL) { vk_result = VK_ERROR_OUT_OF_HOST_MEMORY; goto out; } - strcpy(default_config_home, home); - strcat(default_config_home, config_suffix); + strncpy(default_config_home, home, default_config_home_len); + strncat(default_config_home, config_suffix, default_config_home_len); } if (NULL == xdg_data_home || '\0' == xdg_data_home[0]) { const char data_suffix[] = "/.local/share"; - default_data_home = - loader_instance_heap_alloc(inst, strlen(home) + strlen(data_suffix) + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); + size_t default_data_home_len = strlen(home) + sizeof(data_suffix) + 1; + default_data_home = loader_instance_heap_calloc(inst, default_data_home_len, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); if (default_data_home == NULL) { vk_result = VK_ERROR_OUT_OF_HOST_MEMORY; goto out; } - strcpy(default_data_home, home); - strcat(default_data_home, data_suffix); + strncpy(default_data_home, home, default_data_home_len); + strncat(default_data_home, data_suffix, default_data_home_len); } } @@ -3166,7 +3170,7 @@ VkResult read_data_files_in_search_paths(const struct loader_instance *inst, enu // Add the remaining paths to the list if (NULL != override_path) { - strcpy(cur_path_ptr, override_path); + strncpy(cur_path_ptr, override_path, search_path_size); cur_path_ptr += strlen(override_path); } else { // Add any additional search paths defined in the additive environment variable @@ -3702,8 +3706,11 @@ VkResult loader_icd_scan(const struct loader_instance *inst, struct loader_icd_t } out: - for (uint32_t i = 0; i < manifest_files.count; i++) { - loader_instance_heap_free(inst, icd_details[i].full_library_path); + if (NULL != icd_details) { + // Successfully got the icd_details structure, which means we need to free the paths contained within + for (uint32_t i = 0; i < manifest_files.count; i++) { + loader_instance_heap_free(inst, icd_details[i].full_library_path); + } } free_string_list(inst, &manifest_files); return res; diff --git a/loader/settings.c b/loader/settings.c index fda2e3c..41ddb6b 100644 --- a/loader/settings.c +++ b/loader/settings.c @@ -208,12 +208,13 @@ VkResult check_if_settings_path_exists(const struct loader_instance* inst, char* if (NULL == base || NULL == suffix) { return VK_ERROR_INITIALIZATION_FAILED; } + *settings_file_path = loader_instance_heap_calloc(inst, strlen(base) + strlen(suffix) + 1, VK_SYSTEM_ALLOCATION_SCOPE_COMMAND); if (NULL == *settings_file_path) { return VK_ERROR_OUT_OF_HOST_MEMORY; } - strcpy(*settings_file_path, base); - strcat(*settings_file_path, suffix); + strncpy(*settings_file_path, base, strlen(base)); + strncat(*settings_file_path, suffix, strlen(suffix)); if (!loader_platform_file_exists(*settings_file_path)) { loader_instance_heap_free(inst, *settings_file_path); @@ -743,9 +744,10 @@ VkResult enable_correct_layers_from_settings(const struct loader_instance* inst, } if (!enable_layer && vk_instance_layers_env) { - char* name = loader_stack_alloc(strlen(vk_instance_layers_env) + 1); + 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) { - strcpy(name, vk_instance_layers_env); + strncpy(name, 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); diff --git a/loader/unknown_function_handling.c b/loader/unknown_function_handling.c index 65eb517..792a6bd 100644 --- a/loader/unknown_function_handling.c +++ b/loader/unknown_function_handling.c @@ -281,7 +281,6 @@ void *loader_phys_dev_ext_gpa_impl(struct loader_instance *inst, const char *fun new_function_index = inst->phys_dev_ext_disp_function_count; // increment the count so that the subsequent logic includes the newly added entry point when searching for functions inst->phys_dev_ext_disp_function_count++; - has_found = true; } // Setup the ICD function pointers diff --git a/tests/framework/test_environment.cpp b/tests/framework/test_environment.cpp index d423bca..718da1f 100644 --- a/tests/framework/test_environment.cpp +++ b/tests/framework/test_environment.cpp @@ -210,6 +210,7 @@ std::vector InstWrapper::GetPhysDevs(uint32_t phys_dev_count, std::vector InstWrapper::GetPhysDevs(VkResult result_to_check) { uint32_t physical_count = 0; VkResult res = functions->vkEnumeratePhysicalDevices(inst, &physical_count, nullptr); + EXPECT_EQ(result_to_check, res); std::vector physical_devices; physical_devices.resize(physical_count); res = functions->vkEnumeratePhysicalDevices(inst, &physical_count, physical_devices.data()); -- 2.7.4