Skip to content

Commit

Permalink
Fix an issue where compound datatype member IDs can be leaked during …
Browse files Browse the repository at this point in the history 
…conversion (HDFGroup#4459)

Also fixes issues with handling of partially initialized datatypes during library shutdown
  • Loading branch information
@jhendersonHDF
jhendersonHDF committed May 13, 2024
1 parent f0ecc8b commit baf0b9a
Show file tree
Hide file tree
Showing 4 changed files with 254 additions and 20 deletions.
17 changes: 17 additions & 0 deletions release_docs/RELEASE.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -499,6 +499,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
Expand Down
13 changes: 5 additions & 8 deletions src/H5T.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1643,12 +1643,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() */
Expand Down Expand Up @@ -1862,7 +1861,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.
Expand Down Expand Up @@ -4162,10 +4160,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");

Expand Down Expand Up @@ -4202,10 +4200,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--;

Expand Down
35 changes: 23 additions & 12 deletions src/H5Tconv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2241,21 +2241,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 */
Expand Down
209 changes: 209 additions & 0 deletions test/dtypes.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
*
Expand Down Expand Up @@ -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();
Expand Down

0 comments on commit baf0b9a

Please sign in to comment.