From a9a1638dd835456849a3ada9fe4a40a7402a912f Mon Sep 17 00:00:00 2001
From: Jordan Henderson <jhenderson@hdfgroup.org>
Date: Mon, 6 May 2024 17:50:07 -0500
Subject: [PATCH] Fix an issue where compound datatype member IDs can be leaked
during conversion
Also fixes issues with handling of partially initialized datatypes during library shutdown
---
release_docs/RELEASE.txt | 16 +++
src/H5T.c | 13 +--
src/H5Tconv_compound.c | 35 ++++---
test/dtypes.c | 209 +++++++++++++++++++++++++++++++++++++++
4 files changed, 253 insertions(+), 20 deletions(-)
diff --git a/src/H5T.c b/src/H5T.c
index dc531f85a70..3be2481303a 100644
--- a/src/H5T.c
+++ b/src/H5T.c
@@ -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() */
@@ -1874,7 +1873,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.
@@ -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");
@@ -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--;
diff --git a/src/H5Tconv.c b/src/H5Tconv.c
index 3e988b33f18..9495fbc621e 100644
--- a/src/H5Tconv.c
+++ b/src/H5Tconv.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 cebc1b525b5..ce8ee747b57 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();