Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix leak of internal IDs registered during compound datatype conversion #4459

Merged
merged 1 commit into from
May 10, 2024
Merged

Fix leak of internal IDs registered during compound datatype conversion #4459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -749,6 +749,22 @@ Bug Fixes since HDF5-1.14.0 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 function H5Requal actually to compare the reference pointers

Expand Down
13 changes: 5 additions & 8 deletions src/H5T.c
View file
Open in desktop
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes in this file are for making the library handle unlocking/closing of partially initialized datatypes more gracefully

Original file line number Diff line number Diff line change
Expand Up @@ -1655,12 +1655,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 @@ -1874,7 +1873,6 @@ H5T__close_cb(H5T_t *dt, void **request)

/* Sanity check */
assert(dt);
assert(dt->shared);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handled further down when dt is passed to H5T_close


/* If this datatype is VOL-managed (i.e.: has a VOL object),
* close it through the VOL connector.
Expand Down Expand Up @@ -4154,10 +4152,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 @@ -4194,10 +4192,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_compound.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a guarantee that the memb_id's are initialized to H5I_INVALID_HID?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are initialized a bit above around line 174 and 180

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
Open in desktop
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
Loading