Fix error when overwriting an indirectly nested vlen with a shorter sequence (#4140)

3 files changed, 243 insertions, 2 deletions

diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index 688762e..2d8eb32 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -591,6 +591,13 @@ Bug Fixes since HDF5-1.14.0 release
 
     Library
     -------
+    - Fixed error when overwriting certain nested variable length types
+
+      Previously, when using a datatype that included a variable length type
+      within a compound or array within another variable length type, and
+      overwriting data with a shorter (top level) variable length sequence, an
+      error could occur. This has been fixed.
+
     - Fixed asserts raised by large values of H5Pset_est_link_info() parameters
 
       If large values for est_num_entries and/or est_name_len were passed
@@ -1543,6 +1550,9 @@ Known Problems
     in the HDF5 source. Please report any new problems found to
     help@hdfgroup.org.
 
+    File space may not be released when overwriting or deleting certain nested
+    variable length or reference types.
+
 
 CMake vs. Autotools installations
 =================================
diff --git a/src/H5Tconv.c b/src/H5Tconv.c
index 9a9d656..a37800b 100644
--- a/src/H5Tconv.c
+++ b/src/H5Tconv.c
@@ -3149,6 +3149,67 @@ done:
 } /* end H5T__conv_enum_numeric() */
 
 /*-------------------------------------------------------------------------
+ * Function:    H5T__conv_vlen_nested_free
+ *
+ * Purpose:     Recursively locates and frees any nested VLEN components of
+ *              complex data types (including COMPOUND).
+ *
+ * Return:      Non-negative on success/Negative on failure.
+ *
+ *-------------------------------------------------------------------------
+ */
+static herr_t
+H5T__conv_vlen_nested_free(uint8_t *buf, H5T_t *dt)
+{
+    herr_t ret_value = SUCCEED; /* Return value */
+
+    FUNC_ENTER_PACKAGE
+
+    switch (dt->shared->type) {
+        case H5T_VLEN:
+            /* Pointer buf refers to VLEN data; free it (always reset tmp) */
+            if ((*(dt->shared->u.vlen.cls->del))(dt->shared->u.vlen.file, buf) < 0)
+                HGOTO_ERROR(H5E_DATATYPE, H5E_CANTFREE, FAIL, "can't free nested vlen");
+            break;
+
+        case H5T_COMPOUND:
+            /* Pointer buf refers to COMPOUND data; recurse for each member. */
+            for (unsigned i = 0; i < dt->shared->u.compnd.nmembs; ++i)
+                if (H5T__conv_vlen_nested_free(buf + dt->shared->u.compnd.memb[i].offset,
+                                               dt->shared->u.compnd.memb[i].type) < 0)
+                    HGOTO_ERROR(H5E_DATATYPE, H5E_CANTFREE, FAIL, "can't free compound member");
+            break;
+
+        case H5T_ARRAY:
+            /* Pointer buf refers to ARRAY data; recurse for each element. */
+            for (unsigned i = 0; i < dt->shared->u.array.nelem; ++i)
+                if (H5T__conv_vlen_nested_free(buf + i * dt->shared->parent->shared->size,
+                                               dt->shared->parent) < 0)
+                    HGOTO_ERROR(H5E_DATATYPE, H5E_CANTFREE, FAIL, "can't free array data");
+            break;
+
+        case H5T_INTEGER:
+        case H5T_FLOAT:
+        case H5T_TIME:
+        case H5T_STRING:
+        case H5T_BITFIELD:
+        case H5T_OPAQUE:
+        case H5T_REFERENCE:
+        case H5T_ENUM:
+            /* These types cannot contain vl data */
+            break;
+
+        case H5T_NO_CLASS:
+        case H5T_NCLASSES:
+        default:
+            HGOTO_ERROR(H5E_DATATYPE, H5E_BADTYPE, FAIL, "invalid datatype class");
+    }
+
+done:
+    FUNC_LEAVE_NOAPI(ret_value)
+} /* H5T__conv_vlen_nested_free() */
+
+/*-------------------------------------------------------------------------
  * Function:    H5T__conv_vlen
  *
  * Purpose:    Converts between VL datatypes in memory and on disk.
@@ -3506,8 +3567,8 @@ H5T__conv_vlen(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv_ctx_t
 
                                 tmp = (uint8_t *)tmp_buf + seq_len * dst_base_size;
                                 for (u = seq_len; u < bg_seq_len; u++, tmp += dst_base_size) {
-                                    /* Delete sequence in destination location */
-                                    if ((*(dst->shared->u.vlen.cls->del))(dst->shared->u.vlen.file, tmp) < 0)
+                                    /* Recursively free destination data */
+                                    if (H5T__conv_vlen_nested_free(tmp, dst->shared->parent) < 0)
                                         HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREMOVE, FAIL,
                                                     "unable to remove heap object");
                                 } /* end for */
