Expand check for variable-length or reference types when clearing datatype conversion paths (#4085)

When clearing out datatype conversion paths involving variable-length or reference datatypes
on file close, also check for these datatypes inside compound or array datatypes

2 files changed, 225 insertions, 60 deletions

diff --git a/src/H5T.c b/src/H5T.c
index bfe31b8..31eeb67 100644
--- a/src/H5T.c
+++ b/src/H5T.c
@@ -350,12 +350,13 @@ static H5T_path_t *H5T__path_find_real(const H5T_t *src, const H5T_t *dst, const
                                        H5T_conv_func_t *conv);
 static bool        H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst,
                                   H5VL_object_t *owned_vol_obj, H5T_conv_t func);
-static bool        H5T__detect_vlen_ref(const H5T_t *dt);
-static H5T_t      *H5T__initiate_copy(const H5T_t *old_dt);
-static H5T_t      *H5T__copy_transient(H5T_t *old_dt);
-static H5T_t      *H5T__copy_all(H5T_t *old_dt);
-static herr_t      H5T__complete_copy(H5T_t *new_dt, const H5T_t *old_dt, H5T_shared_t *reopened_fo,
-                                      bool set_memory_type, H5T_copy_func_t copyfn);
+static bool   H5T_path_match_find_type_with_volobj(const H5T_t *datatype, const H5VL_object_t *owned_vol_obj);
+static bool   H5T__detect_vlen_ref(const H5T_t *dt);
+static H5T_t *H5T__initiate_copy(const H5T_t *old_dt);
+static H5T_t *H5T__copy_transient(H5T_t *old_dt);
+static H5T_t *H5T__copy_all(H5T_t *old_dt);
+static herr_t H5T__complete_copy(H5T_t *new_dt, const H5T_t *old_dt, H5T_shared_t *reopened_fo,
+                                 bool set_memory_type, H5T_copy_func_t copyfn);
 
 /*****************************/
 /* Library Private Variables */
