From 355f67ac378a7b4de6f80f1cbf8758bbc2b0b1fe Mon Sep 17 00:00:00 2001 From: Matt L <124107509+mattjala@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:59:03 -0500 Subject: [PATCH] Close datatype IDs derived from memory type in the JNI translation helpers (#6594) * Close datatype IDs derived from the memory type in the JNI translate helpers The object-tree read/write helpers in h5util.c derive a base datatype from the memory type with H5Tget_super() for the variable-length, array and complex classes, but never closed it. Because an hid_t is not reclaimed when the native method returns, every read or write of such data leaked at least one datatype ID for the lifetime of the process, and nested types leaked one per level. This PR updates the helpers to close the derived type in their done: blocks, which covers both the success and the error paths, and to reset the id in the compound loops to avoid the potential for double closes. It also has the helpers release the class references that the per-element helpers look up on entry. These are local references, so they were reclaimed when the enclosing native method returned, but a compound read calls the helper once per member per element and held one set per call until then. Releasing them at the single exit bounds the count of references to the recursion depth. * Add CHANGELOG entry for the JNI datatype ID leak fix * Restrict the derived datatype close guards to strictly positive IDs hid_t 0 is not a valid datatype ID, so H5Tclose(0) would fail. * Revert "Restrict the derived datatype close guards to strictly positive IDs" An hid_t of 0 not being a valid ID is a property of the current H5I encoding rather than a documented guarantee, so the JNI helpers should not depend on it. --- java/src-jni/jni/h5util.c | 59 +++++ java/src-jni/test/TestH5D.java | 223 ++++++++++++++++++ java/src-jni/test/testfiles/JUnit-TestH5D.txt | 5 +- release_docs/CHANGELOG.md | 6 + 4 files changed, 292 insertions(+), 1 deletion(-) diff --git a/java/src-jni/jni/h5util.c b/java/src-jni/jni/h5util.c index e2c33e419fc..84efe663839 100644 --- a/java/src-jni/jni/h5util.c +++ b/java/src-jni/jni/h5util.c @@ -4347,6 +4347,7 @@ translate_atomic_rbuf(JNIEnv *env, jlong mem_type_id, H5T_class_t type_class, vo ENVPTR->DeleteLocalRef(ENVONLY, memb_jobj); } H5Tclose(memb); + memb = H5I_INVALID_HID; } jobj = jList; break; @@ -4558,6 +4559,27 @@ translate_atomic_rbuf(JNIEnv *env, jlong mem_type_id, H5T_class_t type_class, vo } /* switch(type_class) */ done: + /* The base type derived from a VLEN/ARRAY/COMPLEX memory type, or the member type of + * a COMPOUND that has not been closed by the loop above, is owned by this call. */ + if (memb >= 0) + H5Tclose(memb); + + /* Release the class references so a deep recursion (a compound read calls this helper + * once per member per element) does not hold one set per level until the JNI call ends. */ + if (arrCList) + ENVPTR->DeleteLocalRef(ENVONLY, arrCList); + if (cByte) + ENVPTR->DeleteLocalRef(ENVONLY, cByte); + if (cShort) + ENVPTR->DeleteLocalRef(ENVONLY, cShort); + if (cInt) + ENVPTR->DeleteLocalRef(ENVONLY, cInt); + if (cLong) + ENVPTR->DeleteLocalRef(ENVONLY, cLong); + if (cFloat) + ENVPTR->DeleteLocalRef(ENVONLY, cFloat); + if (cDouble) + ENVPTR->DeleteLocalRef(ENVONLY, cDouble); return jobj; } @@ -4678,6 +4700,7 @@ translate_atomic_wbuf(JNIEnv *env, jobject in_obj, jlong mem_type_id, H5T_class_ memb_vlSize); ENVPTR->DeleteLocalRef(ENVONLY, arr_obj); H5Tclose(memb); + memb = H5I_INVALID_HID; } ENVPTR->DeleteLocalRef(ENVONLY, array); @@ -4857,6 +4880,26 @@ translate_atomic_wbuf(JNIEnv *env, jobject in_obj, jlong mem_type_id, H5T_class_ } /* switch(type_class) */ done: + /* The base type derived from a VLEN/ARRAY/COMPLEX memory type, or the member type of + * a COMPOUND that has not been closed by the loop above, is owned by this call. */ + if (memb >= 0) + H5Tclose(memb); + + /* Release the class references so a deep recursion does not hold one set per level. */ + if (arrCList) + ENVPTR->DeleteLocalRef(ENVONLY, arrCList); + if (cByte) + ENVPTR->DeleteLocalRef(ENVONLY, cByte); + if (cShort) + ENVPTR->DeleteLocalRef(ENVONLY, cShort); + if (cInt) + ENVPTR->DeleteLocalRef(ENVONLY, cInt); + if (cLong) + ENVPTR->DeleteLocalRef(ENVONLY, cLong); + if (cFloat) + ENVPTR->DeleteLocalRef(ENVONLY, cFloat); + if (cDouble) + ENVPTR->DeleteLocalRef(ENVONLY, cDouble); return; } @@ -4983,6 +5026,7 @@ translate_rbuf(JNIEnv *env, jobjectArray ret_buf, jlong mem_type_id, H5T_class_t } H5Tclose(memb); + memb = H5I_INVALID_HID; } if (retIsList) ENVPTR->CallBooleanMethod(ENVONLY, ret_buf, arrAddMethod, jList); @@ -5110,6 +5154,13 @@ translate_rbuf(JNIEnv *env, jobjectArray ret_buf, jlong mem_type_id, H5T_class_t } /* switch(type_class) */ done: + /* The base type derived from a VLEN/ARRAY/COMPLEX memory type, or the member type of + * a COMPOUND that has not been closed by the loop above, is owned by this call. */ + if (memb >= 0) + H5Tclose(memb); + + if (arrCList) + ENVPTR->DeleteLocalRef(ENVONLY, arrCList); return; } @@ -5243,6 +5294,7 @@ translate_wbuf(JNIEnv *env, jobjectArray in_buf, jlong mem_type_id, H5T_class_t char_buf + i * typeSize + memb_offset, memb_vlSize); ENVPTR->DeleteLocalRef(ENVONLY, arr_obj); H5Tclose(memb); + memb = H5I_INVALID_HID; } ENVPTR->DeleteLocalRef(ENVONLY, array); @@ -5364,6 +5416,13 @@ translate_wbuf(JNIEnv *env, jobjectArray in_buf, jlong mem_type_id, H5T_class_t } /* switch(type_class) */ done: + /* The base type derived from a VLEN/ARRAY/COMPLEX memory type, or the member type of + * a COMPOUND that has not been closed by the loop above, is owned by this call. */ + if (memb >= 0) + H5Tclose(memb); + + if (arrCList) + ENVPTR->DeleteLocalRef(ENVONLY, arrCList); return; } diff --git a/java/src-jni/test/TestH5D.java b/java/src-jni/test/TestH5D.java index e49582c0bc3..7ff9f0ae0bf 100644 --- a/java/src-jni/test/TestH5D.java +++ b/java/src-jni/test/TestH5D.java @@ -1695,6 +1695,229 @@ public class TestH5D { } } + /* + * The object-tree read/write path derives a base datatype from the memory type with + * H5Tget_super() for the variable-length, array and complex classes. Those transient + * IDs are only visible through the H5F_OBJ_ALL path that walks the global datatype ID + * list. + */ + private long openDatatypeCount() throws Throwable + { + long count = H5.H5Fget_obj_count(HDF5Constants.H5F_OBJ_ALL, HDF5Constants.H5F_OBJ_DATATYPE); + assertTrue("H5Fget_obj_count: ", count >= 0); + return count; + } + + /* + * Reading or writing a dataset of a variable-length type through the object-tree path + * must not accumulate open datatype IDs. + */ + @Test + public void testH5DVL_datatype_ids_stable() throws Throwable + { + long dtype_id = HDF5Constants.H5I_INVALID_HID; + long dspace_id = HDF5Constants.H5I_INVALID_HID; + long dset_id = HDF5Constants.H5I_INVALID_HID; + long[] dims = {2}; + final int NITER = 50; + ArrayList[] wbuf = new ArrayList[2]; + + try { + wbuf[0] = new ArrayList(Arrays.asList(1)); + wbuf[1] = new ArrayList(Arrays.asList(2, 3)); + + dtype_id = H5.H5Tvlen_create(HDF5Constants.H5T_NATIVE_INT); + assertTrue("testH5DVL_datatype_ids_stable.H5Tvlen_create: ", dtype_id >= 0); + dspace_id = H5.H5Screate_simple(1, dims, null); + assertTrue("testH5DVL_datatype_ids_stable.H5Screate_simple: ", dspace_id >= 0); + dset_id = H5.H5Dcreate(H5fid, "VLIntIdCount", dtype_id, dspace_id, HDF5Constants.H5P_DEFAULT, + HDF5Constants.H5P_DEFAULT, HDF5Constants.H5P_DEFAULT); + assertTrue("testH5DVL_datatype_ids_stable.H5Dcreate: ", dset_id >= 0); + + long before = openDatatypeCount(); + + for (int i = 0; i < NITER; i++) { + H5.H5DwriteVL(dset_id, dtype_id, HDF5Constants.H5S_ALL, HDF5Constants.H5S_ALL, + HDF5Constants.H5P_DEFAULT, wbuf); + ArrayList[] rbuf = new ArrayList[2]; + H5.H5DreadVL(dset_id, dtype_id, HDF5Constants.H5S_ALL, HDF5Constants.H5S_ALL, + HDF5Constants.H5P_DEFAULT, rbuf); + assertEquals("testH5DVL_datatype_ids_stable: row 0", Integer.valueOf(1), rbuf[0].get(0)); + } + + assertEquals("testH5DVL_datatype_ids_stable: open datatype count changed after " + NITER + + " write/read cycles on a variable-length datatype", + before, openDatatypeCount()); + } + finally { + if (dset_id >= 0) + try { + H5.H5Dclose(dset_id); + } + catch (Exception ex) { + } + if (dspace_id >= 0) + try { + H5.H5Sclose(dspace_id); + } + catch (Exception ex) { + } + if (dtype_id >= 0) + try { + H5.H5Tclose(dtype_id); + } + catch (Exception ex) { + } + } + } + + /* + * Reading or writing a dataset of an array type through the object-tree path must not + * accumulate open datatype IDs. + */ + @Test + public void testH5DArrayVL_datatype_ids_stable() throws Throwable + { + long dtype_id = HDF5Constants.H5I_INVALID_HID; + long dspace_id = HDF5Constants.H5I_INVALID_HID; + long dset_id = HDF5Constants.H5I_INVALID_HID; + long[] dims = {2}; + long[] arr_dims = {3}; + final int NITER = 50; + ArrayList[] wbuf = new ArrayList[2]; + + try { + wbuf[0] = new ArrayList(Arrays.asList(1, 2, 3)); + wbuf[1] = new ArrayList(Arrays.asList(4, 5, 6)); + + dtype_id = H5.H5Tarray_create(HDF5Constants.H5T_NATIVE_INT, 1, arr_dims); + assertTrue("testH5DArrayVL_datatype_ids_stable.H5Tarray_create: ", dtype_id >= 0); + dspace_id = H5.H5Screate_simple(1, dims, null); + assertTrue("testH5DArrayVL_datatype_ids_stable.H5Screate_simple: ", dspace_id >= 0); + dset_id = H5.H5Dcreate(H5fid, "ArrayIntIdCount", dtype_id, dspace_id, HDF5Constants.H5P_DEFAULT, + HDF5Constants.H5P_DEFAULT, HDF5Constants.H5P_DEFAULT); + assertTrue("testH5DArrayVL_datatype_ids_stable.H5Dcreate: ", dset_id >= 0); + + long before = openDatatypeCount(); + + for (int i = 0; i < NITER; i++) { + H5.H5DwriteVL(dset_id, dtype_id, HDF5Constants.H5S_ALL, HDF5Constants.H5S_ALL, + HDF5Constants.H5P_DEFAULT, wbuf); + ArrayList[] rbuf = new ArrayList[2]; + H5.H5DreadVL(dset_id, dtype_id, HDF5Constants.H5S_ALL, HDF5Constants.H5S_ALL, + HDF5Constants.H5P_DEFAULT, rbuf); + assertEquals("testH5DArrayVL_datatype_ids_stable: row 0", Integer.valueOf(1), rbuf[0].get(0)); + } + + assertEquals("testH5DArrayVL_datatype_ids_stable: open datatype count changed after " + NITER + + " write/read cycles on an array datatype", + before, openDatatypeCount()); + } + finally { + if (dset_id >= 0) + try { + H5.H5Dclose(dset_id); + } + catch (Exception ex) { + } + if (dspace_id >= 0) + try { + H5.H5Sclose(dspace_id); + } + catch (Exception ex) { + } + if (dtype_id >= 0) + try { + H5.H5Tclose(dtype_id); + } + catch (Exception ex) { + } + } + } + + /* + * A compound member that is itself a variable-length type is converted by the + * per-element helpers. Reading or writing such a dataset must not accumulate + * open datatype IDs. + */ + @Test + public void testH5Dcompound_of_vlen_datatype_ids_stable() throws Throwable + { + long vlen_tid = HDF5Constants.H5I_INVALID_HID; + long cmpd_tid = HDF5Constants.H5I_INVALID_HID; + long dspace_id = HDF5Constants.H5I_INVALID_HID; + long dset_id = HDF5Constants.H5I_INVALID_HID; + long[] dims = {2}; + final int NITER = 50; + ArrayList[] wbuf = new ArrayList[2]; + + try { + long intSize = H5.H5Tget_size(HDF5Constants.H5T_NATIVE_INT); + + vlen_tid = H5.H5Tvlen_create(HDF5Constants.H5T_NATIVE_INT); + assertTrue("testH5Dcompound_of_vlen_datatype_ids_stable.H5Tvlen_create: ", vlen_tid >= 0); + cmpd_tid = H5.H5Tcreate(HDF5Constants.H5T_COMPOUND, intSize + H5.H5Tget_size(vlen_tid)); + assertTrue("testH5Dcompound_of_vlen_datatype_ids_stable.H5Tcreate: ", cmpd_tid >= 0); + H5.H5Tinsert(cmpd_tid, "A", 0, HDF5Constants.H5T_NATIVE_INT); + H5.H5Tinsert(cmpd_tid, "B", intSize, vlen_tid); + + dspace_id = H5.H5Screate_simple(1, dims, null); + assertTrue("testH5Dcompound_of_vlen_datatype_ids_stable.H5Screate_simple: ", dspace_id >= 0); + dset_id = H5.H5Dcreate(H5fid, "CmpdVLIdCount", cmpd_tid, dspace_id, HDF5Constants.H5P_DEFAULT, + HDF5Constants.H5P_DEFAULT, HDF5Constants.H5P_DEFAULT); + assertTrue("testH5Dcompound_of_vlen_datatype_ids_stable.H5Dcreate: ", dset_id >= 0); + + for (int r = 0; r < dims[0]; r++) { + ArrayList row = new ArrayList<>(); + row.add(Integer.valueOf(r)); + row.add(new ArrayList(Arrays.asList(r + 1, r + 2))); + wbuf[r] = row; + } + + long before = openDatatypeCount(); + + for (int i = 0; i < NITER; i++) { + H5.H5DwriteVL(dset_id, cmpd_tid, HDF5Constants.H5S_ALL, HDF5Constants.H5S_ALL, + HDF5Constants.H5P_DEFAULT, wbuf); + ArrayList[] rbuf = new ArrayList[2]; + H5.H5DreadVL(dset_id, cmpd_tid, HDF5Constants.H5S_ALL, HDF5Constants.H5S_ALL, + HDF5Constants.H5P_DEFAULT, rbuf); + assertEquals("testH5Dcompound_of_vlen_datatype_ids_stable: row 0 member A", + Integer.valueOf(0), rbuf[0].get(0)); + } + + assertEquals("testH5Dcompound_of_vlen_datatype_ids_stable: open datatype count changed after " + + NITER + " write/read cycles on a compound with a variable-length member", + before, openDatatypeCount()); + } + finally { + if (dset_id >= 0) + try { + H5.H5Dclose(dset_id); + } + catch (Exception ex) { + } + if (dspace_id >= 0) + try { + H5.H5Sclose(dspace_id); + } + catch (Exception ex) { + } + if (cmpd_tid >= 0) + try { + H5.H5Tclose(cmpd_tid); + } + catch (Exception ex) { + } + if (vlen_tid >= 0) + try { + H5.H5Tclose(vlen_tid); + } + catch (Exception ex) { + } + } + } + @Test public void testH5DArray_string_buffer() throws Throwable { diff --git a/java/src-jni/test/testfiles/JUnit-TestH5D.txt b/java/src-jni/test/testfiles/JUnit-TestH5D.txt index 130a72107ba..af0fc3d49e1 100644 --- a/java/src-jni/test/testfiles/JUnit-TestH5D.txt +++ b/java/src-jni/test/testfiles/JUnit-TestH5D.txt @@ -1,6 +1,7 @@ JUnit version 4.13.2 .testH5DArrayenum_rw .testH5DVLwrVL +.testH5DArrayVL_datatype_ids_stable .testH5Dget_storage_size .testH5DArraywr .testH5Diterate_write @@ -18,6 +19,7 @@ JUnit version 4.13.2 .testH5Dget_access_plist .testH5Dread_vlen_of_nested_compound .testH5DArray_datatype_ids_stable +.testH5Dcompound_of_vlen_datatype_ids_stable .testH5DArray_string_buffer_flat_StringArray .testH5Dget_space_closed .testH5DArray_string_buffer_flat_StringArray_write @@ -31,9 +33,10 @@ JUnit version 4.13.2 .testH5DwriteVL_vlen_element_not_arraylist .testH5Dget_type_closed .testH5Dwrite_compound_of_vlen +.testH5DVL_datatype_ids_stable .testH5DwriteVL_undersized_buffer Time: XXXX -OK (33 tests) +OK (36 tests) diff --git a/release_docs/CHANGELOG.md b/release_docs/CHANGELOG.md index bf9d053f66b..1fd4d0da478 100644 --- a/release_docs/CHANGELOG.md +++ b/release_docs/CHANGELOG.md @@ -104,6 +104,12 @@ We would like to thank the many HDF5 community members who contributed to this r ## Java Library +### Fixed datatype ID leaks when reading or writing nested datatypes through the JNI + + The object-tree read and write helpers in the JNI derived a base datatype from the memory type with `H5Tget_super()` for the variable-length, array and complex classes, but never closed it. Because an `hid_t` is not reclaimed when a native method returns, every read or write of such data leaked at least one datatype ID for the lifetime of the process, and a nested type leaked one per level. The helpers now close the derived type on both the success and error paths. + + Fixes GitHub issue #6592 + ## Configuration ### Fixed version handling in installed CMake package version configuration file