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

@@ -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

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

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--;
 

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 */

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();