@@ -5183,10 +5184,10 @@ H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src,
 {
     bool ret_value = true;
 
-    assert(path);
-
     FUNC_ENTER_NOAPI_NOINIT_NOERR
 
+    assert(path);
+
     if (
         /* Check that the specified conversion function persistence matches */
         ((H5T_PERS_SOFT == pers && path->is_hard) || (H5T_PERS_HARD == pers && !path->is_hard)) ||
@@ -5201,11 +5202,12 @@ H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src,
         (src && H5T_cmp(src, path->src, false)) || (dst && H5T_cmp(dst, path->dst, false)) ||
 
         /*
-         * Check that the specified VOL object matches the VOL object
-         * in the conversion path
+         * Check that the specified VOL object pointer matches the `owned_vol_obj`
+         * field for either the source datatype or destination datatype in the
+         * conversion path
          */
-        (owned_vol_obj && (owned_vol_obj != path->src->shared->owned_vol_obj) &&
-         (owned_vol_obj != path->dst->shared->owned_vol_obj)) ||
+        (owned_vol_obj && (H5T_path_match_find_type_with_volobj(path->src, owned_vol_obj) == false) &&
+         (H5T_path_match_find_type_with_volobj(path->dst, owned_vol_obj) == false)) ||
 
         /* Check that the specified conversion function matches */
         (func && func != path->conv.u.app_func))
@@ -5215,6 +5217,70 @@ H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src,
 } /* H5T_path_match() */
 
 /*-------------------------------------------------------------------------
+ * Function:  H5T_path_match_find_type_with_volobj
+ *
+ * Purpose:   Helper function to determine whether a datatype is or
+ *            contains a datatype that has a VOL object pointer matching
+ *            the given VOL object pointer.
+ *
+ * Return:    true/false (can't fail)
+ *
+ *-------------------------------------------------------------------------
+ */
+static bool
+H5T_path_match_find_type_with_volobj(const H5T_t *datatype, const H5VL_object_t *owned_vol_obj)
+{
+    bool ret_value = false;
+
+    FUNC_ENTER_NOAPI_NOINIT_NOERR
+
+    assert(datatype);
+    assert(owned_vol_obj);
+
+    ret_value = (datatype->shared->owned_vol_obj == owned_vol_obj);
+    if (!ret_value) {
+        switch (datatype->shared->type) {
+            case H5T_COMPOUND:
+                for (unsigned i = 0; i < datatype->shared->u.compnd.nmembs; i++) {
+                    if (ret_value)
+                        break;
+                    ret_value = H5T_path_match_find_type_with_volobj(datatype->shared->u.compnd.memb[i].type,
+                                                                     owned_vol_obj);
+                }
+                break;
+
+            case H5T_VLEN:
+                /* Should be an error if no parent, but simplify logic for a true/false return value */
+                if (datatype->shared->parent)
+                    ret_value = H5T_path_match_find_type_with_volobj(datatype->shared->parent, owned_vol_obj);
+                break;
+
+            case H5T_ARRAY:
+                /* Should be an error if no parent, but simplify logic for a true/false return value */
+                if (datatype->shared->parent)
+                    ret_value = H5T_path_match_find_type_with_volobj(datatype->shared->parent, owned_vol_obj);
+                break;
+
+            case H5T_INTEGER:
+            case H5T_FLOAT:
+            case H5T_TIME:
+            case H5T_STRING:
+            case H5T_BITFIELD:
+            case H5T_OPAQUE:
+            case H5T_REFERENCE: /* Should have been determined by above check */
+            case H5T_ENUM:
+            case H5T_NO_CLASS: /* Error value, but simplify logic for a true/false return value */
+            case H5T_NCLASSES: /* Error value, but simplify logic for a true/false return value */
+            default:
+                ret_value = false;
+                break;
+        }
+    }
+
+    FUNC_LEAVE_NOAPI(ret_value)
+}
+
+/*-------------------------------------------------------------------------
  * Function:  H5T_path_noop
  *
  * Purpose:   Is the path the special no-op path? The no-op function can be
diff --git a/test/tmisc.c b/test/tmisc.c
index 4f33758..0022aa6 100644
--- a/test/tmisc.c
+++ b/test/tmisc.c
@@ -6274,17 +6274,32 @@ test_misc37(void)
 static void
 test_misc38(void)
 {
-    H5VL_object_t *file_vol_obj = NULL;
-    const char    *buf[]        = {"attr_value"};
-    herr_t         ret          = SUCCEED;
-    hid_t          file_id      = H5I_INVALID_HID;
-    hid_t          attr_id      = H5I_INVALID_HID;
-    hid_t          str_type     = H5I_INVALID_HID;
-    hid_t          space_id     = H5I_INVALID_HID;
-    int            init_npaths  = 0;
-    int           *irbuf        = NULL;
-    char         **rbuf         = NULL;
+    H5VL_object_t *file_vol_obj  = NULL;
+    const char    *buf[]         = {"attr_value"};
+    const char    *array_buf[]   = {"attr_value1", "attr_value2"};
+    hsize_t        array_dims[1] = {2};
+    herr_t         ret           = SUCCEED;
+    hid_t          file_id       = H5I_INVALID_HID;
+    hid_t          attr_id1      = H5I_INVALID_HID;
+    hid_t          attr_id2      = H5I_INVALID_HID;
+    hid_t          attr_id3      = H5I_INVALID_HID;
+    hid_t          attr_id4      = H5I_INVALID_HID;
+    hid_t          str_type      = H5I_INVALID_HID;
+    hid_t          array_type    = H5I_INVALID_HID;
+    hid_t          compound_type = H5I_INVALID_HID;
+    hid_t          vlen_type     = H5I_INVALID_HID;
+    hid_t          space_id      = H5I_INVALID_HID;
+    int            init_npaths   = 0;
+    char         **rbuf          = NULL;
+    char         **arr_rbuf      = NULL;
     bool           vol_is_native;
+    typedef struct struct_type {
+        const char *buf;
+    } struct_type;
+    struct_type  cbuf          = {.buf = "attr_value"};
+    struct_type *compound_rbuf = NULL;
+    hvl_t        vlen_buf      = {.len = 2, .p = array_buf};
+    hvl_t       *vlen_rbuf     = NULL;
 
     /* Output message about test being performed */
     MESSAGE(5, ("Fix for type conversion path table issue"));
@@ -6316,12 +6331,27 @@ test_misc38(void)
      */
     VERIFY(file_vol_obj->rc, 1, "checking reference count");
 
