diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index b9bf56ae82e..ee7505bdb34 100644 --- a/release_docs/RELEASE.txt +++ b/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 diff --git a/src/H5Ocache.c b/src/H5Ocache.c index 6916a9044c7..cc8033baf55 100644 --- a/src/H5Ocache.c +++ b/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 */ diff --git a/src/H5Oint.c b/src/H5Oint.c index a98e22a1821..e7fcf0cd270 100644 --- a/src/H5Oint.c +++ b/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; diff --git a/src/H5Opkg.h b/src/H5Opkg.h index 1803ae0daa7..ee6126a818c 100644 --- a/src/H5Opkg.h +++ b/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; diff --git a/tools/test/h5dump/CMakeTests.cmake b/tools/test/h5dump/CMakeTests.cmake index 2369f63c746..a4b43eee4e3 100644 --- a/tools/test/h5dump/CMakeTests.cmake +++ b/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) diff --git a/tools/test/h5dump/errfiles/infinite_loop.err b/tools/test/h5dump/errfiles/infinite_loop.err new file mode 100644 index 00000000000..c89e1551d3b --- /dev/null +++ b/tools/test/h5dump/errfiles/infinite_loop.err @@ -0,0 +1 @@ +h5dump error: unable to open file "3790_infinite_loop.h5" diff --git a/tools/test/h5dump/expected/infinite_loop.ddl b/tools/test/h5dump/expected/infinite_loop.ddl new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tools/test/h5dump/testfiles/3790_infinite_loop.h5 b/tools/test/h5dump/testfiles/3790_infinite_loop.h5 new file mode 100644 index 00000000000..0d3910a1a16 Binary files /dev/null and b/tools/test/h5dump/testfiles/3790_infinite_loop.h5 differ diff --git a/tools/test/h5dump/testh5dump.sh.in b/tools/test/h5dump/testh5dump.sh.in index 9df0fb0b7c1..007f93c07e3 100644 --- a/tools/test/h5dump/testh5dump.sh.in +++ b/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