From 221e4295788d3e5d2a8afe147c8127e8cf40732f Mon Sep 17 00:00:00 2001 From: jhendersonHDF Date: Mon, 26 Feb 2024 10:52:33 -0600 Subject: Fix memory leak in H5LTopen_file_image when H5LT_FILE_IMAGE_DONT_COPY flag is used (#4021) When the H5LT_FILE_IMAGE_DONT_COPY flag is passed to H5LTopen_file_image, the internally-allocated udata structure gets leaked as the core file driver doesn't have a way to determine when or if it needs to call the 'udata_free' callback. This has been fixed by freeing the udata structure when the 'image_free' callback gets made during file close, where the file is holding the last reference to the udata structure. --- hl/src/H5LT.c | 85 ++++++++++++++++++++++++++++++++++++++---------- release_docs/RELEASE.txt | 11 ++++++- src/H5Pfapl.c | 14 ++++++-- 3 files changed, 88 insertions(+), 22 deletions(-) diff --git a/hl/src/H5LT.c b/hl/src/H5LT.c index 6bd1337..099356f 100644 --- a/hl/src/H5LT.c +++ b/hl/src/H5LT.c @@ -286,14 +286,33 @@ image_realloc(void *ptr, size_t size, H5FD_file_image_op_t file_image_op, void * goto out; if (file_image_op == H5FD_FILE_IMAGE_OP_FILE_RESIZE) { + void *tmp_realloc; + if (udata->vfd_image_ptr != ptr) goto out; if (udata->vfd_ref_count != 1) goto out; - if (NULL == (udata->vfd_image_ptr = realloc(ptr, size))) + /* Make sure all the udata structure image pointers + * match each other before we update them + */ + assert(udata->vfd_image_ptr == udata->app_image_ptr); + assert(udata->vfd_image_ptr == udata->fapl_image_ptr); + + tmp_realloc = realloc(ptr, size); + if (tmp_realloc) { + udata->vfd_image_ptr = tmp_realloc; + udata->app_image_ptr = udata->vfd_image_ptr; + udata->fapl_image_ptr = udata->vfd_image_ptr; + } + else { + free(ptr); + udata->vfd_image_ptr = NULL; + udata->app_image_ptr = NULL; + udata->fapl_image_ptr = NULL; goto out; + } udata->vfd_image_size = size; return_value = udata->vfd_image_ptr; @@ -359,11 +378,20 @@ image_free(void *ptr, H5FD_file_image_op_t file_image_op, void *_udata) * references */ if (udata->fapl_ref_count == 0 && udata->vfd_ref_count == 0 && !(udata->flags & H5LT_FILE_IMAGE_DONT_RELEASE)) { + /* Make sure we aren't going to leak memory elsewhere */ + assert(udata->app_image_ptr == udata->vfd_image_ptr || udata->app_image_ptr == NULL); + assert(udata->fapl_image_ptr == udata->vfd_image_ptr || udata->fapl_image_ptr == NULL); + free(udata->vfd_image_ptr); udata->app_image_ptr = NULL; udata->fapl_image_ptr = NULL; udata->vfd_image_ptr = NULL; - } /* end if */ + } + + /* release reference to udata structure */ + if (udata_free(udata) < 0) + goto out; + break; /* added unused labels to keep the compiler quite */ @@ -437,9 +465,15 @@ udata_free(void *_udata) udata->ref_count--; - /* checks that there are no references outstanding before deallocating udata */ - if (udata->ref_count == 0 && udata->fapl_ref_count == 0 && udata->vfd_ref_count == 0) + if (udata->ref_count == 0) { + /* There should not be any outstanding references + * to the udata structure at this point. + */ + assert(udata->fapl_ref_count == 0); + assert(udata->vfd_ref_count == 0); + free(udata); + } return (SUCCEED); @@ -728,13 +762,13 @@ out: hid_t H5LTopen_file_image(void *buf_ptr, size_t buf_size, unsigned flags) { - hid_t fapl = -1, file_id = -1; /* HDF5 identifiers */ - unsigned file_open_flags; /* Flags for image open */ - char file_name[64]; /* Filename buffer */ - size_t alloc_incr; /* Buffer allocation increment */ - size_t min_incr = 65536; /* Minimum buffer increment */ - double buf_prcnt = 0.1; /* Percentage of buffer size to set - as increment */ + H5LT_file_image_ud_t *udata = NULL; /* Pointer to udata structure */ + hid_t fapl = -1, file_id = -1; /* HDF5 identifiers */ + unsigned file_open_flags; /* Flags for image open */ + char file_name[64]; /* Filename buffer */ + size_t alloc_incr; /* Buffer allocation increment */ + size_t min_incr = 65536; /* Minimum buffer increment */ + double buf_prcnt = 0.1; /* Percentage of buffer size to set as increment */ static long file_name_counter; H5FD_file_image_callbacks_t callbacks = {&image_malloc, &image_memcpy, &image_realloc, &image_free, &udata_copy, &udata_free, (void *)NULL}; @@ -765,13 +799,11 @@ H5LTopen_file_image(void *buf_ptr, size_t buf_size, unsigned flags) /* Set callbacks for file image ops ONLY if the file image is NOT copied */ if (flags & H5LT_FILE_IMAGE_DONT_COPY) { - H5LT_file_image_ud_t *udata; /* Pointer to udata structure */ - /* Allocate buffer to communicate user data to callbacks */ if (NULL == (udata = (H5LT_file_image_ud_t *)malloc(sizeof(H5LT_file_image_ud_t)))) goto out; - /* Initialize udata with info about app buffer containing file image and flags */ + /* Initialize udata with info about app buffer containing file image and flags */ udata->app_image_ptr = buf_ptr; udata->app_image_size = buf_size; udata->fapl_image_ptr = NULL; @@ -781,17 +813,32 @@ H5LTopen_file_image(void *buf_ptr, size_t buf_size, unsigned flags) udata->vfd_image_size = 0; udata->vfd_ref_count = 0; udata->flags = flags; - udata->ref_count = 1; /* corresponding to the first FAPL */ + + /* + * Initialize the udata structure with a reference count of 1. At + * first, nothing holds this reference to the udata structure. The + * call to H5Pset_file_image_callbacks below will associate the + * udata structure with the FAPL, incrementing the structure's + * reference count and causing the FAPL to hold one of the two + * references to the structure in preparation for transfer of + * ownership to the file driver. Once the file has been opened with + * this FAPL and the FAPL is closed, the reference held by the FAPL + * is released and ownership is transferred to the file driver, which + * will then hold the remaining reference to the udata structure. + * The udata structure will then be freed when the file driver calls + * the image_free callback and releases its reference to the structure. + */ + udata->ref_count = 1; /* copy address of udata into callbacks */ callbacks.udata = (void *)udata; /* Set file image callbacks */ if (H5Pset_file_image_callbacks(fapl, &callbacks) < 0) { - free(udata); + udata_free(udata); goto out; - } /* end if */ - } /* end if */ + } + } /* end if */ /* Assign file image in user buffer to FAPL */ if (H5Pset_file_image(fapl, buf_ptr, buf_size) < 0) @@ -821,8 +868,10 @@ out: H5E_BEGIN_TRY { H5Pclose(fapl); + H5Fclose(file_id); } H5E_END_TRY + return -1; } /* end H5LTopen_file_image() */ diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 4d1aabb..5c075d4 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -1145,7 +1145,16 @@ Bug Fixes since HDF5-1.14.0 release High-Level Library ------------------ - - + - Fixed a memory leak in H5LTopen_file_image with H5LT_FILE_IMAGE_DONT_COPY flag + + When the H5LT_FILE_IMAGE_DONT_COPY flag is passed to H5LTopen_file_image, the + internally-allocated udata structure gets leaked as the core file driver doesn't + have a way to determine when or if it needs to call the "udata_free" callback. + This has been fixed by freeing the udata structure when the "image_free" callback + gets made during file close, where the file is holding the last reference to the + udata structure. + + Fixes GitHub issue #827 Fortran High-Level APIs diff --git a/src/H5Pfapl.c b/src/H5Pfapl.c index e9496bf..6c2ef47 100644 --- a/src/H5Pfapl.c +++ b/src/H5Pfapl.c @@ -3159,9 +3159,10 @@ done: herr_t H5Pset_file_image_callbacks(hid_t fapl_id, H5FD_file_image_callbacks_t *callbacks_ptr) { - H5P_genplist_t *fapl; /* Property list pointer */ - H5FD_file_image_info_t info; /* File image info */ - herr_t ret_value = SUCCEED; /* Return value */ + H5P_genplist_t *fapl; /* Property list pointer */ + H5FD_file_image_info_t info; /* File image info */ + bool copied_udata = false; /* Whether udata structure was copied */ + herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_API(FAIL) H5TRACE2("e", "i*DI", fapl_id, callbacks_ptr); @@ -3209,11 +3210,18 @@ H5Pset_file_image_callbacks(hid_t fapl_id, H5FD_file_image_callbacks_t *callback HGOTO_ERROR(H5E_PLIST, H5E_CANTSET, FAIL, "can't copy the supplied udata"); } /* end if */ + copied_udata = true; + /* Set values */ if (H5P_poke(fapl, H5F_ACS_FILE_IMAGE_INFO_NAME, &info) < 0) HGOTO_ERROR(H5E_PLIST, H5E_CANTSET, FAIL, "can't set file image info"); done: + if (ret_value < 0) { + if (copied_udata && (callbacks_ptr->udata_free(info.callbacks.udata) < 0)) + HDONE_ERROR(H5E_RESOURCE, H5E_CANTFREE, FAIL, "udata_free callback failed"); + } + FUNC_LEAVE_API(ret_value) } /* end H5Pset_file_image_callbacks() */ -- cgit v0.12