Fix for github issue #3790: infinite loop closing library (#4445)

* Fix for github issue #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