+    /* Create a variable-length string type */
     str_type = H5Tcopy(H5T_C_S1);
     CHECK(str_type, H5I_INVALID_HID, "H5Tcopy");
 
     ret = H5Tset_size(str_type, H5T_VARIABLE);
     CHECK(ret, FAIL, "H5Tset_size");
 
+    /* Create an array type of the string type */
+    array_type = H5Tarray_create2(str_type, 1, array_dims);
+    CHECK(array_type, H5I_INVALID_HID, "H5Tarray_create2");
+
+    /* Create a compound type of the string type */
+    compound_type = H5Tcreate(H5T_COMPOUND, sizeof(compound_type));
+    CHECK(compound_type, H5I_INVALID_HID, "H5Tcreate(H5T_COMPOUND, ...)");
+
+    CHECK(H5Tinsert(compound_type, "varstr", HOFFSET(struct_type, buf), str_type), FAIL, "H5Tinsert");
+
+    /* Create a variable-length type of the string type */
+    vlen_type = H5Tvlen_create(str_type);
+    CHECK(vlen_type, H5I_INVALID_HID, "H5Tvlen_create");
+
     space_id = H5Screate(H5S_SCALAR);
     CHECK(space_id, H5I_INVALID_HID, "H5Screate");
 
@@ -6332,8 +6362,19 @@ test_misc38(void)
     VERIFY(H5T__get_path_table_npaths(), init_npaths,
            "checking number of type conversion path table entries");
 
-    attr_id = H5Acreate2(file_id, "attribute", str_type, space_id, H5P_DEFAULT, H5P_DEFAULT);
-    CHECK(attr_id, H5I_INVALID_HID, "H5Acreate2");
+    /* Increments file's VOL object reference count by 1 */
+    attr_id1 = H5Acreate2(file_id, "varstr_attribute", str_type, space_id, H5P_DEFAULT, H5P_DEFAULT);
+    CHECK(attr_id1, H5I_INVALID_HID, "H5Acreate2");
+    /* Increments file's VOL object reference count by 1 */
+    attr_id2 = H5Acreate2(file_id, "array_varstr_attribute", array_type, space_id, H5P_DEFAULT, H5P_DEFAULT);
+    CHECK(attr_id2, H5I_INVALID_HID, "H5Acreate2");
+    /* Increments file's VOL object reference count by 1 */
+    attr_id3 =
+        H5Acreate2(file_id, "compound_varstr_attribute", compound_type, space_id, H5P_DEFAULT, H5P_DEFAULT);
+    CHECK(attr_id3, H5I_INVALID_HID, "H5Acreate2");
+    /* Increments file's VOL object reference count by 2 */
+    attr_id4 = H5Acreate2(file_id, "vlen_varstr_attribute", vlen_type, space_id, H5P_DEFAULT, H5P_DEFAULT);
+    CHECK(attr_id4, H5I_INVALID_HID, "H5Acreate2");
 
     /*
      * Check the number of type conversion path table entries currently
@@ -6344,44 +6385,64 @@ test_misc38(void)
 
     /*
      * Check reference count of file's VOL object field. At this point,
-     * the object should have a reference count of 2. Creating the
-     * attribute on the dataset will have caused a H5T_set_loc call that
-     * associates the attribute's datatype with the file's VOL object
-     * and will have incremented the reference count by 1.
+     * the object should have a reference count of 6. Creating the
+     * attributes in the file will have caused H5T_set_loc calls that
+     * associate each attribute's datatype with the file's VOL object
+     * and will have incremented the reference count by 5.
      */
-    VERIFY(file_vol_obj->rc, 2, "checking reference count");
+    VERIFY(file_vol_obj->rc, 6, "checking reference count");
 
