Fix for github issue HDFGroup#3790: infinite loop closing library (HD…

…FGroup#4445)

* Fix for github issue HDFGroup#3790: infinite loop closing library
Cause of the problem:
When h5dump tries to open the user provided test file, the metadata cache will
call the "get_final_load_size" callback to find out the actual size of the
the root object header.  The callback function will call
H5O__prefix_deserialize() to allocate space for the object header
data structure (via H5FL_CALLOC) and to deserialize the object header prefix
in order to find the actual size of the object header.
The metadata cache will then check whether the actual size obtained
will exceed the file's EOA.
Since the actual size obtained from the test file exceeds the EOA,
the metadata cache throws an error and return.
However, the oh structure that was allocated in H5O__prefix_deserialize()
was not freed and hence causing the problem described in this issue.
Fix:
1) Deallocate the oh structure after obtaining and saving the needed
information in udata which will be used later on in the "verify_chksum" callback.
2) Deserialize the object header prefix in the "object header's
"deserialize" callback regardless.  The original coding intends to keep the
deserialized prefix so that the object header's "deserialize" callback
does not need to deserialize the prefix again if the object header is coming
through the "get_final_load_size" callback.

release_docs/RELEASE.txt

@@ -747,6 +747,20 @@ Bug Fixes since HDF5-1.14.0 release
 
     Library
     -------
+    - Fixed infinite loop closing library issue when h5dump with a user provided test file
+
+      The library's metadata cache calls the "get_final_load_size" client callback
+      to find out the actual size of the object header.  As the size obtained
+      exceeds the file's EOA, it throws an error but the object header structure
+      allocated through the client callback is not freed hence causing the 
+      issue described.
+
+      (1) Free the structure allocated in the object header client callback after 
+      saving the needed information in udata. (2) Deserialize the object header
+      prefix in the object header's "deserialize" callback regardless.
+
+      Fixes GitHub #3790
+
     - Fixed many (future) CVE issues
 
       A partner organization corrected many potential security issues, which

src/H5Ocache.c

