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