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.

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() */