@@ -190,6 +190,15 @@ H5O__cache_get_final_load_size(const void *image, size_t image_len, void *_udata
     /* Set the final size for the cache image */
     *actual_len = udata->chunk0_size + (size_t)H5O_SIZEOF_HDR(udata->oh);
 
+    /* Save the oh version to be used later in verify_chksum callback
+       because oh will be freed before leaving this routine */
+    udata->oh_version = udata->oh->version;
+
+    /* Free allocated memory: fix github issue #3970 */
+    if (H5O__free(udata->oh, false) < 0)
+        HGOTO_ERROR(H5E_OHDR, H5E_CANTRELEASE, FAIL, "can't destroy object header");
+    udata->oh = NULL;
+
 done:
     FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5O__cache_get_final_load_size() */
@@ -215,27 +224,17 @@ H5O__cache_verify_chksum(const void *_image, size_t len, void *_udata)
 
     assert(image);
     assert(udata);
-    assert(udata->oh);
 
     /* There is no checksum for version 1 */
-    if (udata->oh->version != H5O_VERSION_1) {
+    if (udata->oh_version != H5O_VERSION_1) {
         uint32_t stored_chksum;   /* Stored metadata checksum value */
         uint32_t computed_chksum; /* Computed metadata checksum value */
 
         /* Get stored and computed checksums */
         H5F_get_checksums(image, len, &stored_chksum, &computed_chksum);
 
-        if (stored_chksum != computed_chksum) {
-            /* These fields are not deserialized yet in H5O__prefix_deserialize() */
-            assert(udata->oh->chunk == NULL);
-            assert(udata->oh->mesg == NULL);
-            assert(udata->oh->proxy == NULL);
-
-            /* Indicate that udata->oh is to be freed later
-               in H5O__prefix_deserialize() */
-            udata->free_oh = true;
-            ret_value      = false;
-        }
+        if (stored_chksum != computed_chksum)
+            ret_value = false;
     }
     else
         assert(!(udata->common.file_intent & H5F_ACC_SWMR_WRITE));
@@ -275,21 +274,12 @@ H5O__cache_deserialize(const void *image, size_t len, void *_udata, bool *dirty)
     assert(udata->common.f);
     assert(udata->common.cont_msg_info);
     assert(dirty);
+    assert(udata->oh == NULL);
 
-    /* Check for partially deserialized object header
-     *
-     * The Object header prefix will be deserialized if the object header came
-     * through the 'get_final_load_size' callback and not deserialized if
-     * the object header is coming from a cache image.
-     */
-    if (NULL == udata->oh) {
-        /* Deserialize the object header prefix */
-        if (H5O__prefix_deserialize((const uint8_t *)image, len, udata) < 0)
-            HGOTO_ERROR(H5E_OHDR, H5E_CANTDECODE, NULL, "can't deserialize object header prefix");
-        assert(udata->oh);
-    }
+    if (H5O__prefix_deserialize((const uint8_t *)image, len, udata) < 0)
+        HGOTO_ERROR(H5E_OHDR, H5E_CANTDECODE, NULL, "can't deserialize object header prefix");
+    assert(udata->oh);
 
-    /* Retrieve partially deserialized object header from user data */
     oh = udata->oh;
 
     /* Set SWMR flag, if appropriate */
@@ -1144,25 +1134,9 @@ H5O__prefix_deserialize(const uint8_t *_image, size_t len, H5O_cache_ud_t *udata
     if ((size_t)(image - _image) != (size_t)(H5O_SIZEOF_HDR(oh) - H5O_SIZEOF_CHKSUM_OH(oh)))
         HGOTO_ERROR(H5E_OHDR, H5E_BADVALUE, FAIL, "bad object header prefix length");
 
-    /* If udata->oh is to be freed (see H5O__cache_verify_chksum),
-     * save the pointer to udata->oh and free it later after setting
-     * udata->oh with the new object header
-     */
-    if (udata->free_oh) {
-        H5O_t *saved_oh = udata->oh;
-        assert(udata->oh);
-
-        /* Save the object header for later use in 'deserialize' callback */
-        udata->oh = oh;
-        if (H5O__free(saved_oh, false) < 0)
-            HGOTO_ERROR(H5E_OHDR, H5E_CANTRELEASE, FAIL, "can't destroy object header");
-        udata->free_oh = false;
-    }
-    else
-        /* Save the object header for later use in 'deserialize' callback */
-        udata->oh = oh;
-
-    oh = NULL;
+    /* Save the object header for later use in 'deserialize' callback */
+    udata->oh = oh;
+    oh        = NULL;
 
 done:
     /* Release the [possibly partially initialized] object header on errors */

src/H5Oint.c

@@ -976,7 +976,7 @@ H5O_protect(const H5O_loc_t *loc, unsigned prot_flags, bool pin_all_chunks)
     udata.v1_pfx_nmesgs           = 0;
     udata.chunk0_size             = 0;
     udata.oh                      = NULL;
-    udata.free_oh                 = false;
+    udata.oh_version              = 0;
     udata.common.f                = loc->file;
     udata.common.file_intent      = file_intent;
     udata.common.merged_null_msgs = 0;

src/H5Opkg.h

@@ -366,7 +366,7 @@ typedef struct H5O_cache_ud_t {
     unsigned              v1_pfx_nmesgs; /* Number of messages from v1 prefix header */
     size_t                chunk0_size;   /* Size of serialized first chunk    */
     H5O_t                *oh;            /* Partially deserialized object header, for later use */
-    bool                  free_oh;       /* Whether to free the object header or not */
+    uint8_t               oh_version;    /* Oh version number          */
     H5O_common_cache_ud_t common;        /* Common object header cache callback info */
 } H5O_cache_ud_t;
 

tools/test/h5dump/CMakeTests.cmake

@@ -25,6 +25,7 @@
       file_space.ddl
       filter_fail.ddl
       non_existing.ddl
+      infinite_loop.ddl
       packedbits.ddl
       tall-1.ddl
       tall-2.ddl
@@ -298,6 +299,7 @@
       tfpformat.h5
       tfvalues.h5
       tgroup.h5
+      3790_infinite_loop.h5
       tgrp_comments.h5
       tgrpnullspace.h5
       thlink.h5
@@ -371,6 +373,7 @@
   set (HDF5_ERROR_REFERENCE_TEST_FILES
       filter_fail.err
       non_existing.err
+      infinite_loop.err
       tall-1.err
       tall-2A.err
       tall-2A0.err
@@ -1423,6 +1426,9 @@
   # test for non-existing file
   ADD_H5_TEST (non_existing 1 --enable-error-stack tgroup.h5 non_existing.h5)
 
+  # test to verify github issue#3790: infinite loop closing library
+  ADD_H5_TEST (infinite_loop 1 3790_infinite_loop.h5)
+
   # test to verify HDFFV-10333: error similar to H5O_attr_decode in the jira issue
   ADD_H5_TEST (err_attr_dspace 1 err_attr_dspace.h5)
 

tools/test/h5dump/errfiles/infinite_loop.err

@@ -0,0 +1 @@
+h5dump error: unable to open file "3790_infinite_loop.h5"

tools/test/h5dump/testh5dump.sh.in

@@ -138,6 +138,7 @@ $SRC_H5DUMP_TESTFILES/tfloat16_be.h5
 $SRC_H5DUMP_TESTFILES/tfpformat.h5
 $SRC_H5DUMP_TESTFILES/tfvalues.h5
 $SRC_H5DUMP_TESTFILES/tgroup.h5
+$SRC_H5DUMP_TESTFILES/3790_infinite_loop.h5
 $SRC_H5DUMP_TESTFILES/tgrp_comments.h5
 $SRC_H5DUMP_TESTFILES/tgrpnullspace.h5
 $SRC_H5DUMP_TESTFILES/thlink.h5
@@ -200,6 +201,7 @@ $SRC_H5DUMP_OUTFILES/charsets.ddl
 $SRC_H5DUMP_OUTFILES/file_space.ddl
 $SRC_H5DUMP_OUTFILES/filter_fail.ddl
 $SRC_H5DUMP_OUTFILES/non_existing.ddl
+$SRC_H5DUMP_OUTFILES/infinite_loop.ddl
 $SRC_H5DUMP_OUTFILES/packedbits.ddl
 $SRC_H5DUMP_OUTFILES/tall-1.ddl
 $SRC_H5DUMP_OUTFILES/tall-2.ddl
@@ -395,6 +397,7 @@ $SRC_H5DUMP_EXPFILES/twithddlfile.exp
 LIST_ERROR_TEST_FILES="
 ${SRC_H5DUMP_ERRORFILES}/filter_fail.err
 ${SRC_H5DUMP_ERRORFILES}/non_existing.err
+${SRC_H5DUMP_ERRORFILES}/infinite_loop.err
 ${SRC_H5DUMP_ERRORFILES}/tall-1.err
 ${SRC_H5DUMP_ERRORFILES}/tall-2A.err
 ${SRC_H5DUMP_ERRORFILES}/tall-2A0.err
@@ -1561,6 +1564,9 @@ TOOLTEST2 tall-6.exp --enable-error-stack -y -o tall-6.txt -d /g1/g1.1/dset1.1.1
 # test for non-existing file
 TOOLTEST3 non_existing.ddl --enable-error-stack tgroup.h5 non_existing.h5
 
+# test to verify github issue #3790: infinite loop closing library
+TOOLTEST4 infinite_loop.ddl 3790_infinite_loop.h5
+
 # test to verify HDFFV-10333: error similar to H5O_attr_decode in the jira issue
 TOOLTEST err_attr_dspace.ddl err_attr_dspace.h5