-    ret = H5Awrite(attr_id, str_type, buf);
+    /* Increments file's VOL object reference count by 1 */
+    ret = H5Awrite(attr_id1, str_type, buf);
+    CHECK(ret, FAIL, "H5Awrite");
+    /* Increments file's VOL object reference count by 1 */
+    ret = H5Awrite(attr_id2, array_type, array_buf);
+    CHECK(ret, FAIL, "H5Awrite");
+    /* Increments file's VOL object reference count by 2 */
+    ret = H5Awrite(attr_id3, compound_type, &cbuf);
+    CHECK(ret, FAIL, "H5Awrite");
+    /* Increments file's VOL object reference count by 2 */
+    ret = H5Awrite(attr_id4, vlen_type, &vlen_buf);
     CHECK(ret, FAIL, "H5Awrite");
 
     /*
      * Check the number of type conversion path table entries currently
-     * stored in the cache. The H5Awrite call should have added a new
-     * type conversion path. Note that if another test in this file uses
-     * the same conversion path, this check may fail and need to be
-     * refactored.
+     * stored in the cache. The H5Awrite calls should have added new
+     * type conversion paths. Note that if another test in this file
+     * uses the same conversion path, this check may fail and need to
+     * be refactored.
      */
-    VERIFY(H5T__get_path_table_npaths(), init_npaths + 1,
+    VERIFY(H5T__get_path_table_npaths(), init_npaths + 4,
            "checking number of type conversion path table entries");
 
     /*
      * Check reference count of file's VOL object field. At this point,
-     * the object should have a reference count of 3. Writing to the
-     * variable-length typed attribute will have caused an H5T_convert
-     * call that ends up incrementing the reference count of the
-     * associated file's VOL object.
+     * the object should have a reference count of 12. Writing to the
+     * attributes will have caused H5T_path_find calls that end up
+     * incrementing the reference count of the associated file's VOL
+     * object.
      */
-    VERIFY(file_vol_obj->rc, 3, "checking reference count");
+    VERIFY(file_vol_obj->rc, 12, "checking reference count");
 
-    ret = H5Aclose(attr_id);
+    ret = H5Aclose(attr_id1);
+    CHECK(ret, FAIL, "H5Aclose");
+    ret = H5Aclose(attr_id2);
+    CHECK(ret, FAIL, "H5Aclose");
+    ret = H5Aclose(attr_id3);
+    CHECK(ret, FAIL, "H5Aclose");
+    ret = H5Aclose(attr_id4);
     CHECK(ret, FAIL, "H5Aclose");
     ret = H5Fclose(file_id);
     CHECK(ret, FAIL, "H5Fclose");
 
-    irbuf = malloc(100 * 100 * sizeof(int));
-    CHECK_PTR(irbuf, "int read buf allocation");
     rbuf = malloc(sizeof(char *));
     CHECK_PTR(rbuf, "varstr read buf allocation");
+    arr_rbuf = malloc(array_dims[0] * sizeof(char *));
+    CHECK_PTR(arr_rbuf, "array varstr read buf allocation");
+    compound_rbuf = malloc(sizeof(struct_type));
+    CHECK_PTR(compound_rbuf, "compound varstr read buf allocation");
+    vlen_rbuf = malloc(sizeof(hvl_t));
+    CHECK_PTR(vlen_rbuf, "vlen varstr read buf allocation");
 
     for (size_t i = 0; i < 10; i++) {
         file_id = H5Fopen(MISC38_FILE, H5F_ACC_RDONLY, H5P_DEFAULT);
@@ -6397,51 +6458,89 @@ test_misc38(void)
          */
         VERIFY(file_vol_obj->rc, 1, "checking reference count");
 
-        attr_id = H5Aopen(file_id, "attribute", H5P_DEFAULT);
-        CHECK(attr_id, H5I_INVALID_HID, "H5Aopen");
+        /* Increments file's VOL object reference count by 1 */
+        attr_id1 = H5Aopen(file_id, "varstr_attribute", H5P_DEFAULT);
+        CHECK(attr_id1, H5I_INVALID_HID, "H5Aopen");
+        /* Increments file's VOL object reference count by 1 */
+        attr_id2 = H5Aopen(file_id, "array_varstr_attribute", H5P_DEFAULT);
+        CHECK(attr_id2, H5I_INVALID_HID, "H5Aopen");
+        /* Increments file's VOL object reference count by 1 */
+        attr_id3 = H5Aopen(file_id, "compound_varstr_attribute", H5P_DEFAULT);
+        CHECK(attr_id3, H5I_INVALID_HID, "H5Aopen");
+        /* Increments file's VOL object reference count by 2 */
+        attr_id4 = H5Aopen(file_id, "vlen_varstr_attribute", H5P_DEFAULT);
+        CHECK(attr_id4, H5I_INVALID_HID, "H5Aopen");
 
         /*
          * Check reference count of file's VOL object field. At this point,
-         * the object should have a reference count of 2 since opening
-         * the attribute will also have associated its type with the file's
-         * VOL object.
+         * the object should have a reference count of 6 since opening
+         * the attributes will also have associated their datatypes with
+         * the file's VOL object.
          */
-        VERIFY(file_vol_obj->rc, 2, "checking reference count");
+        VERIFY(file_vol_obj->rc, 6, "checking reference count");
 
-        ret = H5Aread(attr_id, str_type, rbuf);
+        /* Increments file's VOL object reference count by 1 */
+        ret = H5Aread(attr_id1, str_type, rbuf);
+        CHECK(ret, FAIL, "H5Aread");
+        /* Increments file's VOL object reference count by 1 */
+        ret = H5Aread(attr_id2, array_type, arr_rbuf);
+        CHECK(ret, FAIL, "H5Aread");
+        /* Increments file's VOL object reference count by 2 */
+        ret = H5Aread(attr_id3, compound_type, compound_rbuf);
+        CHECK(ret, FAIL, "H5Aread");
+        /* Increments file's VOL object reference count by 2 */
+        ret = H5Aread(attr_id4, vlen_type, vlen_rbuf);
         CHECK(ret, FAIL, "H5Aread");
 
         /*
          * Check the number of type conversion path table entries currently
          * stored in the cache. Each H5Aread call shouldn't cause this number
-         * to go up, as the library should have removed the cached conversion
-         * paths on file close.
+         * to keep going up, as the library should remove the cached conversion
+         * paths on file close during each iteration. The value should stay at
+         * a constant "initial_num_paths + number of H5Aread calls above".
          */
-        VERIFY(H5T__get_path_table_npaths(), init_npaths + 1,
+        VERIFY(H5T__get_path_table_npaths(), init_npaths + 4,
                "checking number of type conversion path table entries");
 
         /*
          * Check reference count of file's VOL object field. At this point,
-         * the object should have a reference count of 3. Writing to the
-         * variable-length typed attribute will have caused an H5T_convert
-         * call that ends up incrementing the reference count of the
-         * associated file's VOL object.
+         * the object should have a reference count of 12. Reading from the
+         * attributes will have caused H5T_path_find calls that end up
+         * incrementing the reference count of the associated file's VOL
+         * object.
          */
-        VERIFY(file_vol_obj->rc, 3, "checking reference count");
+        VERIFY(file_vol_obj->rc, 12, "checking reference count");
 
         ret = H5Treclaim(str_type, space_id, H5P_DEFAULT, rbuf);
+        ret = H5Treclaim(array_type, space_id, H5P_DEFAULT, arr_rbuf);
+        ret = H5Treclaim(compound_type, space_id, H5P_DEFAULT, compound_rbuf);
+        ret = H5Treclaim(vlen_type, space_id, H5P_DEFAULT, vlen_rbuf);
 
-        ret = H5Aclose(attr_id);
+        ret = H5Aclose(attr_id1);
+        CHECK(ret, FAIL, "H5Aclose");
+        ret = H5Aclose(attr_id2);
+        CHECK(ret, FAIL, "H5Aclose");
+        ret = H5Aclose(attr_id3);
+        CHECK(ret, FAIL, "H5Aclose");
+        ret = H5Aclose(attr_id4);
         CHECK(ret, FAIL, "H5Aclose");
         ret = H5Fclose(file_id);
         CHECK(ret, FAIL, "H5Fclose");
     }
 
-    free(irbuf);
     free(rbuf);
+    free(arr_rbuf);
+    free(compound_rbuf);
+    free(vlen_rbuf);
 
     ret = H5Tclose(str_type);
     CHECK(ret, FAIL, "H5Tclose");
+    ret = H5Tclose(array_type);
+    CHECK(ret, FAIL, "H5Tclose");
+    ret = H5Tclose(compound_type);
+    CHECK(ret, FAIL, "H5Tclose");
+    ret = H5Tclose(vlen_type);
+    CHECK(ret, FAIL, "H5Tclose");
     ret = H5Sclose(space_id);
     CHECK(ret, FAIL, "H5Sclose");
 }