Skip to content

Commit

Permalink
coll/han: reorder free calls and avoid read-after-free in debug builds
Browse files Browse the repository at this point in the history 
Inside the main loop, whenever we read a new string we free it first.
The first iteration will be free(NULL), which is legal.
At the end, we free all strings in all paths.
This removes a potential read-after-free in a debug build and removes
some calls to free from the error paths.

Signed-off-by: Joseph Schuchart <[email protected]>
  • Loading branch information
@devreal
devreal committed Dec 19, 2022
1 parent fefae1a commit 0bb10c4
Showing 1 changed file with 13 additions and 25 deletions.
38 changes: 13 additions & 25 deletions ompi/mca/coll/han/coll_han_dynamic_file.c
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* -*- Mode: C; c-basic-offset:4 ; indent-tabs-mode:nil -*- */
/*
* Copyright (c) 2018-2020 The University of Tennessee and The University
* Copyright (c) 2018-2022 The University of Tennessee and The University
* of Tennessee Research Foundation. All rights
* reserved.
* Copyright (c) 2022 IBM Corporation. All rights reserved
Expand Down Expand Up @@ -65,6 +65,7 @@ mca_coll_han_init_dynamic_rules(void)
int algorithm_id;
char * coll_name = NULL;
char * algorithm_name = NULL;
char * target_comp_name = NULL;
collective_rule_t *coll_rules;

/* Topo information */
Expand Down Expand Up @@ -135,6 +136,7 @@ mca_coll_han_init_dynamic_rules(void)
mca_coll_han_component.dynamic_rules.nb_collectives = i+1;

/* Get the collective identifier */
free(coll_name);
if( getnext_string(fptr, &coll_name) < 0 ) {
opal_output_verbose(5, mca_coll_han_component.han_output,
"coll:han:mca_coll_han_init_dynamic_rules invalid collective at line %d."
Expand All @@ -155,9 +157,7 @@ mca_coll_han_init_dynamic_rules(void)
coll_name, fileline, ALLGATHER, COLLCOUNT);
goto file_reading_error;
}
if( NULL != coll_name ) {
free(coll_name);
}
free(coll_name);
coll_name = strdup(mca_coll_base_colltype_to_str(coll_id));
}

Expand Down Expand Up @@ -321,7 +321,6 @@ mca_coll_han_init_dynamic_rules(void)

/* Iterate on message size rules */
for( l = 0; l < nb_msg_size; l++ ) {
char* target_comp_name = NULL;
conf_rules[k].nb_msg_size = l+1;

/* Get the message size */
Expand All @@ -338,6 +337,7 @@ mca_coll_han_init_dynamic_rules(void)
}

/* Get the component identifier for this message size rule */
free(target_comp_name);
if( getnext_string(fptr, &target_comp_name) < 0 ) {
opal_output_verbose(5, mca_coll_han_component.han_output,
"coll:han:mca_coll_han_init_dynamic_rules found an error on dynamic rules file %s "
Expand All @@ -353,38 +353,32 @@ mca_coll_han_init_dynamic_rules(void)
"reader encountered an unexpected EOF. Collective component id must be at "
"least %d and less than %d\n",
fname, fileline, target_comp_name, SELF, COMPONENTS_COUNT);
free(target_comp_name);
target_comp_name = NULL;
goto file_reading_error;
}

/* Get the optionnal algorithm for han */
algorithm_id = 0; // default for all collectives
if ((component == HAN) && (1 == ompi_coll_base_file_peek_next_char_is(fptr, &fileline, '@')) ) {

free(algorithm_name);
algorithm_name = NULL;
if( getnext_string(fptr, &algorithm_name) < 0 ) {
opal_output_verbose(5, mca_coll_han_component.han_output,
"coll:han:mca_coll_han_init_dynamic_rules found an error on dynamic rules file %s "
"at line %d: cannot read the name/id of an algorithm\n",
fname, fileline);
free(target_comp_name);
target_comp_name = NULL;
goto file_reading_error;
}
algorithm_id = mca_coll_han_algorithm_name_to_id(coll_id, algorithm_name);
if (algorithm_id < 0) {
char *endp;
algorithm_id = (int)strtol(algorithm_name, &endp, 10);
char endc = *endp;
free(algorithm_name);
algorithm_name = NULL;
if (('\0' != endc ) || !mca_coll_han_algorithm_id_is_valid(coll_id, algorithm_id)) {
opal_output_verbose(5, mca_coll_han_component.han_output,
"coll:han:mca_coll_han_init_dynamic_rules found an error on dynamic rules file %s "
"at line %d: unknown algorithm '%s' for %s\n",
fname, fileline, algorithm_name, coll_name);
free(target_comp_name);
target_comp_name = NULL;
goto file_reading_error;
}
}
Expand Down Expand Up @@ -422,19 +416,13 @@ mca_coll_han_init_dynamic_rules(void)
"file %s line %d found end of file while reading the optional list "
"of segment lengths for collective %s component %s\n",
fname, fileline, coll_name, target_comp_name);
free(target_comp_name);
goto file_reading_error;
}
}
}
free(target_comp_name);
}
}
}
if( NULL != coll_name ) {
free(coll_name);
coll_name = NULL;
}
}

if( getnext_long(fptr, &nb_coll) > 0 ) {
Expand All @@ -455,7 +443,9 @@ mca_coll_han_init_dynamic_rules(void)
fclose(fptr);

check_dynamic_rules();
free(coll_name);
free(algorithm_name);
free(target_comp_name);
return OMPI_SUCCESS;

cannot_allocate:
Expand All @@ -465,10 +455,9 @@ mca_coll_han_init_dynamic_rules(void)
opal_output_verbose(0, mca_coll_han_component.han_output,
"coll:han:mca_coll_han_init_dynamic_rules "
"cannot allocate dynamic rules\n");
if( NULL != coll_name ) {
free(coll_name);
}
free(coll_name);
free(algorithm_name);
free(target_comp_name);
fclose (fptr);
/* We disable the module, we don't need to keep the rules */
mca_coll_han_free_dynamic_rules();
Expand All @@ -481,10 +470,9 @@ mca_coll_han_init_dynamic_rules(void)
"Will use mca parameters defined rules. "
"To see error detail, please set "
"collective verbosity level over 5\n");
if( NULL != coll_name ) {
free(coll_name);
}
free(coll_name);
free(algorithm_name);
free(target_comp_name);
fclose (fptr);
/* We disable the module, we don't need to keep the rules */
mca_coll_han_free_dynamic_rules();
Expand Down

0 comments on commit 0bb10c4

Please sign in to comment.