From cea735b3c32a66dc8859b6c9f4a80953b344fa38 Mon Sep 17 00:00:00 2001 From: Nathan Hjelm Date: Thu, 21 May 2015 13:33:31 -0600 Subject: [PATCH] mca/base: fix coverity issues and enable project name in MCA groups CID 1047278 Unchecked return value Updated check for mca_base_var_generate_full_name4 to match other checks. Logically equivalent to the old check. Not a bug. CID 1196685 Dereference null return Added check for NULL when looking up the original variable for a synonym. CID 1269705 Logically dead code Removed code that set the project to NULL. Code was intended to be removed with an earlier commit that added the project name into the component structure. Added code to actually support searching for a group with a wildcard ('*'). CID 1292739 Dereference null return CID 1269819 Dereference null return Removed unnecessary string duplication and strchr. CID 1287030 Logically dead code Refactored fixup_files code and confirmed that the code in question is not reachable. Removed the dead code. CID 1292740 Use of untrusted string Use strdup to silence coverity warning. CID 1294413 Free of address-of expression Reset mitem to NULL after the OPAL_LIST_FOREACH loop to ensure we never try to free the list sentinel. CID 1294414 Unchecked return value Use (void) to indicate we do not care about the return code in this instance. CID 1294415 Resource leak On error free all the base pointer. Signed-off-by: Nathan Hjelm --- opal/mca/base/mca_base_component_repository.c | 8 +- opal/mca/base/mca_base_components_close.c | 3 +- opal/mca/base/mca_base_pvar.c | 4 +- opal/mca/base/mca_base_var.c | 185 +++++++++--------- opal/mca/base/mca_base_var_group.c | 46 ++++- 5 files changed, 138 insertions(+), 108 deletions(-) diff --git a/opal/mca/base/mca_base_component_repository.c b/opal/mca/base/mca_base_component_repository.c index 9bcd3e62ea..b2eb70bf58 100644 --- a/opal/mca/base/mca_base_component_repository.c +++ b/opal/mca/base/mca_base_component_repository.c @@ -125,6 +125,7 @@ static int process_repository_item (const char *filename, void *data) ri = OBJ_NEW(mca_base_component_repository_item_t); if (NULL == ri) { + free (base); return OPAL_ERR_OUT_OF_RESOURCE; } @@ -227,14 +228,14 @@ int mca_base_component_repository_init(void) OBJ_CONSTRUCT(&mca_base_component_repository, opal_hash_table_t); ret = opal_hash_table_init (&mca_base_component_repository, 128); if (OPAL_SUCCESS != ret) { - mca_base_framework_close (&opal_dl_base_framework); + (void) mca_base_framework_close (&opal_dl_base_framework); return ret; } ret = mca_base_component_repository_add (mca_base_component_path); if (OPAL_SUCCESS != ret) { OBJ_DESTRUCT(&mca_base_component_repository); - mca_base_framework_close (&opal_dl_base_framework); + (void) mca_base_framework_close (&opal_dl_base_framework); return ret; } #endif @@ -326,6 +327,9 @@ int mca_base_component_repository_open (mca_base_framework_t *framework, } } + /* silence coverity issue (invalid free) */ + mitem = NULL; + if (NULL != ri->ri_dlhandle) { opal_output_verbose(40, 0, "mca_base_component_repository_open: already loaded. returning cached component"); mitem = OBJ_NEW(mca_base_component_list_item_t); diff --git a/opal/mca/base/mca_base_components_close.c b/opal/mca/base/mca_base_components_close.c index 232b0a38b3..ed0fd380cb 100644 --- a/opal/mca/base/mca_base_components_close.c +++ b/opal/mca/base/mca_base_components_close.c @@ -37,8 +37,7 @@ void mca_base_component_unload (const mca_base_component_t *component, int outpu "mca: base: close: unloading component %s", component->mca_component_name); - /* XXX -- TODO -- Replace reserved by mca_project_name for 1.9 */ - ret = mca_base_var_group_find (component->reserved, component->mca_type_name, + ret = mca_base_var_group_find (component->mca_project_name, component->mca_type_name, component->mca_component_name); if (0 <= ret) { mca_base_var_group_deregister (ret); diff --git a/opal/mca/base/mca_base_pvar.c b/opal/mca/base/mca_base_pvar.c index 863bf0b287..8d4758c5ed 100644 --- a/opal/mca/base/mca_base_pvar.c +++ b/opal/mca/base/mca_base_pvar.c @@ -260,8 +260,8 @@ int mca_base_pvar_register (const char *project, const char *framework, const ch do { /* generate the variable's full name */ - mca_base_var_generate_full_name4 (NULL, framework, component, name, &pvar->name); - if (NULL == pvar->name) { + ret = mca_base_var_generate_full_name4 (NULL, framework, component, name, &pvar->name); + if (OPAL_SUCCESS != ret) { ret = OPAL_ERR_OUT_OF_RESOURCE; break; } diff --git a/opal/mca/base/mca_base_var.c b/opal/mca/base/mca_base_var.c index d60f813e24..f2a6f7a406 100644 --- a/opal/mca/base/mca_base_var.c +++ b/opal/mca/base/mca_base_var.c @@ -324,41 +324,46 @@ int mca_base_var_init(void) static void process_env_list(char *env_list, char ***argv, char sep) { - int i; char** tokens; - char* ptr; - char* param, *value; + char *ptr, *value; + tokens = opal_argv_split(env_list, (int)sep); - if (NULL != tokens) { - for (i = 0; NULL != tokens[i]; i++) { - if (NULL == (ptr = strchr(tokens[i], '='))) { - value = getenv(tokens[i]); - if (NULL != value) { - if (NULL != strchr(value, '=')) { - param = strdup(value); - value = strchr(param, '='); - *value = '\0'; - value++; - opal_setenv(param, value, true, argv); - free(param); - } else { - opal_setenv(tokens[i], value, true, argv); - } - } else { - opal_show_help("help-mca-var.txt", "incorrect-env-list-param", - true, tokens[i], env_list); - } - } else { - param = strdup(tokens[i]); - value = strchr(param, '='); - *value = '\0'; - value++; - opal_setenv(param, value, true, argv); - free(param); - } - } - opal_argv_free(tokens); + if (NULL == tokens) { + return; } + + for (int i = 0 ; NULL != tokens[i] ; ++i) { + if (NULL == (ptr = strchr(tokens[i], '='))) { + value = getenv(tokens[i]); + if (NULL == value) { + opal_show_help("help-mca-var.txt", "incorrect-env-list-param", + true, tokens[i], env_list); + break; + } + + /* duplicate the value to silence tainted string coverity issue */ + value = strdup (value); + if (NULL == value) { + /* out of memory */ + break; + } + + if (NULL != (ptr = strchr(value, '='))) { + *ptr = '\0'; + opal_setenv(value, ptr + 1, true, argv); + } else { + opal_setenv(tokens[i], value, true, argv); + } + + free (value); + } else { + *ptr = '\0'; + opal_setenv(tokens[i], ptr + 1, true, argv); + /* NTH: don't bother resetting ptr to = since the string will not be used again */ + } + } + + opal_argv_free(tokens); } int mca_base_var_process_env_list(char ***argv) @@ -1175,6 +1180,7 @@ static int fixup_files(char **file_list, char * path, bool rel_path_search, char char **search_path = NULL; char * tmp_file = NULL; char **argv = NULL; + char *rel_path; int mode = R_OK; /* The file exists, and we can read it */ int count, i, argc = 0; @@ -1182,83 +1188,64 @@ static int fixup_files(char **file_list, char * path, bool rel_path_search, char files = opal_argv_split(*file_list, sep); count = opal_argv_count(files); + rel_path = force_agg_path ? force_agg_path : cwd; + /* Read in reverse order, so we can preserve the original ordering */ for (i = 0 ; i < count; ++i) { - /* Absolute paths preserved */ - if ( opal_path_is_absolute(files[i]) ) { - if( NULL == (tmp_file = opal_path_access(files[i], NULL, mode)) ) { - opal_show_help("help-mca-var.txt", "missing-param-file", - true, getpid(), files[i], path); - exit_status = OPAL_ERROR; - goto cleanup; - } else { - opal_argv_append(&argc, &argv, files[i]); - } + char *msg_path = path; + if (opal_path_is_absolute(files[i])) { + /* Absolute paths preserved */ + tmp_file = opal_path_access(files[i], NULL, mode); + } else if (!rel_path_search && NULL != strchr(files[i], OPAL_PATH_SEP[0])) { + /* Resolve all relative paths: + * - If filename contains a "/" (e.g., "./foo" or "foo/bar") + * - look for it relative to cwd + * - if exists, use it + * - ow warn/error + */ + msg_path = rel_path; + tmp_file = opal_path_access(files[i], rel_path, mode); + } else { + /* Resolve all relative paths: + * - Use path resolution + * - if found and readable, use it + * - otherwise, warn/error + */ + tmp_file = opal_path_find (files[i], search_path, mode, NULL); } - /* Resolve all relative paths: - * - If filename contains a "/" (e.g., "./foo" or "foo/bar") - * - look for it relative to cwd - * - if exists, use it - * - ow warn/error - */ - else if (!rel_path_search && NULL != strchr(files[i], OPAL_PATH_SEP[0]) ) { - if( NULL != force_agg_path ) { - tmp_file = opal_path_access(files[i], force_agg_path, mode); - } - else { - tmp_file = opal_path_access(files[i], cwd, mode); - } - if( NULL == tmp_file ) { - opal_show_help("help-mca-var.txt", "missing-param-file", - true, getpid(), files[i], cwd); - exit_status = OPAL_ERROR; - goto cleanup; - } - else { - opal_argv_append(&argc, &argv, tmp_file); - } - } - /* Resolve all relative paths: - * - Use path resolution - * - if found and readable, use it - * - otherwise, warn/error - */ - else { - if( NULL != (tmp_file = opal_path_find(files[i], search_path, mode, NULL)) ) { - opal_argv_append(&argc, &argv, tmp_file); - } - else { - opal_show_help("help-mca-var.txt", "missing-param-file", - true, getpid(), files[i], path); - exit_status = OPAL_ERROR; - goto cleanup; - } + if (NULL == tmp_file) { + opal_show_help("help-mca-var.txt", "missing-param-file", + true, getpid(), files[i], msg_path); + exit_status = OPAL_ERROR; + break; } + + opal_argv_append(&argc, &argv, tmp_file); + free(tmp_file); + tmp_file = NULL; } - tmp_file = NULL; + if (OPAL_SUCCESS == exit_status) { + free(*file_list); + *file_list = opal_argv_join(argv, sep); + } - free(*file_list); - *file_list = opal_argv_join(argv, sep); - - cleanup: if( NULL != files ) { opal_argv_free(files); files = NULL; } + if( NULL != argv ) { opal_argv_free(argv); argv = NULL; } + if( NULL != search_path ) { opal_argv_free(search_path); search_path = NULL; } - if( NULL != tmp_file ) { - free(tmp_file); - } return exit_status; } @@ -1266,7 +1253,7 @@ static int fixup_files(char **file_list, char * path, bool rel_path_search, char static int read_files(char *file_list, opal_list_t *file_values, char sep) { char **tmp = opal_argv_split(file_list, sep); - int i, count, ret; + int i, count; if (!tmp) { return OPAL_ERR_OUT_OF_RESOURCE; @@ -1349,6 +1336,16 @@ static int register_variable (const char *project_name, const char *framework_na } #endif + if (flags & MCA_BASE_VAR_FLAG_SYNONYM) { + original = opal_pointer_array_get_item (&mca_base_vars, synonym_for); + if (NULL == original) { + /* Attempting to create a synonym for a non-existent variable. probably a + * developer error. */ + assert (NULL != original); + return OPAL_ERR_NOT_FOUND; + } + } + /* There are data holes in the var struct */ OPAL_DEBUG_ZERO(var); @@ -1494,17 +1491,16 @@ static int register_variable (const char *project_name, const char *framework_na var->mbv_enumerator = enumerator; - if (flags & MCA_BASE_VAR_FLAG_SYNONYM) { - original = opal_pointer_array_get_item (&mca_base_vars, synonym_for); - - opal_value_array_append_item(&original->mbv_synonyms, &var_index); - } else { + if (!original) { var->mbv_storage = storage; /* make a copy of the default string value */ if ((MCA_BASE_VAR_TYPE_STRING == type || MCA_BASE_VAR_TYPE_VERSION_STRING == type) && NULL != ((char **)storage)[0]) { ((char **)storage)[0] = strdup (((char **)storage)[0]); } + } else { + /* synonym variable */ + opal_value_array_append_item(&original->mbv_synonyms, &var_index); } /* go ahead and mark this variable as valid */ @@ -1718,7 +1714,6 @@ static int var_set_from_file (mca_base_var_t *var, mca_base_var_t *original, opa bool deprecated = VAR_IS_DEPRECATED(var[0]); bool is_synonym = VAR_IS_SYNONYM(var[0]); mca_base_var_file_value_t *fv; - int ret; /* Scan through the list of values read in from files and try to find a match. If we do, cache it on the param (for future diff --git a/opal/mca/base/mca_base_var_group.c b/opal/mca/base/mca_base_var_group.c index fa711e374b..64f9defd6f 100644 --- a/opal/mca/base/mca_base_var_group.c +++ b/opal/mca/base/mca_base_var_group.c @@ -144,6 +144,40 @@ static int group_find_by_name (const char *full_name, int *index, bool invalidok return OPAL_ERR_NOT_FOUND; } +static bool compare_strings (const char *str1, const char *str2) { + if ((NULL != str1 && 0 == strcmp (str1, "*")) || + (NULL == str1 && NULL == str2)) { + return true; + } + + if (NULL != str1 && NULL != str2) { + return 0 == strcmp (str1, str2); + } + + return false; +} + +static int group_find_linear (const char *project_name, const char *framework_name, + const char *component_name, bool invalidok) +{ + for (int i = 0 ; i < mca_base_var_group_count ; ++i) { + mca_base_var_group_t *group; + + int rc = mca_base_var_group_get_internal (i, &group, invalidok); + if (OPAL_SUCCESS != rc) { + continue; + } + + if (compare_strings (project_name, group->group_project) && + compare_strings (framework_name, group->group_framework) && + compare_strings (component_name, group->group_component)) { + return i; + } + } + + return OPAL_ERR_NOT_FOUND; +} + static int group_find (const char *project_name, const char *framework_name, const char *component_name, bool invalidok) { @@ -154,8 +188,11 @@ static int group_find (const char *project_name, const char *framework_name, return OPAL_ERR_NOT_FOUND; } - /* TODO -- deal with the project name correctly (including the wildcard '*') */ - project_name = NULL; + /* check for wildcards */ + if ((project_name && '*' == project_name[0]) || (framework_name && '*' == framework_name[0]) || + (component_name && '*' == component_name[0])) { + return group_find_linear (project_name, framework_name, component_name, invalidok); + } ret = mca_base_var_generate_full_name4(project_name, framework_name, component_name, NULL, &full_name); @@ -181,11 +218,6 @@ static int group_register (const char *project_name, const char *framework_name, return -1; } - /* XXX -- remove this once the project name is available in the component structure */ - if (framework_name || component_name) { - project_name = NULL; - } - group_id = group_find (project_name, framework_name, component_name, true); if (0 <= group_id) { ret = mca_base_var_group_get_internal (group_id, &group, true);