From 50d80121bbb49f82aeb6592225b82d819be3d08d Mon Sep 17 00:00:00 2001 From: Joseph Schuchart Date: Tue, 3 May 2022 10:21:35 -0400 Subject: [PATCH 1/2] MPI_Info_dup: allocate info through ompi_info_allocate instead of OBJ_NEW The call to ompi_info_allocate ensures that the ompi instance is properly retained (similar to MPI_Info_create). The instance is then released in MPI_Info_free. Thanks to Lisandro Dalcin for reporting and providing an easy reproducer. Signed-off-by: Joseph Schuchart --- ompi/mpi/c/info_dup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ompi/mpi/c/info_dup.c b/ompi/mpi/c/info_dup.c index bbe320d3955..f678c2738b0 100644 --- a/ompi/mpi/c/info_dup.c +++ b/ompi/mpi/c/info_dup.c @@ -75,7 +75,7 @@ int MPI_Info_dup(MPI_Info info, MPI_Info *newinfo) { } } - *newinfo = OBJ_NEW(ompi_info_t); + *newinfo = ompi_info_allocate(); if (NULL == *newinfo) { return OMPI_ERRHANDLER_NOHANDLE_INVOKE(MPI_ERR_NO_MEM, FUNC_NAME); From 7ef4f20760fc1fe3024972181fd5f536b99cd8a1 Mon Sep 17 00:00:00 2001 From: Joseph Schuchart Date: Tue, 3 May 2022 18:05:24 -0400 Subject: [PATCH 2/2] ROMIO: store temporary ompi_info_t on stack No need to allocate it on the heap using OBJ_NEW. Also fixes a mismatch between OBJ_NEW and ompi_info_free that potentially leads to inconsistencies in ref-counting the ompi instance. Signed-off-by: Joseph Schuchart --- .../io/romio341/src/io_romio341_component.c | 11 +++++----- .../io/romio341/src/io_romio341_file_open.c | 22 +++++++++---------- 2 files changed, 15 insertions(+), 18 deletions(-) diff --git a/ompi/mca/io/romio341/src/io_romio341_component.c b/ompi/mca/io/romio341/src/io_romio341_component.c index 8d94bbf027b..8149b085454 100644 --- a/ompi/mca/io/romio341/src/io_romio341_component.c +++ b/ompi/mca/io/romio341/src/io_romio341_component.c @@ -243,17 +243,16 @@ static int delete_select(const char *filename, struct opal_info_t *info, // An opal_info_t isn't a full ompi_info_t. so if we're using an MPI call // below with an MPI_Info, we need to create an equivalent MPI_Info. This // isn't ideal but it only happens a few places. - ompi_info_t *ompi_info; - ompi_info = OBJ_NEW(ompi_info_t); - if (!ompi_info) { return(MPI_ERR_NO_MEM); } - opal_info_t *opal_info = &(ompi_info->super); + ompi_info_t ompi_info; + OBJ_CONSTRUCT(&ompi_info, ompi_info_t); + opal_info_t *opal_info = &(ompi_info.super); opal_info_dup (info, &opal_info); OPAL_THREAD_LOCK (&mca_io_romio341_mutex); - ret = ROMIO_PREFIX(MPI_File_delete)(filename, ompi_info); + ret = ROMIO_PREFIX(MPI_File_delete)(filename, &ompi_info); OPAL_THREAD_UNLOCK (&mca_io_romio341_mutex); - ompi_info_free(&ompi_info); + OBJ_DESTRUCT(&ompi_info); return ret; } diff --git a/ompi/mca/io/romio341/src/io_romio341_file_open.c b/ompi/mca/io/romio341/src/io_romio341_file_open.c index 587f56d3302..0deb291c563 100644 --- a/ompi/mca/io/romio341/src/io_romio341_file_open.c +++ b/ompi/mca/io/romio341/src/io_romio341_file_open.c @@ -41,19 +41,18 @@ mca_io_romio341_file_open (ompi_communicator_t *comm, // An opal_info_t isn't a full ompi_info_t. so if we're using an MPI call // below with an MPI_Info, we need to create an equivalent MPI_Info. This // isn't ideal but it only happens a few places. - ompi_info_t *ompi_info; - ompi_info = OBJ_NEW(ompi_info_t); - if (!ompi_info) { return(MPI_ERR_NO_MEM); } - opal_info_t *opal_info = &(ompi_info->super); + ompi_info_t ompi_info; + OBJ_CONSTRUCT(&ompi_info, ompi_info_t); + opal_info_t *opal_info = &(ompi_info.super); opal_info_dup (info, &opal_info); data = (mca_io_romio341_data_t *) fh->f_io_selected_data; // OPAL_THREAD_LOCK (&mca_io_romio341_mutex); - ret = ROMIO_PREFIX(MPI_File_open)(comm, filename, amode, ompi_info, + ret = ROMIO_PREFIX(MPI_File_open)(comm, filename, amode, &ompi_info, &data->romio_fh); // OPAL_THREAD_UNLOCK (&mca_io_romio341_mutex); - ompi_info_free(&ompi_info); + OBJ_DESTRUCT(&ompi_info); return ret; } @@ -206,20 +205,19 @@ mca_io_romio341_file_set_view (ompi_file_t *fh, // An opal_info_t isn't a full ompi_info_t. so if we're using an MPI call // below with an MPI_Info, we need to create an equivalent MPI_Info. This // isn't ideal but it only happens a few places. - ompi_info_t *ompi_info; - ompi_info = OBJ_NEW(ompi_info_t); - if (!ompi_info) { return(MPI_ERR_NO_MEM); } - opal_info_t *opal_info = &(ompi_info->super); + ompi_info_t ompi_info; + OBJ_CONSTRUCT(&ompi_info, ompi_info_t); + opal_info_t *opal_info = &(ompi_info.super); opal_info_dup (info, &opal_info); data = (mca_io_romio341_data_t *) fh->f_io_selected_data; OPAL_THREAD_LOCK (&mca_io_romio341_mutex); ret = ROMIO_PREFIX(MPI_File_set_view) (data->romio_fh, disp, etype, filetype, - datarep, ompi_info); + datarep, &ompi_info); OPAL_THREAD_UNLOCK (&mca_io_romio341_mutex); - ompi_info_free(&ompi_info); + OBJ_DESTRUCT(&ompi_info); return ret; }