From 73bb382e9e77ca9847c09dff37b9e2338f26bbb8 Mon Sep 17 00:00:00 2001 From: Dana Robinson <43805+derobins@users.noreply.github.com> Date: Fri, 30 Apr 2021 12:47:51 -0700 Subject: [PATCH] Fixes crashes when size_hint > UINT32_MAX is passed to H5Gcreate1 (#611) * Committing clang-format changes * Fixes incorrect size_hint handling in H5Gcreate1 * Updates the size hint type for group creation * Updates the RELEASE.txt note * Revert "Updates the RELEASE.txt note" This reverts commit 3df386acca806d652bbe2209f7c4503b30f068ff. * Reverts previous behavior to use a uint32_t struct field * Updates RELEASE.txt Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> --- release_docs/RELEASE.txt | 18 ++++++++++++++++++ src/H5Gdeprec.c | 10 ++++++---- src/H5Gpublic.h | 12 +++++------- test/tmisc.c | 25 +++++++++++++++++++++++-- 4 files changed, 52 insertions(+), 13 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 8b0739384d..be8440fbac 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -414,6 +414,24 @@ New Features Library: -------- + - H5Gcreate1() now rejects size_hint parameters larger than UINT32_MAX + + The size_hint value is ultimately stored in a uint32_t struct field, + so specifying a value larger than this on a 64-bit machine can cause + undefined behavior including crashing the system. + + The documentation for this API call was also incorrect, stating that + passing a negative value would cause the library to use a default + value. Instead, passing a "negative" value actually passes a very large + value, which is probably not what the user intends and can cause + crashes on 64-bit systems. + + The Doxygen documentation has been updated and passing values larger + than UINT32_MAX for size_hint will now produce a normal HDF5 error. + + (DER - 2021/04/29, HDFFV-11241) + + - H5Pset_fapl_log() no longer crashes when passed an invalid fapl ID When passed an invalid fapl ID, H5Pset_fapl_log() would usually diff --git a/src/H5Gdeprec.c b/src/H5Gdeprec.c index 453796ed8a..45065e8774 100644 --- a/src/H5Gdeprec.c +++ b/src/H5Gdeprec.c @@ -141,10 +141,10 @@ H5G_map_obj_type(H5O_type_t obj_type) * specified NAME. The group is opened for write access * and it's object ID is returned. * - * The optional SIZE_HINT specifies how much file space to - * reserve to store the names that will appear in this - * group. If a non-positive value is supplied for the SIZE_HINT - * then a default size is chosen. + * The SIZE_HINT parameter specifies how much file space to reserve + * to store the names that will appear in this group. This number + * must be less than or equal to UINT32_MAX. If zero is supplied + * for the SIZE_HINT then a default size is chosen. * * Note: Deprecated in favor of H5Gcreate2 * @@ -174,6 +174,8 @@ H5Gcreate1(hid_t loc_id, const char *name, size_t size_hint) /* Check arguments */ if (!name || !*name) HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, H5I_INVALID_HID, "no name given") + if (size_hint > UINT32_MAX) + HGOTO_ERROR(H5E_ARGS, H5E_BADVALUE, H5I_INVALID_HID, "size_hint cannot be larger than UINT32_MAX") /* Check if we need to create a non-standard GCPL */ if (size_hint > 0) { diff --git a/src/H5Gpublic.h b/src/H5Gpublic.h index 416ff2c500..b009d4175a 100644 --- a/src/H5Gpublic.h +++ b/src/H5Gpublic.h @@ -569,8 +569,8 @@ typedef struct H5G_stat_t { * * \fgdta_loc_id * \param[in] name Name of the group to create - * \param[in] size_hint Optional parameter indicating the number of bytes - * to reserve for the names that will appear in the group + * \param[in] size_hint The number of bytes to reserve for the names + * that will appear in the group * * \return \hid_t{group} * @@ -592,11 +592,9 @@ typedef struct H5G_stat_t { * group, is not limited. * * \p size_hint is a hint for the number of bytes to reserve to store - * the names which will be eventually added to the new group. Passing a - * value of zero for \p size_hint is usually adequate since the library - * is able to dynamically resize the name heap, but a correct hint may - * result in better performance. If a non-positive value is supplied - * for \p size_hint, then a default size is chosen. + * the names which will be eventually added to the new group. This + * value must be between 0 and UINT32_MAX (inclusive). If this + * parameter is zero, a default value will be used. * * The return value is a group identifier for the open group. This * group identifier should be closed by calling H5Gclose() when it is diff --git a/test/tmisc.c b/test/tmisc.c index 03f995299d..51e203a0a2 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -4088,9 +4088,31 @@ test_misc23(void) H5E_END_TRY; VERIFY(tmp_id, FAIL, "H5Gcreate1"); + /* Make sure that size_hint values that can't fit into a 32-bit + * unsigned integer are rejected. Only necessary on systems where + * size_t is a 64-bit type. + */ + if (SIZE_MAX > UINT32_MAX) { + H5E_BEGIN_TRY + { + tmp_id = H5Gcreate1(file_id, "/size_hint_too_large", SIZE_MAX); + } + H5E_END_TRY; + VERIFY(tmp_id, FAIL, "H5Gcreate1"); + } + + /* Make sure the largest size_hint value works */ + H5E_BEGIN_TRY + { + tmp_id = H5Gcreate1(file_id, "/largest_size_hint", UINT32_MAX); + } + H5E_END_TRY; + CHECK(tmp_id, FAIL, "H5Gcreate1"); + status = H5Gclose(tmp_id); + CHECK(status, FAIL, "H5Gclose"); + tmp_id = H5Gcreate1(file_id, "/A/grp", (size_t)0); CHECK(tmp_id, FAIL, "H5Gcreate1"); - status = H5Gclose(tmp_id); CHECK(status, FAIL, "H5Gclose"); @@ -4103,7 +4125,6 @@ test_misc23(void) tmp_id = H5Dcreate1(file_id, "/A/dset", type_id, space_id, create_id); CHECK(tmp_id, FAIL, "H5Dcreate1"); - status = H5Dclose(tmp_id); CHECK(status, FAIL, "H5Dclose"); #endif /* H5_NO_DEPRECATED_SYMBOLS */