From b23affc2a63ec3b6d891fe99c93f0f3d15d440b8 Mon Sep 17 00:00:00 2001 From: vchoi-hdfgroup <55293060+vchoi-hdfgroup@users.noreply.github.com> Date: Tue, 30 Apr 2024 07:21:08 -0500 Subject: [PATCH] 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 | 14 ++++ src/H5Ocache.c | 64 ++++++------------ src/H5Oint.c | 2 +- src/H5Opkg.h | 2 +- tools/test/h5dump/CMakeTests.cmake | 6 ++ tools/test/h5dump/errfiles/infinite_loop.err | 1 + tools/test/h5dump/expected/infinite_loop.ddl | 0 .../h5dump/testfiles/3790_infinite_loop.h5 | Bin 0 -> 829 bytes tools/test/h5dump/testh5dump.sh.in | 6 ++ 9 files changed, 48 insertions(+), 47 deletions(-) create mode 100644 tools/test/h5dump/errfiles/infinite_loop.err create mode 100644 tools/test/h5dump/expected/infinite_loop.ddl create mode 100644 tools/test/h5dump/testfiles/3790_infinite_loop.h5 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 0000000000000000000000000000000000000000..0d3910a1a1638301afab1157bc65ff0ab63892fd GIT binary patch literal 829 zcmeD5aB<`1lHy_j0S*oZ76t(@6Gr@p0tIG>2#gPtPk=HQp>zk7Ucm%mFfxE31A_zu z!*8Ho20^I#=;}g(TwOsrVCKVUh$#rt0l^$)jfMa?1nL=F9RonD2xx$CK