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

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

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<Integer>(Arrays.asList(1));
+            wbuf[1] = new ArrayList<Integer>(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<Integer>(Arrays.asList(1, 2, 3));
+            wbuf[1] = new ArrayList<Integer>(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<Object> row = new ArrayList<>();
+                row.add(Integer.valueOf(r));
+                row.add(new ArrayList<Integer>(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
     {

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)
 

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