Skip to content

Coverity Fixes for coll/tuned and coll/han/alltoallv #13187

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Apr 15, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 11 additions & 7 deletions ompi/mca/coll/han/coll_han_alltoallv.c
Original file line number Diff line number Diff line change
Expand Up @@ -804,6 +804,7 @@ int mca_coll_han_alltoallv_using_smsc(
low_gather_out = malloc(sizeof(*low_gather_out) * low_size);
struct peer_data *peers = malloc(sizeof(*peers) * low_size);
opal_datatype_t *peer_send_types = malloc(sizeof(*peer_send_types) * low_size);
bool have_bufs_and_types = false;

low_gather_in.serialization_buffer = serialization_buf;
low_gather_in.sbuf = (void*)sbuf; // cast to discard the const
Expand Down Expand Up @@ -896,6 +897,7 @@ int mca_coll_han_alltoallv_using_smsc(
peers[jrank].sendtype = &peer_send_types[jrank];
}

have_bufs_and_types = true;
send_from_addrs = malloc(sizeof(*send_from_addrs)*low_size);
recv_to_addrs = malloc(sizeof(*recv_to_addrs)*low_size);
send_counts = malloc(sizeof(*send_counts)*low_size);
Expand Down Expand Up @@ -964,14 +966,16 @@ int mca_coll_han_alltoallv_using_smsc(
free(recv_types);
}

for (int jlow=0; jlow<low_size; jlow++) {
if (jlow != low_rank) {
OBJ_DESTRUCT(&peer_send_types[jlow]);
}
if (have_bufs_and_types) {
for (int jlow=0; jlow<low_size; jlow++) {
if (jlow != low_rank) {
OBJ_DESTRUCT(&peer_send_types[jlow]);
}

for (int jbuf=0; jbuf<2; jbuf++) {
if (peers[jlow].map_ctx[jbuf]) {
mca_smsc->unmap_peer_region(peers[jlow].map_ctx[jbuf]);
for (int jbuf=0; jbuf<2; jbuf++) {
if (peers[jlow].map_ctx[jbuf]) {
mca_smsc->unmap_peer_region(peers[jlow].map_ctx[jbuf]);
}
}
}
}
Expand Down
6 changes: 3 additions & 3 deletions ompi/mca/coll/tuned/coll_tuned_component.c
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ mca_coll_tuned_module_construct(mca_coll_tuned_module_t *module)

int coll_tuned_alg_from_str(int collective_id, const char *alg_name, int *alg_value) {
int rc;
if (collective_id > COLLCOUNT || collective_id < 0) { return OPAL_ERROR; };
if (collective_id >= COLLCOUNT || collective_id < 0) { return OPAL_ERROR; };
rc = coll_tuned_algorithm_enums[collective_id]->value_from_string(
coll_tuned_algorithm_enums[collective_id],
alg_name, alg_value );
Expand All @@ -314,7 +314,7 @@ int coll_tuned_alg_from_str(int collective_id, const char *alg_name, int *alg_va
/* return the enum's value and string. caller's responsibility to free alg_string if NULL was not provided. */
int coll_tuned_alg_to_str(int collective_id, int alg_value, char **alg_string) {
int rc;
if (collective_id > COLLCOUNT || collective_id < 0) { return OPAL_ERROR; };
if (collective_id >= COLLCOUNT || collective_id < 0) { return OPAL_ERROR; };
rc = coll_tuned_algorithm_enums[collective_id]->string_from_value(
coll_tuned_algorithm_enums[collective_id],
alg_value, alg_string );
Expand All @@ -326,7 +326,7 @@ int coll_tuned_alg_register_options(int collective_id, mca_base_var_enum_t *opti
/* use the same enum used for mca parameters to allow tuning files to use
algorithm names rather than just numbers.*/
if (!options) { return OPAL_ERROR; }
if (collective_id > COLLCOUNT || collective_id < 0) {
if (collective_id >= COLLCOUNT || collective_id < 0) {
return OPAL_ERROR;
}

Expand Down
29 changes: 17 additions & 12 deletions ompi/mca/coll/tuned/coll_tuned_dynamic_file.c
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,11 @@ static int ompi_coll_tuned_read_rules_json (const opal_json_t *json_root, ompi_c

const opal_json_t *collectives_obj = NULL;
const opal_json_t *comm_rule_array = NULL;
const opal_json_t *comm_rule = NULL;
const opal_json_t *msg_size_array = NULL;

size_t num_collectives = 0;
size_t num_comm_rules;
size_t num_comm_rules = 0;
rc = opal_json_get_key(json_root, "collectives", &collectives_obj);
if (rc != OPAL_SUCCESS) {
opal_output_verbose(1, ompi_coll_tuned_stream,
Expand All @@ -269,15 +271,14 @@ static int ompi_coll_tuned_read_rules_json (const opal_json_t *json_root, ompi_c
opal_output_verbose(1, ompi_coll_tuned_stream,
"Internal json error when attempting to parse the collective at index %ld\n",
jcol);
goto error_bad_coll;
goto error_cleanup;
}
coll_id = mca_coll_base_name_to_colltype(coll_name);
if (coll_id < 0) {
opal_output_verbose(1, ompi_coll_tuned_stream,
"Unrecognized collective: \"%s\". Use all lowercase such as \"allgather\"",
coll_name);
opal_json_free(&comm_rule_array);
goto error_bad_coll;
goto error_cleanup;
}

alg_p = &alg_rules[coll_id];
Expand All @@ -287,14 +288,12 @@ static int ompi_coll_tuned_read_rules_json (const opal_json_t *json_root, ompi_c
opal_output_verbose(1, ompi_coll_tuned_stream,
"Problem parsing the collective at index %ld (for %s). Expected an array of comm-related rules.",
jcol, mca_coll_base_colltype_to_str(coll_id) );
goto error_bad_coll;
goto error_cleanup;
}
alg_p->n_com_sizes = (int)num_comm_rules;
alg_p->com_rules = ompi_coll_tuned_mk_com_rules (num_comm_rules, coll_id);

for (jcomm_rule=0; jcomm_rule < num_comm_rules; jcomm_rule++) {
const opal_json_t *comm_rule;
const opal_json_t *msg_size_array;
size_t num_msg_rules;
rc = opal_json_get_index(comm_rule_array, jcomm_rule, &comm_rule);
com_p = &(alg_p->com_rules[jcomm_rule]);
Expand Down Expand Up @@ -353,15 +352,16 @@ static int ompi_coll_tuned_read_rules_json (const opal_json_t *json_root, ompi_c
"Problem occurred within collective %s, "
"comm_size_min=%d, comm_size_max=%d (entry number %ld in the comm_size array), "
"message size entry number %ld.", coll_name, com_p->mpi_comsize_min, com_p->mpi_comsize_max, 1+jcomm_rule, 1+jmsg_rule);
opal_json_free(&collectives_obj);
return OMPI_ERROR;
goto error_cleanup;
error_bad_comm_rule:
opal_output_verbose(1, ompi_coll_tuned_stream,
"Problem occurred within collective %s, "
"in entry number %ld of the comm_size array", coll_name, 1+jcomm_rule);
opal_json_free(&collectives_obj);
return OMPI_ERROR;
error_bad_coll:
goto error_cleanup;
error_cleanup:
opal_json_free(&msg_size_array);
opal_json_free(&comm_rule);
opal_json_free(&comm_rule_array);
opal_json_free(&collectives_obj);
return OMPI_ERROR;
}
Expand Down Expand Up @@ -494,6 +494,11 @@ static int ompi_coll_tuned_read_rules_config_file_classic (char *fname, ompi_col
}
opal_output_verbose(25, ompi_coll_tuned_stream,
"Read communicator count %ld for dynamic rule for collective ID %ld\n", NCOMSIZES, COLID);
if( NCOMSIZES > INT_MAX) {
opal_output_verbose(25, ompi_coll_tuned_stream,
"Refusing to proceed: suspiciously large number found for the number of communicator-based rules: %ld\n", NCOMSIZES);
goto on_file_error;
}
alg_p->n_com_sizes = NCOMSIZES;
alg_p->com_rules = ompi_coll_tuned_mk_com_rules (NCOMSIZES, COLID);
if (NULL == alg_p->com_rules) {
Expand Down
2 changes: 1 addition & 1 deletion opal/util/json/opal_json.c
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ int opal_json_get_key_by_index(const opal_json_t *json, const size_t index, cons

json_object_entry entry = {0};

if (index < 0 || index >= in->value->u.object.length) {
if (index >= in->value->u.object.length) {
opal_show_help("help-json.txt", "Index out of bound", true, index,
in->value->u.array.length);
return ret;
Expand Down