From ba9deaeb66ac820f079702fc298c3aea91793a0f Mon Sep 17 00:00:00 2001 From: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Date: Fri, 19 Mar 2021 17:04:16 +0000 Subject: [PATCH 1/7] Committing clang-format changes --- src/H5Tconv.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/H5Tconv.c b/src/H5Tconv.c index f2dff0befbe..0c986331c2c 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -3843,7 +3843,7 @@ H5T__conv_i_i(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t half_size; /*half the type size */ size_t olap; /*num overlapping elements */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ size_t first; ssize_t sfirst; /*a signed version of `first' */ @@ -4286,7 +4286,7 @@ H5T__conv_f_f(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t olap; /*num overlapping elements */ ssize_t bitno = 0; /*bit number */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ uint8_t tmp1, tmp2; /*temp variables for swapping bytes*/ @@ -8401,7 +8401,7 @@ H5T__conv_f_i(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t tsize; /*type size for swapping bytes */ size_t olap; /*num overlapping elements */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ uint8_t tmp1, tmp2; /*temp variables for swapping bytes*/ @@ -9027,7 +9027,7 @@ H5T__conv_i_f(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, siz size_t tsize; /*type size for swapping bytes */ size_t olap; /*num overlapping elements */ uint8_t * s, *sp, *d, *dp; /*source and dest traversal ptrs*/ - uint8_t * src_rev = NULL; /*order-reversed source buffer */ + uint8_t * src_rev = NULL; /*order-reversed source buffer */ uint8_t dbuf[64] = {0}; /*temp destination buffer */ uint8_t tmp1, tmp2; /*temp variables for swapping bytes*/ From 7a6ee4e3f49d3462e8efd447e32750e2ce1537ce Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Thu, 29 Apr 2021 19:18:38 -0700 Subject: [PATCH 2/7] Fixes incorrect size_hint handling in H5Gcreate1 --- release_docs/RELEASE.txt | 18 ++++++++++++++++++ src/H5Gdeprec.c | 10 ++++++---- src/H5Gpublic.h | 12 +++++------- test/tmisc.c | 13 +++++++++++++ 4 files changed, 42 insertions(+), 11 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 8b0739384d7..d2b239b1362 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 field 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 453796ed8ae..45065e8774d 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 416ff2c5004..b009d4175af 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 03f995299d5..4d0c04dd211 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -4088,6 +4088,19 @@ 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"); + } + tmp_id = H5Gcreate1(file_id, "/A/grp", (size_t)0); CHECK(tmp_id, FAIL, "H5Gcreate1"); From 17fc91e0efecfe005bd1928e5138d232df85ba2e Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Thu, 29 Apr 2021 20:13:07 -0700 Subject: [PATCH 3/7] Updates the size hint type for group creation --- src/H5Gdeprec.c | 9 +++------ src/H5Gpublic.h | 3 +-- src/H5Oprivate.h | 2 +- test/tmisc.c | 17 ++++------------- 4 files changed, 9 insertions(+), 22 deletions(-) diff --git a/src/H5Gdeprec.c b/src/H5Gdeprec.c index 45065e8774d..255fb32b106 100644 --- a/src/H5Gdeprec.c +++ b/src/H5Gdeprec.c @@ -142,9 +142,8 @@ H5G_map_obj_type(H5O_type_t obj_type) * and it's object ID is returned. * * 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. + * to store the names that will appear in this group. If zero is + * supplied for the SIZE_HINT then a default size is chosen. * * Note: Deprecated in favor of H5Gcreate2 * @@ -174,8 +173,6 @@ 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) { @@ -199,7 +196,7 @@ H5Gcreate1(hid_t loc_id, const char *name, size_t size_hint) HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, H5I_INVALID_HID, "can't get group info") /* Set the non-default local heap size hint */ - H5_CHECKED_ASSIGN(ginfo.lheap_size_hint, uint32_t, size_hint, size_t); + ginfo.lheap_size_hint = size_hint; if (H5P_set(gc_plist, H5G_CRT_GROUP_INFO_NAME, &ginfo) < 0) HGOTO_ERROR(H5E_PLIST, H5E_CANTSET, H5I_INVALID_HID, "can't set group info") } diff --git a/src/H5Gpublic.h b/src/H5Gpublic.h index b009d4175af..8fe2fe8055b 100644 --- a/src/H5Gpublic.h +++ b/src/H5Gpublic.h @@ -592,8 +592,7 @@ 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. This - * value must be between 0 and UINT32_MAX (inclusive). If this + * the names which will be eventually added to the new group. If this * parameter is zero, a default value will be used. * * The return value is a group identifier for the open group. This diff --git a/src/H5Oprivate.h b/src/H5Oprivate.h index 65f030876ff..ae2448cf86c 100644 --- a/src/H5Oprivate.h +++ b/src/H5Oprivate.h @@ -684,7 +684,7 @@ typedef struct H5O_bogus_t { */ typedef struct H5O_ginfo_t { /* "Old" format group info (not stored) */ - uint32_t lheap_size_hint; /* Local heap size hint */ + size_t lheap_size_hint; /* Local heap size hint */ /* "New" format group info (stored) */ diff --git a/test/tmisc.c b/test/tmisc.c index 4d0c04dd211..9235e0c832f 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -4088,22 +4088,13 @@ 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"); - } - tmp_id = H5Gcreate1(file_id, "/A/grp", (size_t)0); CHECK(tmp_id, FAIL, "H5Gcreate1"); + /* Check that the largest size hint is acceptable */ + tmp_id = H5Gcreate1(file_id, "/enormous_size_hint", SIZE_MAX); + CHECK(tmp_id, FAIL, "H5Gcreate1"); + status = H5Gclose(tmp_id); CHECK(status, FAIL, "H5Gclose"); From 3df386acca806d652bbe2209f7c4503b30f068ff Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Thu, 29 Apr 2021 20:36:08 -0700 Subject: [PATCH 4/7] Updates the RELEASE.txt note --- release_docs/RELEASE.txt | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index d2b239b1362..22ad208e850 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -414,21 +414,19 @@ New Features Library: -------- - - H5Gcreate1() now rejects size_hint parameters larger than UINT32_MAX + - H5Gcreate1() now handles large size_hint parameters correctly - The size_hint value is ultimately stored in a uint32_t struct field, - so specifying a value larger than this on a 64-bit field can cause - undefined behavior including crashing the system. + On 64-bit systems, large values of the size_t size_hint parameter + would be passed through a uint32_t struct field, causing a crash + on 64-bit Windows w/ MSVC and probably incorrect behavior or crashes + on other 64-bit systems. 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) From 5f825f9b5691035aa7b78ee718a4358081513cdf Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 30 Apr 2021 09:11:04 -0700 Subject: [PATCH 5/7] Revert "Updates the RELEASE.txt note" This reverts commit 3df386acca806d652bbe2209f7c4503b30f068ff. --- release_docs/RELEASE.txt | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 22ad208e850..d2b239b1362 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -414,19 +414,21 @@ New Features Library: -------- - - H5Gcreate1() now handles large size_hint parameters correctly + - H5Gcreate1() now rejects size_hint parameters larger than UINT32_MAX - On 64-bit systems, large values of the size_t size_hint parameter - would be passed through a uint32_t struct field, causing a crash - on 64-bit Windows w/ MSVC and probably incorrect behavior or crashes - on other 64-bit systems. + The size_hint value is ultimately stored in a uint32_t struct field, + so specifying a value larger than this on a 64-bit field 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) From 850e79e052d6bb7a3f5f44b756afd123d48e1c97 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 30 Apr 2021 09:28:17 -0700 Subject: [PATCH 6/7] Reverts previous behavior to use a uint32_t struct field --- src/H5Gdeprec.c | 9 ++++++--- src/H5Gpublic.h | 3 ++- src/H5Oprivate.h | 2 +- test/tmisc.c | 27 ++++++++++++++++++++++----- 4 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/H5Gdeprec.c b/src/H5Gdeprec.c index 255fb32b106..45065e8774d 100644 --- a/src/H5Gdeprec.c +++ b/src/H5Gdeprec.c @@ -142,8 +142,9 @@ H5G_map_obj_type(H5O_type_t obj_type) * and it's object ID is returned. * * The SIZE_HINT parameter specifies how much file space to reserve - * to store the names that will appear in this group. If zero is - * supplied for the SIZE_HINT then a default size is chosen. + * 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 * @@ -173,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) { @@ -196,7 +199,7 @@ H5Gcreate1(hid_t loc_id, const char *name, size_t size_hint) HGOTO_ERROR(H5E_PLIST, H5E_CANTGET, H5I_INVALID_HID, "can't get group info") /* Set the non-default local heap size hint */ - ginfo.lheap_size_hint = size_hint; + H5_CHECKED_ASSIGN(ginfo.lheap_size_hint, uint32_t, size_hint, size_t); if (H5P_set(gc_plist, H5G_CRT_GROUP_INFO_NAME, &ginfo) < 0) HGOTO_ERROR(H5E_PLIST, H5E_CANTSET, H5I_INVALID_HID, "can't set group info") } diff --git a/src/H5Gpublic.h b/src/H5Gpublic.h index 8fe2fe8055b..b009d4175af 100644 --- a/src/H5Gpublic.h +++ b/src/H5Gpublic.h @@ -592,7 +592,8 @@ 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. If this + * 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 diff --git a/src/H5Oprivate.h b/src/H5Oprivate.h index ae2448cf86c..65f030876ff 100644 --- a/src/H5Oprivate.h +++ b/src/H5Oprivate.h @@ -684,7 +684,7 @@ typedef struct H5O_bogus_t { */ typedef struct H5O_ginfo_t { /* "Old" format group info (not stored) */ - size_t lheap_size_hint; /* Local heap size hint */ + uint32_t lheap_size_hint; /* Local heap size hint */ /* "New" format group info (stored) */ diff --git a/test/tmisc.c b/test/tmisc.c index 9235e0c832f..51e203a0a23 100644 --- a/test/tmisc.c +++ b/test/tmisc.c @@ -4088,13 +4088,31 @@ test_misc23(void) H5E_END_TRY; VERIFY(tmp_id, FAIL, "H5Gcreate1"); - tmp_id = H5Gcreate1(file_id, "/A/grp", (size_t)0); - CHECK(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"); + } - /* Check that the largest size hint is acceptable */ - tmp_id = H5Gcreate1(file_id, "/enormous_size_hint", SIZE_MAX); + /* 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"); @@ -4107,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 */ From a4227fbcdd95f9d6aa7359f8dd0c8744283b7d11 Mon Sep 17 00:00:00 2001 From: Dana Robinson Date: Fri, 30 Apr 2021 09:30:15 -0700 Subject: [PATCH 7/7] Updates RELEASE.txt --- release_docs/RELEASE.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index d2b239b1362..be8440fbac5 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -417,7 +417,7 @@ New Features - 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 field can cause + 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