[svn-r15682] Purpose: fix bug 1298

Description: The optimized "subset" compound conversion function would
improperly copy the "blank" space at the end of compound types.  Modified
H5T_conv_struct_init to detect when the subset type has extra space at the end,
and calculate the size of the data that should be copied into the conversion
buffer for each element.  Changes to the functions that implement these
conversions.

Tested: kagiso, linew, smirom (h5committest)

9 files changed, 228 insertions, 47 deletions

diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt
index 4d012c0..f52dd75 100644
--- a/release_docs/RELEASE.txt
+++ b/release_docs/RELEASE.txt
@@ -109,6 +109,8 @@ Bug Fixes since HDF5-1.8.0 release
 
     Library
     -------
+        - Fixed an issue that could cause data to be improperly overwritten
+            during compound type conversion.  (NAF - 2008/09/19)
         - Fixed pointer alignment violations that could occur during vlen
             conversion.  (NAF - 2008/09/16)
         - Fixed problem where library could cause a segmentation fault when
diff --git a/src/H5Dio.c b/src/H5Dio.c
index 45fa21a..b7c1d72 100644
--- a/src/H5Dio.c
+++ b/src/H5Dio.c
@@ -745,7 +745,7 @@ H5D_typeinfo_init(const H5D_t *dset, const H5D_dxpl_cache_t *dxpl_cache,
     type_info->is_conv_noop = H5T_path_noop(type_info->tpath);
     type_info->is_xform_noop = H5Z_xform_noop(dxpl_cache->data_xform_prop);
     if(type_info->is_xform_noop && type_info->is_conv_noop) {
-        type_info->cmpd_subset = H5T_SUBSET_FALSE;
+        type_info->cmpd_subset = NULL;
         type_info->need_bkg = H5T_BKG_NO;
     } /* end if */
     else {
diff --git a/src/H5Dpkg.h b/src/H5Dpkg.h
index 9f02dbc..f6732ff 100644
--- a/src/H5Dpkg.h
+++ b/src/H5Dpkg.h
@@ -86,7 +86,7 @@ typedef struct H5D_type_info_t {
     size_t max_type_size;	        /* Size of largest source/destination type */
     hbool_t is_conv_noop;               /* Whether the type conversion is a NOOP */
     hbool_t is_xform_noop;              /* Whether the data transform is a NOOP */
-    H5T_subset_t cmpd_subset;           /* Whether (and which) the source/destination datatypes are compound subsets of one another */
+    const H5T_subset_info_t *cmpd_subset;   /* Info related to the compound subset conversion functions */
     H5T_bkg_t need_bkg;		        /* Type of background buf needed */
     size_t request_nelmts;		/* Requested strip mine	*/
     uint8_t *tconv_buf;	                /* Datatype conv buffer	*/
diff --git a/src/H5Dscatgath.c b/src/H5Dscatgath.c
index cb07708..867303f 100644
--- a/src/H5Dscatgath.c
+++ b/src/H5Dscatgath.c
@@ -519,7 +519,7 @@ H5D_scatgath_read(const H5D_io_info_t *io_info, const H5D_type_info_t *type_info
          * and no conversion is needed, copy the data directly into user's buffer and
          * bypass the rest of steps.
          */
-        if(H5T_SUBSET_FALSE != type_info->cmpd_subset) {
+        if(type_info->cmpd_subset && H5T_SUBSET_FALSE != type_info->cmpd_subset->subset) {
             if(H5D_compound_opt_read(smine_nelmts, mem_space, &mem_iter, dxpl_cache,
                     type_info, buf /*out*/) < 0)
                 HGOTO_ERROR(H5E_DATASET, H5E_CANTINIT, FAIL, "datatype conversion failed")
@@ -649,7 +649,8 @@ H5D_scatgath_write(const H5D_io_info_t *io_info, const H5D_type_info_t *type_inf
          * is a subset of the destination, the optimization is done in conversion
          * function H5T_conv_struct_opt to protect the background data.
          */
-        if(H5T_SUBSET_DST == type_info->cmpd_subset) {
+        if(type_info->cmpd_subset && H5T_SUBSET_DST == type_info->cmpd_subset->subset
+            && type_info->dst_type_size == type_info->cmpd_subset->copy_size) {
             if(H5D_compound_opt_write(smine_nelmts, type_info) < 0)
                 HGOTO_ERROR(H5E_DATASET, H5E_CANTINIT, FAIL, "datatype conversion failed")
         } /* end if */
@@ -745,7 +746,7 @@ H5D_compound_opt_read(size_t nelmts, const H5S_t *space,
     hsize_t    *off = NULL;                     /* Pointer to sequence offsets */
     size_t     _len[H5D_IO_VECTOR_SIZE];        /* Array to store sequence lengths */
     size_t     *len = NULL;                     /* Pointer to sequence lengths */
-    size_t     src_stride, dst_stride, type_size;
+    size_t     src_stride, dst_stride, copy_size;
     herr_t     ret_value = SUCCEED;	       /*return value		*/
 
     FUNC_ENTER_NOAPI_NOINIT(H5D_compound_opt_read)
@@ -756,6 +757,9 @@ H5D_compound_opt_read(size_t nelmts, const H5S_t *space,
     HDassert(iter);
     HDassert(dxpl_cache);
     HDassert(type_info);
+    HDassert(type_info->cmpd_subset);
+    HDassert(H5T_SUBSET_SRC == type_info->cmpd_subset->subset ||
+        H5T_SUBSET_DST == type_info->cmpd_subset->subset);
     HDassert(user_buf);
 
     /* Allocate the vector I/O arrays */
@@ -774,12 +778,8 @@ H5D_compound_opt_read(size_t nelmts, const H5S_t *space,
     src_stride = type_info->src_type_size;
     dst_stride = type_info->dst_type_size;
 
-    if(H5T_SUBSET_SRC == type_info->cmpd_subset)
-        type_size = src_stride;
-    else {
-        HDassert(H5T_SUBSET_DST == type_info->cmpd_subset);
-        type_size = dst_stride;
-    } /* end else */
+    /* Get the size, in bytes, to copy for each element */
+    copy_size = type_info->cmpd_subset->copy_size;
 
     /* Loop until all elements are written */
     xdbuf = type_info->tconv_buf;
@@ -811,7 +811,7 @@ H5D_compound_opt_read(size_t nelmts, const H5S_t *space,
 
             /* Copy the data into the right place. */
             for(i = 0; i < curr_nelmts; i++) {
-                HDmemmove(xubuf, xdbuf, type_size);
+                HDmemmove(xubuf, xdbuf, copy_size);
 
                 /* Update pointers */
                 xdbuf += src_stride;
diff --git a/src/H5T.c b/src/H5T.c
index 7fd19fa..293a5c5 100644
--- a/src/H5T.c
+++ b/src/H5T.c
@@ -4526,19 +4526,23 @@ H5T_path_noop(const H5T_path_t *p)
  *                                                 TYPE5 E;
  *                                             };
  *
- * Return:	One of the values of H5T_subset_t (can't fail).
+ * Return:	A pointer to the subset info struct in p, or NULL if there are
+ *          no compounds.  Points directly into the H5T_path_t structure.
  *
  * Programmer:	Raymond Lu
  *		8 June 2007
  *
- * Modifications:
+ * Modifications:  Neil Fortner
+ *      19 September 2008
+ *      Changed return value to H5T_subset_info_t
+ *      (to allow it to return copy_size)
  *
  *-------------------------------------------------------------------------
  */
-H5T_subset_t
+H5T_subset_info_t *
 H5T_path_compound_subset(const H5T_path_t *p)
 {
-    H5T_subset_t ret_value = FALSE;
+    H5T_subset_info_t *ret_value = NULL;
 
     FUNC_ENTER_NOAPI_NOINIT_NOFUNC(H5T_path_compound_subset);
 
diff --git a/src/H5Tconv.c b/src/H5Tconv.c
index 276d05a..b02e72a 100644
--- a/src/H5Tconv.c
+++ b/src/H5Tconv.c
@@ -883,7 +883,7 @@ typedef struct H5T_conv_struct_t {
     hid_t	*src_memb_id;		/*source member type ID's	     */
     hid_t	*dst_memb_id;		/*destination member type ID's	     */
     H5T_path_t	**memb_path;		/*conversion path for each member    */
-    H5T_subset_t     smembs_subset;     /*are source and dest members a subset of each other? */
+    H5T_subset_info_t   subset_info;    /*info related to compound subsets   */
 } H5T_conv_struct_t;
 
 /* Conversion data for H5T_conv_enum() */
@@ -1827,7 +1827,8 @@ H5T_conv_struct_init(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata, hid_t dxpl_id)
 
         /* The flag of special optimization to indicate if source members and destination
          * members are a subset of each other.  Initialize it to FALSE */
-        priv->smembs_subset = H5T_SUBSET_FALSE;
+        priv->subset_info.subset = H5T_SUBSET_FALSE;
+        priv->subset_info.copy_size = 0;
 
         /*
          * Insure that members are sorted.
@@ -1899,23 +1900,41 @@ H5T_conv_struct_init(H5T_t *src, H5T_t *dst, H5T_cdata_t *cdata, hid_t dxpl_id)
         cdata->need_bkg = H5T_BKG_YES;
 
         if(src_nmembs < dst_nmembs) {
-            priv->smembs_subset = H5T_SUBSET_SRC;
+            priv->subset_info.subset = H5T_SUBSET_SRC;
             for(i = 0; i < src_nmembs; i++) {
                 /* If any of source members doesn't have counterpart in the same
                  * order or there's conversion between members, don't do the
                  * optimization.
                  */
-                if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE)
-                    priv->smembs_subset = H5T_SUBSET_FALSE;
+                if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE) {
+                    priv->subset_info.subset = H5T_SUBSET_FALSE;
+                    break;
+                } /* end if */
             } /* end for */
+            /* Compute the size of the data to be copied for each element.  It
+             * may be smaller than either src or dst if there is extra space at
+             * the end of src.
+             */
+            if(priv->subset_info.subset == H5T_SUBSET_SRC)
+                priv->subset_info.copy_size = src->shared->u.compnd.memb[src_nmembs-1].offset
+                    + src->shared->u.compnd.memb[src_nmembs-1].size;
         } else if(dst_nmembs < src_nmembs) {
-            priv->smembs_subset = H5T_SUBSET_DST;
+            priv->subset_info.subset = H5T_SUBSET_DST;
             for(i = 0; i < dst_nmembs; i++) {
                 /* If any of source members doesn't have counterpart in the same order or
                  * there's conversion between members, don't do the optimization. */
-                if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE)
-                    priv->smembs_subset = H5T_SUBSET_FALSE;
+                if(src2dst[i] != i || (src->shared->u.compnd.memb[i].offset != dst->shared->u.compnd.memb[i].offset) || (priv->memb_path[i])->is_noop == FALSE) {
+                    priv->subset_info.subset = H5T_SUBSET_FALSE;
+                    break;
+                }
             } /* end for */
+            /* Compute the size of the data to be copied for each element.  It
+             * may be smaller than either src or dst if there is extra space at
+             * the end of dst.
+             */
+            if(priv->subset_info.subset == H5T_SUBSET_DST)
+                priv->subset_info.copy_size = dst->shared->u.compnd.memb[dst_nmembs-1].offset
+                    + dst->shared->u.compnd.memb[dst_nmembs-1].size;
         } else /* If the numbers of source and dest members are equal and no conversion is needed,
                 * the case should have been handled as noop earlier in H5Dio.c. */
           ;
@@ -1945,14 +1964,15 @@ done:
  *                                                 TYPE5 E;
  *                                             };
  *
- * Return:      One of the value from H5T_subset_t.
+ * Return:      A pointer to the subset info struct in p.  Points directly
+ *              into the structure.
  *
  * Programmer:	Raymond Lu
  *		8 June 2007
  *
  *-------------------------------------------------------------------------
  */
-H5T_subset_t
+H5T_subset_info_t *
 H5T_conv_struct_subset(const H5T_cdata_t *cdata)
 {
     H5T_conv_struct_t	*priv;
@@ -1964,7 +1984,7 @@ H5T_conv_struct_subset(const H5T_cdata_t *cdata)
 
     priv = (H5T_conv_struct_t *)(cdata->priv);
 
-    FUNC_LEAVE_NOAPI(priv->smembs_subset)
+    FUNC_LEAVE_NOAPI((H5T_subset_info_t *) &priv->subset_info)
 } /* end H5T_conv_struct_subset() */
 
 
@@ -2388,22 +2408,12 @@ H5T_conv_struct_opt(hid_t src_id, hid_t dst_id, H5T_cdata_t *cdata,
                 buf_stride = src->shared->size;
             }
 
-            if(priv->smembs_subset == H5T_SUBSET_SRC || priv->smembs_subset == H5T_SUBSET_DST) {
+            if(priv->subset_info.subset == H5T_SUBSET_SRC || priv->subset_info.subset == H5T_SUBSET_DST) {
                 /* If the optimization flag is set to indicate source members are a subset and
                  * in the top of the destination, simply copy the source members to background buffer. */
                 xbuf = buf;
                 xbkg = bkg;
-                if(dst->shared->size <= src->shared->size)
-                    /* This is to deal with a very special situation when the fields and their
-                     * offset for both source and destination are identical but the datatype
-                     * sizes of source and destination are different.  The library still
-                     * considers these two types different and does conversion.  It happens
-                     * in table API test (hdf5/hl/test/test_table.c) when a table field is
-                     * deleted.
-                     */
-                    copy_size = dst->shared->size;
-                else
-                    copy_size = src->shared->size;
+                copy_size = priv->subset_info.copy_size;
 
                 for (elmtno=0; elmtno<nelmts; elmtno++) {
                     HDmemmove(xbkg, xbuf, copy_size);
diff --git a/src/H5Tpkg.h b/src/H5Tpkg.h
index a534f1b..7ef4b63 100644
--- a/src/H5Tpkg.h
+++ b/src/H5Tpkg.h
@@ -288,7 +288,7 @@ typedef enum H5T_sort_t {
 typedef struct H5T_cmemb_t {
     char		*name;		/*name of this member		     */
     size_t		offset;		/*offset from beginning of struct    */
-    size_t		size;		/*total size: dims * type_size	     */
+    size_t		size;		/*size of this member		     */
     struct H5T_t	*type;		/*type of this member		     */
 } H5T_cmemb_t;
 
@@ -1380,7 +1380,7 @@ H5_DLL herr_t H5T_insert(H5T_t *parent, const char *name, size_t offset,
         const H5T_t *member);
 H5_DLL size_t H5T_get_member_size(const H5T_t *dt, unsigned membno);
 H5_DLL htri_t H5T_is_packed(const H5T_t *dt);
-H5_DLL H5T_subset_t H5T_conv_struct_subset(const H5T_cdata_t *cdata);
+H5_DLL H5T_subset_info_t *H5T_conv_struct_subset(const H5T_cdata_t *cdata);
 
 /* Enumerated type functions */
 H5_DLL H5T_t *H5T_enum_create(const H5T_t *parent);
diff --git a/src/H5Tprivate.h b/src/H5Tprivate.h
index f8f2abc..9189f04 100644
--- a/src/H5Tprivate.h
+++ b/src/H5Tprivate.h
@@ -92,6 +92,11 @@ typedef enum {
     H5T_SUBSET_CAP              /* Must be the last value */
 } H5T_subset_t;
 
+typedef struct H5T_subset_info_t {
+    H5T_subset_t    subset;     /* See above */
+    size_t          copy_size;  /* Size in bytes, to copy for each element */
+} H5T_subset_info_t;
+
 /* Forward declarations for prototype arguments */
 struct H5O_t;
 
@@ -119,7 +124,7 @@ H5_DLL H5T_path_t *H5T_path_find(const H5T_t *src, const H5T_t *dst,
     const char *name, H5T_conv_t func, hid_t dxpl_id, hbool_t is_api);
 H5_DLL hbool_t H5T_path_noop(const H5T_path_t *p);
 H5_DLL H5T_bkg_t H5T_path_bkg(const H5T_path_t *p);
-H5_DLL H5T_subset_t H5T_path_compound_subset(const H5T_path_t *p);
+H5_DLL H5T_subset_info_t *H5T_path_compound_subset(const H5T_path_t *p);
 H5_DLL herr_t H5T_convert(H5T_path_t *tpath, hid_t src_id, hid_t dst_id,
     size_t nelmts, size_t buf_stride, size_t bkg_stride, void *buf, void *bkg,
     hid_t dset_xfer_plist);
diff --git a/test/dtypes.c b/test/dtypes.c
index f688371..674c42f 100644
--- a/test/dtypes.c
+++ b/test/dtypes.c
@@ -2683,10 +2683,10 @@ test_compound_14(void)
     } /* end if */
 
     rdata1.c1 = rdata1.c2 = 0;
-    if(rdata1.str) free(rdata1.str);
+    if(rdata1.str) HDfree(rdata1.str);
 
     rdata2.c1 = rdata2.c2 = rdata2.l1 = rdata2.l2 = rdata2.l3 = rdata2.l4 = 0;
-    if(rdata2.str) free(rdata2.str);
+    if(rdata2.str) HDfree(rdata2.str);
 
     if(H5Dread(dset1_id, cmpd_m1_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, &rdata1) < 0) {
         H5_FAILED(); AT();
@@ -2713,8 +2713,8 @@ test_compound_14(void)
         goto error;
     } /* end if */
 
-    if(rdata1.str) free(rdata1.str);
-    if(rdata2.str) free(rdata2.str);
+    if(rdata1.str) HDfree(rdata1.str);
+    if(rdata2.str) HDfree(rdata2.str);
 
     if(H5Dclose(dset1_id) < 0)
         goto error;
@@ -2732,7 +2732,166 @@ test_compound_14(void)
 
  error:
     return 1;
-}
+} /* end test_compound_14() */
+
+
+/*-------------------------------------------------------------------------
+ * Function:    test_compound_15
+ *
+ * Purpose:     Tests that conversion occurs correctly when the source is
+ *              subset of the destination, but there is extra space at the
+ *              end of the source type.
+ *
+ * Return:      Success:        0
+ *
+ *              Failure:        number of errors
+ *
+ * Programmer:  Neil Fortner
+ *              Friday, September 19, 2008
+ *
+ * Modifications:
+ *
+ *-------------------------------------------------------------------------
+ */
+static int
+test_compound_15(void)
+{
+    typedef struct cmpd_struct {
+       int  i1;
+       int  i2;
+    } cmpd_struct;
+
+    cmpd_struct wdata1 = {1254, 5471};
+    cmpd_struct rdata;
+    int         wdata2[2] = {1, 2};
+    hid_t       file;
+    hid_t       cmpd_m_tid, cmpd_f_tid;
+    hid_t       space_id;
+    hid_t       dset_id;
+    hsize_t     dim1[1];
+    char        filename[1024];
+
+    TESTING("compound subset conversion with extra space in source");
+
+    /* Create File */
+    h5_fixname(FILENAME[3], H5P_DEFAULT, filename, sizeof filename);
+    if((file=H5Fcreate(filename, H5F_ACC_TRUNC, H5P_DEFAULT, H5P_DEFAULT)) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't create file!\n");
+        goto error;
+    } /* end if */
+
+    /* Create file compound datatype */
+    if((cmpd_f_tid = H5Tcreate( H5T_COMPOUND, sizeof(struct cmpd_struct))) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't create datatype!\n");
+        goto error;
+    } /* end if */
+
+    if(H5Tinsert(cmpd_f_tid,"i1",HOFFSET(struct cmpd_struct,i1),H5T_NATIVE_INT) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't insert field 'i1'\n");
+        goto error;
+    } /* end if */
+
+    if(H5Tinsert(cmpd_f_tid,"i2",HOFFSET(struct cmpd_struct,i2),H5T_NATIVE_INT) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't insert field 'i2'\n");
+        goto error;
+    } /* end if */
+
+    /* Create memory compound datatype */
+    if((cmpd_m_tid = H5Tcreate( H5T_COMPOUND, sizeof(struct cmpd_struct))) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't create datatype!\n");
+        goto error;
+    } /* end if */
+
+    if(H5Tinsert(cmpd_m_tid,"i1",0,H5T_NATIVE_INT) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't insert field 'i1'\n");
+        goto error;
+    } /* end if */
+
+    /* Create space, dataset, write wdata1 */
+    dim1[0] = 1;
+    if((space_id = H5Screate_simple(1, dim1, NULL)) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't create space\n");
+        goto error;
+    } /* end if */
+
+    if((dset_id = H5Dcreate2(file, "Dataset", cmpd_f_tid, space_id, H5P_DEFAULT, H5P_DEFAULT, H5P_DEFAULT)) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't create dataset\n");
+        goto error;
+    } /* end if */
+
+    if(H5Dwrite(dset_id, cmpd_f_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, &wdata1) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't write data\n");
+        goto error;
+    } /* end if */
+
+    /* Write wdata2.  The use of cmpd_m_tid here should cause only the first
+     * element of wdata2 to be written. */
+    if(H5Dwrite(dset_id, cmpd_m_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, &wdata2) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't write data\n");
+        goto error;
+    } /* end if */
+
+    /* Read data */
+    if(H5Dread(dset_id, cmpd_f_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, &rdata) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't read data\n");
+        goto error;
+    } /* end if */
+
+    /* Check for correctness of read data */
+    if(rdata.i1 != wdata2[0] || rdata.i2 != wdata1.i2) {
+        H5_FAILED(); AT();
+        printf("incorrect read data\n");
+        goto error;
+    } /* end if */
+
+    /* Now try reading only the i1 field, verify it does not overwrite i2 in the
+     * read buffer */
+    rdata.i1 = wdata1.i1;
+    rdata.i2 = wdata2[1];
+
+    /* Read data */
+    if(H5Dread(dset_id, cmpd_m_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, &rdata) < 0) {
+        H5_FAILED(); AT();
+        printf("Can't read data\n");
+        goto error;
+    } /* end if */
+
+    /* Check for correctness of read data */
+    if(rdata.i1 != wdata2[0] || rdata.i2 != wdata2[1]) {
+        H5_FAILED(); AT();
+        printf("incorrect read data\n");
+        goto error;
+    } /* end if */
+
+    /* Close */
+    if(H5Dclose(dset_id) < 0)
+        goto error;
+    if(H5Tclose(cmpd_f_tid) < 0)
+        goto error;
+    if(H5Tclose(cmpd_m_tid) < 0)
+        goto error;
+    if(H5Sclose(space_id) < 0)
+        goto error;
+    if(H5Fclose(file) < 0)
+        goto error;
+
+    PASSED();
+    return 0;
+
+ error:
+    return 1;
+} /* end test_compound_14() */
 
 
 /*-------------------------------------------------------------------------
@@ -5427,6 +5586,7 @@ main(void)
     nerrors += test_compound_12();
     nerrors += test_compound_13();
     nerrors += test_compound_14();
+    nerrors += test_compound_15();
     nerrors += test_conv_enum_1();
     nerrors += test_conv_enum_2();
     nerrors += test_conv_bitfield();