diff --git a/test/dsets.c b/test/dsets.c
index 5fa74f9..3203ed0 100644
--- a/test/dsets.c
+++ b/test/dsets.c
@@ -15717,6 +15717,174 @@ error:
 } /* end test_0sized_dset_metadata_alloc() */
 
 /*-------------------------------------------------------------------------
+ * Function:    test_downsize_vlen_scalar_dataset
+ *
+ * Purpose:     Tests H5Dwrite on a scalar dataset containing a VLEN array
+ *              of { double, C-string }. This causes special code to free
+ *              the nested VLEN (in this case, C-string) allocations.
+ *
+ * Return:      Success: 0
+ *              Failure: -1
+ *
+ *-------------------------------------------------------------------------
+ */
+#define VLEN_DS_NAME       "test_downsize_vlen_scalar_dataset"
+#define VLEN_DS_DIM        100
+#define VLEN_DS_STRLEN     20
+#define VLEN_DS_STRING     "vlen test string"
+#define VLEN_DS_VALUE      0.12345678901234567890
+#define VLEN_DS_ARRAY_DIM1 3
+#define VLEN_DS_ARRAY_DIM2 5
+
+typedef struct {
+    double value;
+    char  *string[VLEN_DS_ARRAY_DIM1][VLEN_DS_ARRAY_DIM2];
+} vlen_ds_compound_file_t;
+
+typedef struct {
+    int    padding1;
+    double value;
+    int    padding2;
+    char  *string[VLEN_DS_ARRAY_DIM1][VLEN_DS_ARRAY_DIM2];
+    int    padding3;
+} vlen_ds_compound_memory_t;
+
+static herr_t
+test_downsize_vlen_scalar_dataset(hid_t file)
+{
+    hid_t                      scalar_sid               = -1; /* Scalar dataspace ID */
+    hid_t                      string_tid               = -1; /* VARIABLE string datatype ID */
+    hid_t                      string_array_tid         = -1; /* VARIABLE string array datatype ID */
+    hid_t                      compound_file_tid        = -1; /* COMPOUND datatype ID */
+    hid_t                      compound_memory_tid      = -1; /* COMPOUND datatype ID */
+    hid_t                      vlen_compound_file_tid   = -1; /* VARIABLE COMPOUND datatype ID */
+    hid_t                      vlen_compound_memory_tid = -1; /* VARIABLE COMPOUND datatype ID */
+    hid_t                      scalar_did               = -1; /* SCALAR dataset ID */
+    hvl_t                      vlen_compound_data;            /* Top-level VLEN data */
+    vlen_ds_compound_memory_t *compound_data = NULL;          /* Contents of VLEN data */
+    char                       common_string[VLEN_DS_STRLEN]; /* Common string contents */
+    hsize_t                    array_dims[2] = {VLEN_DS_ARRAY_DIM1, VLEN_DS_ARRAY_DIM2};
+    int                        i, dim1, dim2; /* Local index variables */
+
+    TESTING("H5Dwrite() on down-sized VLEN contents");
+
+    /* Allocate space for compound data */
+    if (NULL == (compound_data =
+                     (vlen_ds_compound_memory_t *)malloc(VLEN_DS_DIM * sizeof(vlen_ds_compound_memory_t))))
+        TEST_ERROR;
+
+    /* Create scalar dataspace */
+    if ((scalar_sid = H5Screate(H5S_SCALAR)) < 0)
+        TEST_ERROR;
+
+    /* Create datatype VLEN{COMPOUND{"value":H5T_NATIVE_DOUBLE, "string":H5T_C_S1|H5T_VARIABLE}} */
+    /* Note: the memory and file structures must be different to invoke the bug @ H5Tconv.c:3323 */
+    if ((string_tid = H5Tcopy(H5T_C_S1)) < 0)
+        TEST_ERROR;
+    if (H5Tset_size(string_tid, H5T_VARIABLE) < 0)
+        TEST_ERROR;
+
+    if ((string_array_tid = H5Tarray_create2(string_tid, 2, array_dims)) < 0)
+        TEST_ERROR;
+
+    if ((compound_file_tid = H5Tcreate(H5T_COMPOUND, sizeof(vlen_ds_compound_file_t))) < 0)
+        TEST_ERROR;
+    if (H5Tinsert(compound_file_tid, "value", HOFFSET(vlen_ds_compound_file_t, value), H5T_NATIVE_DOUBLE) < 0)
+        TEST_ERROR;
+    if (H5Tinsert(compound_file_tid, "string", HOFFSET(vlen_ds_compound_file_t, string), string_array_tid) <
+        0)
+        TEST_ERROR;
+    if ((vlen_compound_file_tid = H5Tvlen_create(compound_file_tid)) < 0)
+        TEST_ERROR;
+
+    if ((compound_memory_tid = H5Tcreate(H5T_COMPOUND, sizeof(vlen_ds_compound_memory_t))) < 0)
+        TEST_ERROR;
+    if (H5Tinsert(compound_memory_tid, "value", HOFFSET(vlen_ds_compound_memory_t, value),
+                  H5T_NATIVE_DOUBLE) < 0)
+        TEST_ERROR;
+    if (H5Tinsert(compound_memory_tid, "string", HOFFSET(vlen_ds_compound_memory_t, string),
+                  string_array_tid) < 0)
+        TEST_ERROR;
+    if ((vlen_compound_memory_tid = H5Tvlen_create(compound_memory_tid)) < 0)
+        TEST_ERROR;
+
+    /* Create the scalar dataset of this data type */
+    if ((scalar_did = H5Dcreate2(file, VLEN_DS_NAME, vlen_compound_file_tid, scalar_sid, H5P_DEFAULT,
+                                 H5P_DEFAULT, H5P_DEFAULT)) < 0)
+        TEST_ERROR;
+
+    /* Setup the variable-length data. Note that if the double "value" field is set to 0.0, the bug will NOT
+     */
+    /* occur because this is the data at offset zero of the element, and it then looks like a NULL VLEN data
+     */
+    strcpy(common_string, VLEN_DS_STRING);
+
+    for (i = 0; i < VLEN_DS_DIM; ++i) {
+        compound_data[i].value = VLEN_DS_VALUE;
+        for (dim1 = 0; dim1 < VLEN_DS_ARRAY_DIM1; ++dim1) {
+            for (dim2 = 0; dim2 < VLEN_DS_ARRAY_DIM2; ++dim2) {
+                compound_data[i].string[dim1][dim2] = common_string;
+            }
+        }
+        compound_data[i].padding1 = 0;
+        compound_data[i].padding2 = 0;
+        compound_data[i].padding3 = 0;
+    }
+
+    /* Starting with the maximum size, progressively over-write the content of the dataset with smaller
+     * arrays. */
+    /* Note: the bug in v1.8.14 is tripped on the second iteration, when 100 elements are over-written
+     * with 99. */
+    for (i = VLEN_DS_DIM; i > 0; --i) {
+        vlen_compound_data.len = (size_t)i;
+        vlen_compound_data.p   = compound_data;
+        if (H5Dwrite(scalar_did, vlen_compound_memory_tid, scalar_sid, scalar_sid, H5P_DEFAULT,
+                     &vlen_compound_data) < 0)
+            TEST_ERROR;
+    }
+
+    /* Close everything */
+    if (H5Sclose(scalar_sid) < 0)
+        TEST_ERROR;
+    if (H5Tclose(string_tid) < 0)
+        TEST_ERROR;
+    if (H5Tclose(string_array_tid) < 0)
+        TEST_ERROR;
+    if (H5Tclose(compound_file_tid) < 0)
+        TEST_ERROR;
+    if (H5Tclose(vlen_compound_file_tid) < 0)
+        TEST_ERROR;
+    if (H5Tclose(compound_memory_tid) < 0)
+        TEST_ERROR;
+    if (H5Tclose(vlen_compound_memory_tid) < 0)
+        TEST_ERROR;
+    if (H5Dclose(scalar_did) < 0)
+        TEST_ERROR;
+    free(compound_data);
+    compound_data = NULL;
+
+    PASSED();
+    return 0;
+
+error:
+    H5E_BEGIN_TRY
+    {
+        H5Sclose(scalar_sid);
+        H5Tclose(string_tid);
+        H5Tclose(string_array_tid);
+        H5Tclose(compound_file_tid);
+        H5Tclose(vlen_compound_file_tid);
+        H5Tclose(compound_memory_tid);
+        H5Tclose(vlen_compound_memory_tid);
+        H5Dclose(scalar_did);
+        free(compound_data);
+        compound_data = NULL;
+    }
+    H5E_END_TRY;
+    return -1;
+} /* end test_downsize_vlen_scalar_dataset() */
+
+/*-------------------------------------------------------------------------
  * Function:    main
  *
  * Purpose:     Tests the dataset interface (H5D)
@@ -15958,6 +16126,8 @@ main(void)
                 nerrors += (test_farray_hdr_fd(envval, my_fapl) < 0 ? 1 : 0);
                 nerrors += (test_bt2_hdr_fd(envval, my_fapl) < 0 ? 1 : 0);
 
+                nerrors += (test_downsize_vlen_scalar_dataset(file) < 0 ? 1 : 0);
+
                 if (H5Fclose(file) < 0)
                     goto error;
             } /* end for new_format */