From 9e56ef0da9cc9366a1cf73e6fc868cefce415701 Mon Sep 17 00:00:00 2001 From: Nathan Hjelm Date: Tue, 26 May 2015 13:30:04 -0600 Subject: [PATCH] opal/crs: clean up parsing code to fix coverity issues CID 70622 Dereference before null check (REVERSE_INULL) CID 70459 Logically dead code (DEADCODE) Cleanup some cludgy code which (among other things) reimplemented strcat, strdup, and strchr. In the process this resolved two outstanding coverity issues. CID 70631 Dereference before null check (REVERSE_INULL) best_module can not be NULL in this code path. Remove NULL check and unnecessary goto statements. Signed-off-by: Nathan Hjelm --- opal/mca/crs/base/crs_base_fns.c | 147 ++++++++++++---------------- opal/mca/crs/base/crs_base_select.c | 20 ++-- 2 files changed, 73 insertions(+), 94 deletions(-) diff --git a/opal/mca/crs/base/crs_base_fns.c b/opal/mca/crs/base/crs_base_fns.c index bfdf4f947d8..1363c6cf4d0 100644 --- a/opal/mca/crs/base/crs_base_fns.c +++ b/opal/mca/crs/base/crs_base_fns.c @@ -1,3 +1,4 @@ +/* -*- Mode: C; c-basic-offset:4 ; indent-tabs-mode:nil -*- */ /* * Copyright (c) 2004-2010 The Trustees of Indiana University. * All rights reserved. @@ -11,6 +12,8 @@ * Copyright (c) 2015 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. + * Copyright (c) 2015 Los Alamos National Security, LLC. All rights + * reserved. * $COPYRIGHT$ * * Additional copyrights may follow @@ -359,122 +362,98 @@ int opal_crs_base_self_register_continue_callback(opal_crs_base_self_continue_fn static int metadata_extract_next_token(FILE *file, char **token, char **value) { int exit_status = OPAL_SUCCESS; - int max_len = 256; - char * line = NULL; - int line_len = 0; - int c = 0, s = 0, v = 0; - char *local_token = NULL; + const int max_len = 256; + /* NTH: as long as max_len remains small (256 bytes) there is no need + * to allocate line on the heap */ + char line[max_len]; + int line_len = 0, value_len; char *local_value = NULL; bool end_of_line = false; + char *tmp; - line = (char *) malloc(sizeof(char) * max_len); - - try_again: /* * If we are at the end of the file, then just return */ - if(0 != feof(file) ) { - exit_status = OPAL_ERROR; - goto cleanup; - } + do { + /* + * Other wise grab the next token/value pair + */ + if (NULL == fgets(line, max_len, file) ) { + /* the calling code doesn't distinguish error types so + * returning OPAL_ERROR on error or EOF is ok. if this + * changes re-add the check for EOF. */ + return OPAL_ERROR; + } - /* - * Other wise grab the next token/value pair - */ - if (NULL == fgets(line, max_len, file) ) { - exit_status = OPAL_ERROR; - goto cleanup; - } - line_len = strlen(line); - /* Strip off the new line if it it there */ - if('\n' == line[line_len-1]) { - line[line_len-1] = '\0'; - line_len--; - end_of_line = true; - } - else { - end_of_line = false; - } + line_len = strlen(line); - /* Ignore lines with just '#' too */ - if(2 >= line_len) - goto try_again; + /* Strip off the new line if it is there */ + end_of_line = ('\n' == line[line_len-1]); + + if (end_of_line) { + line[--line_len] = '\0'; + } + + /* Ignore lines with just '#' too */ + } while (line_len <= 2); /* * Extract the token from the set */ - for(c = 0; - line[c] != ':' && - c < line_len; - ++c) { - ; + tmp = strchr (line, ':'); + if (!tmp) { + /* no separator */ + return OPAL_ERROR; } - c += 2; /* For the ' ' and the '\0' */ - local_token = (char *)malloc(sizeof(char) * (c + 1)); - for(s = 0; s < c; ++s) { - local_token[s] = line[s]; - } + tmp = '\0'; + + *token = strdup (line); + local_value = strdup (tmp + 1); - local_token[s] = '\0'; - *token = strdup(local_token); + if (NULL == *token || NULL == local_value) { + if (local_value) { + free (local_value); + } - if( NULL != local_token) { - free(local_token); - local_token = NULL; + return OPAL_ERR_OUT_OF_RESOURCE; } + value_len = strlen (local_value) + 1; + /* * Extract the value from the set */ - local_value = (char *)malloc(sizeof(char) * (line_len - c + 1)); - for(v = 0, s = c; - s < line_len; - ++s, ++v) { - local_value[v] = line[s]; - } - while(!end_of_line) { if (NULL == fgets(line, max_len, file) ) { exit_status = OPAL_ERROR; - goto cleanup; + break; } + line_len = strlen(line); - /* Strip off the new line if it it there */ - if('\n' == line[line_len-1]) { - line[line_len-1] = '\0'; - line_len--; - end_of_line = true; - } - else { - end_of_line = false; + + /* Strip off the new line if it is there */ + end_of_line = ('\n' == line[line_len-1]); + + if (end_of_line) { + line[--line_len] = '\0'; } + + value_len += line_len; - local_value = (char *)realloc(local_value, sizeof(char) * line_len); - for(s = 0; - s < line_len; - ++s, ++v) { - local_value[v] = line[s]; + tmp = (char *) realloc(local_value, value_len); + if (NULL == tmp) { + exit_status = OPAL_ERR_OUT_OF_RESOURCE; + break; } - } - - local_value[v] = '\0'; - *value = strdup(local_value); - cleanup: - if( NULL != local_token) { - free(local_token); - local_token = NULL; - } - - if( NULL != local_value) { - free(local_value); - local_value = NULL; + strcat (local_value, line); } - if( NULL != line) { - free(line); - line = NULL; + if (OPAL_SUCCESS == exit_status) { + *value = local_value; + } else { + free (local_value); } return exit_status; diff --git a/opal/mca/crs/base/crs_base_select.c b/opal/mca/crs/base/crs_base_select.c index 1a23daa8297..09d0e09b54f 100644 --- a/opal/mca/crs/base/crs_base_select.c +++ b/opal/mca/crs/base/crs_base_select.c @@ -1,3 +1,4 @@ +/* -*- Mode: C; c-basic-offset:4 ; indent-tabs-mode:nil -*- */ /* * Copyright (c) 2004-2010 The Trustees of Indiana University. * All rights reserved. @@ -8,6 +9,8 @@ * Copyright (c) 2004-2005 The Regents of the University of California. * All rights reserved. * Copyright (c) 2007 Evergrid, Inc. All rights reserved. + * Copyright (c) 2015 Los Alamos National Security, LLC. All rights + * reserved. * * $COPYRIGHT$ * @@ -33,9 +36,9 @@ bool opal_crs_base_do_not_select = false; int opal_crs_base_select(void) { - int ret, exit_status = OPAL_SUCCESS; opal_crs_base_component_t *best_component = NULL; opal_crs_base_module_t *best_module = NULL; + int ret; if( !opal_cr_is_enabled ) { opal_output_verbose(10, opal_crs_base_framework.framework_output, @@ -57,22 +60,19 @@ int opal_crs_base_select(void) (mca_base_module_t **) &best_module, (mca_base_component_t **) &best_component) ) { /* This will only happen if no component was selected */ - exit_status = OPAL_ERROR; - goto cleanup; + return OPAL_ERROR; } + /* best_module and best_component should not be NULL here */ + /* Save the winner */ opal_crs_base_selected_component = *best_component; opal_crs = *best_module; /* Initialize the winner */ - if (NULL != best_module) { - if (OPAL_SUCCESS != (ret = opal_crs.crs_init()) ) { - exit_status = ret; - goto cleanup; - } + if (OPAL_SUCCESS != (ret = opal_crs.crs_init()) ) { + return ret; } - cleanup: - return exit_status; + return OPAL_SUCCESS; }