From ddc85a18e6c33beaf38acd0b0e2e2cbbe1be04aa Mon Sep 17 00:00:00 2001 From: Dana Robinson <43805+derobins@users.noreply.github.com> Date: Thu, 7 Mar 2024 18:36:58 -0800 Subject: [PATCH] Fixed asserts due to H5Pset_est_link_info() values (#4081) * Fixed asserts due to H5Pset_est_link_info() values If large values for est_num_entries and/or est_name_len were passed to H5Pset_est_link_info(), the library would attempt to create an object header NIL message to reserve enough space to hold the links in compact form (i.e., concatenated), which could exceed allowable object header message size limits and trip asserts in the library. This bug only occurred when using the HDF5 1.8 file format or later and required the product of the two values to be ~64k more than the size of any links written to the group, which would cause the library to write out a too-large NIL spacer message to reserve the space for the unwritten links. The library now inspects the phase change values to see if the dataset is likely to be compact and checks the size to ensure any NIL spacer messages won't be larger than the library allows. Fixes GitHub #1632 * Fix copy-paste comments --- release_docs/RELEASE.txt | 20 ++++++++ src/H5Gobj.c | 20 +++++++- test/tmisc.c | 100 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 139 insertions(+), 1 deletion(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index c002f90d6a1..08250c361c9 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -219,6 +219,26 @@ Bug Fixes since HDF5-1.14.3 release =================================== Library ------- + - Fixed asserts raised by large values of H5Pset_est_link_info() parameters + + If large values for est_num_entries and/or est_name_len were passed + to H5Pset_est_link_info(), the library would attempt to create an + object header NIL message to reserve enough space to hold the links in + compact form (i.e., concatenated), which could exceed allowable object + header message size limits and trip asserts in the library. + + This bug only occurred when using the HDF5 1.8 file format or later and + required the product of the two values to be ~64k more than the size + of any links written to the group, which would cause the library to + write out a too-large NIL spacer message to reserve the space for the + unwritten links. + + The library now inspects the phase change values to see if the dataset + is likely to be compact and checks the size to ensure any NIL spacer + messages won't be larger than the library allows. + + Fixes GitHub #1632 + - Fixed a bug where H5Tset_fields does not account for any offset set for a floating-point datatype when determining if values set for spos, epos, esize, mpos and msize make sense for the datatype diff --git a/src/H5Gobj.c b/src/H5Gobj.c index e701922ab4e..c5bdfc1be46 100644 --- a/src/H5Gobj.c +++ b/src/H5Gobj.c @@ -218,7 +218,25 @@ H5G__obj_create_real(H5F_t *f, const H5O_ginfo_t *ginfo, const H5O_linfo_t *linf assert(link_size); /* Compute size of header to use for creation */ - hdr_size = linfo_size + ginfo_size + pline_size + (ginfo->est_num_entries * link_size); + + /* Basic header size */ + hdr_size = linfo_size + ginfo_size + pline_size; + + /* If this is likely to be a compact group, add space for the link + * messages, unless the size of the link messages is greater than + * the largest allowable object header message size, since the size + * of the link messages is the size of the NIL spacer message that + * would have to be written out to reserve enough space to hold the + * links if the group were left empty. + */ + bool compact = ginfo->est_num_entries <= ginfo->max_compact; + if (compact) { + + size_t size_of_links = ginfo->est_num_entries * link_size; + + if (size_of_links < H5O_MESG_MAX_SIZE) + hdr_size += size_of_links; + } } /* end if */ else hdr_size = (size_t)(4 + 2 * H5F_SIZEOF_ADDR(f)); diff --git a/test/tmisc.c b/test/tmisc.c index ddebc3d648f..4f337582a1c 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -338,6 +338,7 @@ typedef struct { #define CVE_2020_10812_FILENAME "cve_2020_10812.h5" #define MISC38_FILE "type_conversion_path_table_issue.h5" +#define MISC39_FILE "set_est_link_info.h5" /**************************************************************** ** @@ -6445,6 +6446,103 @@ test_misc38(void) CHECK(ret, FAIL, "H5Sclose"); } +/**************************************************************** +** +** test_misc39(): Ensure H5Pset_est_link_info() handles large +** values +** +** H5Pset_est_link_info() values can be set to large values, +** which could cause the library to attempt to create large +** object headers that exceed limits and trip asserts in +** the library. +** +** This test ensures that the library doesn't error regardless +** of the values passed to H5Pset_est_link_info() and +** H5Pset_link_phase_change(). +** +****************************************************************/ +static void +test_misc39(void) +{ + hid_t fid = H5I_INVALID_HID; /* File ID */ + hid_t gid = H5I_INVALID_HID; /* Group ID */ + hid_t fapl = H5I_INVALID_HID; /* File access property list ID */ + hid_t gcpl = H5I_INVALID_HID; /* Group creation property list ID */ + herr_t ret = H5I_INVALID_HID; /* Generic return value */ + + /* Output message about test being performed */ + MESSAGE(5, ("Ensure H5Pset_est_link_info handles large values\n")); + + /* Compose file access property list + * + * NOTE: The bug in question only occurs in new-style groups + */ + fapl = H5Pcreate(H5P_FILE_ACCESS); + CHECK(fapl, H5I_INVALID_HID, "H5Pcreate"); + ret = H5Pset_libver_bounds(fapl, H5F_LIBVER_LATEST, H5F_LIBVER_LATEST); + CHECK(ret, FAIL, "H5Pset_libver_bounds"); + + /* Create the file */ + fid = H5Fcreate(MISC39_FILE, H5F_ACC_TRUNC, H5P_DEFAULT, fapl); + CHECK(fid, H5I_INVALID_HID, "H5Fcreate"); + + /* Compose group creation property list */ + gcpl = H5Pcreate(H5P_GROUP_CREATE); + CHECK(gcpl, H5I_INVALID_HID, "H5Pcreate"); + + /* Set the estimated link info values to large numbers */ + ret = H5Pset_est_link_info(gcpl, UINT16_MAX, UINT16_MAX); + CHECK(ret, FAIL, "H5Pset_est_link_info"); + + /* Create a group */ + gid = H5Gcreate2(fid, "foo", H5P_DEFAULT, gcpl, H5P_DEFAULT); + CHECK(gid, H5I_INVALID_HID, "H5Gcreate2"); + + /* Close the group */ + ret = H5Gclose(gid); + CHECK(ret, FAIL, "H5Gclose"); + + /* Close the file. Asserts typically occur here, when the metadata cache + * objects are flushed. + */ + ret = H5Fclose(fid); + CHECK(ret, FAIL, "H5Fclose"); + + /* Re-open the file */ + fid = H5Fopen(MISC25C_FILE, H5F_ACC_RDWR, H5P_DEFAULT); + CHECK(fid, H5I_INVALID_HID, "H5Fopen"); + + /* Set the compact/dense value high, to see if we can trick the + * library into creating a dense group object header that is + * larger than the maximum allowed size. + */ + ret = H5Pset_link_phase_change(gcpl, UINT16_MAX, UINT16_MAX); + CHECK(ret, FAIL, "H5Pset_link_phase_change"); + + /* Set the estimated link info values to large numbers */ + ret = H5Pset_est_link_info(gcpl, UINT16_MAX / 2, UINT16_MAX); + CHECK(ret, FAIL, "H5Pset_est_link_info"); + + /* Create another group */ + gid = H5Gcreate2(fid, "bar", H5P_DEFAULT, gcpl, H5P_DEFAULT); + CHECK(gid, H5I_INVALID_HID, "H5Gcreate2"); + + /* Close the group */ + ret = H5Gclose(gid); + CHECK(ret, FAIL, "H5Gclose"); + + /* Close the file */ + ret = H5Fclose(fid); + CHECK(ret, FAIL, "H5Fclose"); + + /* Close the property lists */ + ret = H5Pclose(fapl); + CHECK(ret, FAIL, "H5Pclose"); + ret = H5Pclose(gcpl); + CHECK(ret, FAIL, "H5Pclose"); + +} /* end test_misc39() */ + /**************************************************************** ** ** test_misc(): Main misc. test routine. @@ -6514,6 +6612,7 @@ test_misc(void) test_misc36(); /* Exercise H5atclose and H5is_library_terminating */ test_misc37(); /* Test for seg fault failure at file close */ test_misc38(); /* Test for type conversion path table issue */ + test_misc39(); /* Ensure H5Pset_est_link_info() handles large values */ } /* test_misc() */ @@ -6570,6 +6669,7 @@ cleanup_misc(void) H5Fdelete(MISC31_FILE, H5P_DEFAULT); #endif /* H5_NO_DEPRECATED_SYMBOLS */ H5Fdelete(MISC38_FILE, H5P_DEFAULT); + H5Fdelete(MISC39_FILE, H5P_DEFAULT); } H5E_END_TRY } /* end cleanup_misc() */