From cf451beffa0cc2df12b23bf6f5f37a702a88fb21 Mon Sep 17 00:00:00 2001 From: Neil Fortner Date: Mon, 22 Sep 2008 18:41:58 -0500 Subject: [svn-r15683] 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) --- release_docs/RELEASE.txt | 2 + src/H5Dio.c | 2 +- src/H5Dpkg.h | 2 +- src/H5Dscatgath.c | 20 +++--- src/H5T.c | 12 ++-- src/H5Tconv.c | 56 +++++++++------- src/H5Tpkg.h | 4 +- src/H5Tprivate.h | 7 +- test/dtypes.c | 170 +++++++++++++++++++++++++++++++++++++++++++++-- 9 files changed, 228 insertions(+), 47 deletions(-) diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index 70c75c5..b0c6f61 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -104,6 +104,8 @@ Bug Fixes since HDF5-1.8.1 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