From 5f36553713d93e0b84c29f05027387d1236e872a Mon Sep 17 00:00:00 2001 From: jhendersonHDF Date: Tue, 14 May 2024 07:35:01 -0500 Subject: [PATCH] Fix an issue where compound datatype member IDs can be leaked during conversion (#4459) (#4483) Also fixes issues with handling of partially initialized datatypes during library shutdown --- release_docs/HISTORY-1_14.txt | 17 +++ release_docs/RELEASE.txt | 1 - src/H5T.c | 13 +-- src/H5Tconv_compound.c | 35 ++++-- test/dtypes.c | 209 ++++++++++++++++++++++++++++++++++ 5 files changed, 254 insertions(+), 21 deletions(-) diff --git a/release_docs/HISTORY-1_14.txt b/release_docs/HISTORY-1_14.txt index 050e0249bad..30229472d97 100644 --- a/release_docs/HISTORY-1_14.txt +++ b/release_docs/HISTORY-1_14.txt @@ -514,6 +514,23 @@ Bug Fixes since HDF5-1.14.3 release Library ------- + - Fixed a leak of datatype IDs created internally during datatype conversion + + Fixed an issue where the library could leak IDs that it creates internally + for compound datatype members during datatype conversion. When the library's + table of datatype conversion functions is modified (such as when a new + conversion function is registered with the library from within an application), + the compound datatype conversion function has to recalculate data that it + has cached. When recalculating that data, the library was registering new + IDs for each of the members of the source and destination compound datatypes + involved in the conversion process and was overwriting the old cached IDs + without first closing them. This would result in use-after-free issues due + to multiple IDs pointing to the same internal H5T_t structure, as well as + crashes due to the library not gracefully handling partially initialized or + partially freed datatypes on library termination. + + Fixes h5py GitHub #2419 + - Fixed many (future) CVE issues A partner organization corrected many potential security issues, which diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 88cd0dc43ff..0108f235722 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -502,7 +502,6 @@ Bug Fixes since HDF5-1.14.4 release =================================== Library ------- - - Fixed function H5Requal actually to compare the reference pointers Fixed an issue with H5Requal always returning true because the diff --git a/src/H5T.c b/src/H5T.c index a386dc95a6d..7d63d003e27 100644 --- a/src/H5T.c +++ b/src/H5T.c @@ -1654,12 +1654,11 @@ H5T__unlock_cb(void *_dt, hid_t H5_ATTR_UNUSED id, void *_udata) FUNC_ENTER_PACKAGE_NOERR assert(dt); - assert(dt->shared); - if (H5T_STATE_IMMUTABLE == dt->shared->state) { + if (dt->shared && (H5T_STATE_IMMUTABLE == dt->shared->state)) { dt->shared->state = H5T_STATE_RDONLY; (*n)++; - } /* end if */ + } FUNC_LEAVE_NOAPI(SUCCEED) } /* end H5T__unlock_cb() */ @@ -1873,7 +1872,6 @@ H5T__close_cb(H5T_t *dt, void **request) /* Sanity check */ assert(dt); - assert(dt->shared); /* If this datatype is VOL-managed (i.e.: has a VOL object), * close it through the VOL connector. @@ -4153,10 +4151,10 @@ H5T_close_real(H5T_t *dt) FUNC_ENTER_NOAPI(FAIL) /* Sanity check */ - assert(dt && dt->shared); + assert(dt); /* Clean up resources, depending on shared state */ - if (dt->shared->state != H5T_STATE_OPEN) { + if (dt->shared && (dt->shared->state != H5T_STATE_OPEN)) { if (H5T__free(dt) < 0) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTFREE, FAIL, "unable to free datatype"); @@ -4193,10 +4191,9 @@ H5T_close(H5T_t *dt) /* Sanity check */ assert(dt); - assert(dt->shared); /* Named datatype cleanups */ - if (dt->shared->state == H5T_STATE_OPEN) { + if (dt->shared && (dt->shared->state == H5T_STATE_OPEN)) { /* Decrement refcount count on open named datatype */ dt->shared->fo_count--; diff --git a/src/H5Tconv_compound.c b/src/H5Tconv_compound.c index 3e988b33f18..9495fbc621e 100644 --- a/src/H5Tconv_compound.c +++ b/src/H5Tconv_compound.c @@ -264,21 +264,32 @@ H5T__conv_struct_init(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, co if (need_ids) { hid_t tid; - if ((tid = H5I_register(H5I_DATATYPE, priv->src_memb[i], false)) < 0) { - H5T__conv_struct_free(priv); - cdata->priv = NULL; - HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, FAIL, - "can't register ID for source compound member datatype"); + /* Only register new IDs for the source and destination member datatypes + * if IDs weren't already registered for them. If the cached conversion + * information has to be recalculated (in the case where the library's + * table of conversion functions is modified), the same IDs can be reused + * since the only information that needs to be recalculated is the conversion + * paths used. + */ + if (priv->src_memb_id[i] == H5I_INVALID_HID) { + if ((tid = H5I_register(H5I_DATATYPE, priv->src_memb[i], false)) < 0) { + H5T__conv_struct_free(priv); + cdata->priv = NULL; + HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, FAIL, + "can't register ID for source compound member datatype"); + } + priv->src_memb_id[i] = tid; } - priv->src_memb_id[i] = tid; - if ((tid = H5I_register(H5I_DATATYPE, priv->dst_memb[src2dst[i]], false)) < 0) { - H5T__conv_struct_free(priv); - cdata->priv = NULL; - HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, FAIL, - "can't register ID for destination compound member datatype"); + if (priv->dst_memb_id[src2dst[i]] == H5I_INVALID_HID) { + if ((tid = H5I_register(H5I_DATATYPE, priv->dst_memb[src2dst[i]], false)) < 0) { + H5T__conv_struct_free(priv); + cdata->priv = NULL; + HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, FAIL, + "can't register ID for destination compound member datatype"); + } + priv->dst_memb_id[src2dst[i]] = tid; } - priv->dst_memb_id[src2dst[i]] = tid; } } /* end if */ } /* end for */ diff --git a/test/dtypes.c b/test/dtypes.c index 51dbf123fe0..023981c5bbe 100644 --- a/test/dtypes.c +++ b/test/dtypes.c @@ -3913,6 +3913,214 @@ test_user_compound_conversion(void) return 1; } /* end test_user_compound_conversion() */ +/*------------------------------------------------------------------------- + * Function: test_compound_member_convert_id_leak_func1 + * + * Purpose: Datatype conversion function for the + * test_compound_member_convert_id_leak test that just + * converts a float value to a double value with a cast. + * + * Return: Success: 0 + * Failure: number of errors + * + *------------------------------------------------------------------------- + */ +static herr_t +test_compound_member_convert_id_leak_func1(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, + size_t H5_ATTR_UNUSED buf_stride, size_t H5_ATTR_UNUSED bkg_stride, + void *buf, void H5_ATTR_UNUSED *bkg, + hid_t H5_ATTR_UNUSED dset_xfer_plist) +{ + float tmp_val; + double tmp_val2; + + switch (cdata->command) { + case H5T_CONV_INIT: + if (!H5Tequal(src_id, H5T_NATIVE_FLOAT) || !H5Tequal(dst_id, H5T_NATIVE_DOUBLE)) + return FAIL; + break; + case H5T_CONV_CONV: + if (nelmts != 1) + return FAIL; + + memcpy(&tmp_val, buf, sizeof(float)); + tmp_val2 = (double)tmp_val; + memcpy(buf, &tmp_val2, sizeof(double)); + + break; + case H5T_CONV_FREE: + break; + default: + return FAIL; + } + + return SUCCEED; +} + +/*------------------------------------------------------------------------- + * Function: test_compound_member_convert_id_leak_func2 + * + * Purpose: Datatype conversion function for the + * test_compound_member_convert_id_leak test that just + * returns the double value 0.1. + * + * Return: Success: 0 + * Failure: number of errors + * + *------------------------------------------------------------------------- + */ +static herr_t +test_compound_member_convert_id_leak_func2(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata, size_t nelmts, + size_t H5_ATTR_UNUSED buf_stride, size_t H5_ATTR_UNUSED bkg_stride, + void *buf, void H5_ATTR_UNUSED *bkg, + hid_t H5_ATTR_UNUSED dset_xfer_plist) +{ + double tmp_val = 0.1; + + switch (cdata->command) { + case H5T_CONV_INIT: + if (!H5Tequal(src_id, H5T_NATIVE_FLOAT) || !H5Tequal(dst_id, H5T_NATIVE_DOUBLE)) + return FAIL; + break; + case H5T_CONV_CONV: + if (nelmts != 1) + return FAIL; + + memcpy(buf, &tmp_val, sizeof(double)); + + break; + case H5T_CONV_FREE: + break; + default: + return FAIL; + } + + return SUCCEED; +} + +/*------------------------------------------------------------------------- + * Function: test_compound_member_convert_id_leak + * + * Purpose: Tests for an issue where IDs that are registered for + * compound datatype members during datatype conversion were + * leaked when the library's conversion path table is modified + * and the compound conversion path recalculates its cached + * data. + * + * Return: Success: 0 + * Failure: number of errors + * + *------------------------------------------------------------------------- + */ +static int +test_compound_member_convert_id_leak(void) +{ + int64_t num_dtype_ids = 0; + float val1; + double val2; + double bkg; + hid_t tid1 = H5I_INVALID_HID; + hid_t tid2 = H5I_INVALID_HID; + + TESTING("compound conversion member ID leak"); + + if ((tid1 = H5Tcreate(H5T_COMPOUND, sizeof(float))) < 0) + TEST_ERROR; + if ((tid2 = H5Tcreate(H5T_COMPOUND, sizeof(double))) < 0) + TEST_ERROR; + + if (H5Tinsert(tid1, "mem", 0, H5T_NATIVE_FLOAT) < 0) + TEST_ERROR; + if (H5Tinsert(tid2, "mem", 0, H5T_NATIVE_DOUBLE) < 0) + TEST_ERROR; + + /* Store the current number of datatype IDs registered */ + if ((num_dtype_ids = H5I_nmembers(H5I_DATATYPE)) < 0) + TEST_ERROR; + + /* Convert a value from float to double */ + val1 = 3.0f; + val2 = 0.0; + memcpy(&val2, &val1, sizeof(float)); + if (H5Tconvert(tid1, tid2, 1, &val2, &bkg, H5P_DEFAULT) < 0) + TEST_ERROR; + + /* Make sure the number of datatype IDs registered didn't change */ + if (num_dtype_ids != H5I_nmembers(H5I_DATATYPE)) + TEST_ERROR; + + /* Register a custom conversion function from float to double + * and convert the value again + */ + if (H5Tregister(H5T_PERS_HARD, "myflttodbl", H5T_NATIVE_FLOAT, H5T_NATIVE_DOUBLE, + test_compound_member_convert_id_leak_func1) < 0) + TEST_ERROR; + + val1 = 3.0f; + val2 = 0.0; + memcpy(&val2, &val1, sizeof(float)); + if (H5Tconvert(tid1, tid2, 1, &val2, &bkg, H5P_DEFAULT) < 0) + TEST_ERROR; + + /* Since an application conversion function was used, two IDs should + * have been registered, one for the source type and one for the + * destination type + */ + num_dtype_ids += 2; + + /* Make sure the number of datatype IDs registered is correct */ + if (num_dtype_ids != H5I_nmembers(H5I_DATATYPE)) + TEST_ERROR; + + if (H5Tunregister(H5T_PERS_HARD, "myflttodbl", H5T_NATIVE_FLOAT, H5T_NATIVE_DOUBLE, + test_compound_member_convert_id_leak_func1) < 0) + TEST_ERROR; + + /* Register a different custom conversion function from float to double + * and convert the value again + */ + if (H5Tregister(H5T_PERS_HARD, "myflttodbl", H5T_NATIVE_FLOAT, H5T_NATIVE_DOUBLE, + test_compound_member_convert_id_leak_func2) < 0) + TEST_ERROR; + + val1 = 3.0f; + val2 = 0.0; + memcpy(&val2, &val1, sizeof(float)); + if (H5Tconvert(tid1, tid2, 1, &val2, &bkg, H5P_DEFAULT) < 0) + TEST_ERROR; + + /* Make sure the number of datatype IDs registered didn't change */ + if (num_dtype_ids != H5I_nmembers(H5I_DATATYPE)) + TEST_ERROR; + + if (H5Tunregister(H5T_PERS_HARD, "myflttodbl", H5T_NATIVE_FLOAT, H5T_NATIVE_DOUBLE, + test_compound_member_convert_id_leak_func2) < 0) + TEST_ERROR; + + if (H5Tclose(tid1) < 0) + TEST_ERROR; + if (H5Tclose(tid2) < 0) + TEST_ERROR; + + PASSED(); + + return 0; + +error: + H5E_BEGIN_TRY + { + H5Tunregister(H5T_PERS_HARD, "myflttodbl", H5T_NATIVE_FLOAT, H5T_NATIVE_DOUBLE, + test_compound_member_convert_id_leak_func1); + H5Tunregister(H5T_PERS_HARD, "myflttodbl", H5T_NATIVE_FLOAT, H5T_NATIVE_DOUBLE, + test_compound_member_convert_id_leak_func2); + H5Tclose(tid1); + H5Tclose(tid2); + } + H5E_END_TRY; + + return 1; +} /* end test_compound_member_convert_id_leak() */ + /*------------------------------------------------------------------------- * Function: test_query * @@ -10198,6 +10406,7 @@ main(void) nerrors += test_compound_17(); nerrors += test_compound_18(); nerrors += test_user_compound_conversion(); + nerrors += test_compound_member_convert_id_leak(); nerrors += test_conv_enum_1(); nerrors += test_conv_enum_2(); nerrors += test_conv_bitfield();