diff --git a/release_docs/RELEASE.txt b/release_docs/RELEASE.txt index d96e2f2a6f5..2ff4bd8f017 100644 --- a/release_docs/RELEASE.txt +++ b/release_docs/RELEASE.txt @@ -693,6 +693,14 @@ Bug Fixes since HDF5-1.14.0 release Library ------- + - Fixed a bug when using array datatypes with certain parent types + + Array datatype conversion would never use a background buffer, even if the + array's parent type (what the array is an array of) required a background + buffer for conversion. This resulted in crashes in some cases when using + an array of compound, variable length, or reference datatypes. Array types + now use a background buffer if needed by the parent type. + - Fixed potential buffer read overflows in H5PB_read H5PB_read previously did not account for the fact that the size of the diff --git a/src/H5Tconv.c b/src/H5Tconv.c index 55b6d7d01da..fe207b0c89d 100644 --- a/src/H5Tconv.c +++ b/src/H5Tconv.c @@ -1186,6 +1186,11 @@ typedef struct H5T_conv_enum_t { int *src2dst; /*map from src to dst index */ } H5T_conv_enum_t; +/* Cnversion data for H5T__conv_array() */ +typedef struct H5T_conv_array_t { + H5T_path_t *tpath; /* Conversion path for parent types */ +} H5T_conv_array_t; + /* Conversion data for the hardware conversion functions */ typedef struct H5T_conv_hw_t { size_t s_aligned; /*number source elements aligned */ @@ -1221,9 +1226,6 @@ static herr_t H5T__reverse_order(uint8_t *rev, uint8_t *s, size_t size, H5T_orde /* Declare a free list to manage pieces of vlen data */ H5FL_BLK_DEFINE_STATIC(vlen_seq); -/* Declare a free list to manage pieces of array data */ -H5FL_BLK_DEFINE_STATIC(array_seq); - /* Declare a free list to manage pieces of reference data */ H5FL_BLK_DEFINE_STATIC(ref_seq); @@ -2521,7 +2523,7 @@ H5T__conv_struct(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H } /* end if */ else offset -= dst_memb->size; - memmove(xbkg + dst_memb->offset, xbuf + offset, dst_memb->size); + memcpy(xbkg + dst_memb->offset, xbuf + offset, dst_memb->size); } /* end for */ tmp_conv_ctx.u.conv.recursive = false; @@ -2543,7 +2545,7 @@ H5T__conv_struct(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H * buffer. */ for (xbuf = buf, xbkg = bkg, elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbuf, xbkg, dst->shared->size); + memcpy(xbuf, xbkg, dst->shared->size); xbuf += buf_stride ? buf_stride : dst->shared->size; xbkg += bkg_delta; } /* end for */ @@ -2743,7 +2745,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con copy_size = priv->subset_info.copy_size; for (elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbkg, xbuf, copy_size); + memcpy(xbkg, xbuf, copy_size); /* Update pointers */ xbuf += buf_stride; @@ -2781,7 +2783,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con "unable to convert compound datatype member"); for (elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbkg, xbuf, dst_memb->size); + memcpy(xbkg, xbuf, dst_memb->size); xbuf += buf_stride; xbkg += bkg_stride; } /* end for */ @@ -2826,7 +2828,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL, "unable to convert compound datatype member"); for (elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbkg, xbuf, dst_memb->size); + memcpy(xbkg, xbuf, dst_memb->size); xbuf += buf_stride; xbkg += bkg_stride; } /* end for */ @@ -2840,7 +2842,7 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con /* Move background buffer into result buffer */ for (xbuf = buf, xbkg = bkg, elmtno = 0; elmtno < nelmts; elmtno++) { - memmove(xbuf, xbkg, dst->shared->size); + memcpy(xbuf, xbkg, dst->shared->size); xbuf += buf_stride; xbkg += bkg_stride; } /* end for */ @@ -3094,7 +3096,7 @@ H5T__conv_enum_init(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, cons } FUNC_LEAVE_NOAPI(ret_value) -} +} /* end H5T__conv_enum_init() */ /*------------------------------------------------------------------------- * Function: H5T__conv_enum @@ -3904,20 +3906,19 @@ H5T__conv_vlen(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H5T herr_t H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, const H5T_conv_ctx_t H5_ATTR_UNUSED *conv_ctx, size_t nelmts, size_t buf_stride, - size_t bkg_stride, void *_buf, void H5_ATTR_UNUSED *_bkg) -{ - H5T_conv_ctx_t tmp_conv_ctx = {0}; /* Temporary conversion context */ - H5T_path_t *tpath; /* Type conversion path */ - H5T_t *tsrc_cpy = NULL; /*temporary copy of source base datatype */ - H5T_t *tdst_cpy = NULL; /*temporary copy of destination base datatype */ - hid_t tsrc_id = H5I_INVALID_HID; /*temporary type atom */ - hid_t tdst_id = H5I_INVALID_HID; /*temporary type atom */ - uint8_t *sp, *dp; /*source and dest traversal ptrs */ - ssize_t src_delta, dst_delta; /*source & destination stride */ - int direction; /*direction of traversal */ - bool need_ids = false; /*whether we need IDs for the datatypes */ - void *bkg_buf = NULL; /*temporary background buffer */ - herr_t ret_value = SUCCEED; /* Return value */ + size_t bkg_stride, void *_buf, void *_bkg) +{ + H5T_conv_array_t *priv = NULL; /* Private conversion data */ + H5T_conv_ctx_t tmp_conv_ctx = {0}; /* Temporary conversion context */ + H5T_t *tsrc_cpy = NULL; /* Temporary copy of source base datatype */ + H5T_t *tdst_cpy = NULL; /* Temporary copy of destination base datatype */ + hid_t tsrc_id = H5I_INVALID_HID; /* Temporary type atom */ + hid_t tdst_id = H5I_INVALID_HID; /* Temporary type atom */ + uint8_t *sp, *dp, *bp; /* Source, dest, and bkg traversal ptrs */ + ssize_t src_delta, dst_delta, bkg_delta; /* Source, dest, and bkg strides */ + int direction; /* Direction of traversal */ + bool need_ids = false; /* Whether we need IDs for the datatypes */ + herr_t ret_value = SUCCEED; /* Return value */ FUNC_ENTER_PACKAGE @@ -3944,13 +3945,35 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, "array datatypes do not have the same sizes of dimensions"); - /* Array datatypes don't need a background buffer */ - cdata->need_bkg = H5T_BKG_NO; + /* Initialize parent type conversion if necessary. We need to do this here because we need to + * report whether we need a background buffer or not. */ + if (!cdata->priv) { + /* Allocate private data */ + if (NULL == (priv = (H5T_conv_array_t *)(cdata->priv = calloc(1, sizeof(*priv))))) + HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed"); + + /* Find conversion path between parent types */ + if (NULL == (priv->tpath = H5T_path_find(src->shared->parent, dst->shared->parent))) { + free(priv); + cdata->priv = NULL; + HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, + "unable to convert between src and dest datatype"); + } + + /* Array datatypes don't need a background buffer by themselves, but the parent type might. + * Pass the need_bkg field through to the upper layer. */ + cdata->need_bkg = priv->tpath->cdata.need_bkg; + } break; case H5T_CONV_FREE: - /* QAK - Nothing to do currently */ + /* + * Free private data + */ + free(cdata->priv); + cdata->priv = NULL; + break; case H5T_CONV_CONV: @@ -3961,6 +3984,7 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "not a datatype"); if (NULL == conv_ctx) HGOTO_ERROR(H5E_DATATYPE, H5E_BADVALUE, FAIL, "invalid datatype conversion context pointer"); + priv = (H5T_conv_array_t *)cdata->priv; /* Initialize temporary conversion context */ tmp_conv_ctx = *conv_ctx; @@ -3974,11 +3998,14 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, */ if (src->shared->size >= dst->shared->size || buf_stride > 0) { sp = dp = (uint8_t *)_buf; + bp = _bkg; direction = 1; } else { - sp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : src->shared->size); - dp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : dst->shared->size); + sp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : src->shared->size); + dp = (uint8_t *)_buf + (nelmts - 1) * (buf_stride ? buf_stride : dst->shared->size); + bp = _bkg ? (uint8_t *)_bkg + (nelmts - 1) * (bkg_stride ? bkg_stride : dst->shared->size) + : NULL; direction = -1; } @@ -3990,13 +4017,10 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, H5_CHECK_OVERFLOW(dst->shared->size, size_t, ssize_t); src_delta = (ssize_t)direction * (ssize_t)(buf_stride ? buf_stride : src->shared->size); dst_delta = (ssize_t)direction * (ssize_t)(buf_stride ? buf_stride : dst->shared->size); + bkg_delta = (ssize_t)direction * (ssize_t)(bkg_stride ? bkg_stride : dst->shared->size); /* Set up conversion path for base elements */ - if (NULL == (tpath = H5T_path_find(src->shared->parent, dst->shared->parent))) { - HGOTO_ERROR(H5E_DATATYPE, H5E_UNSUPPORTED, FAIL, - "unable to convert between src and dest datatypes"); - } - else if (!H5T_path_noop(tpath)) { + if (!H5T_path_noop(priv->tpath)) { if (NULL == (tsrc_cpy = H5T_copy(src->shared->parent, H5T_COPY_ALL))) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCOPY, FAIL, "unable to copy src base type for conversion"); @@ -4019,17 +4043,6 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, tmp_conv_ctx.u.conv.dst_type_id = tdst_id; } - /* Check if we need a background buffer for this conversion */ - if (tpath->cdata.need_bkg) { - size_t bkg_buf_size; /*size of background buffer in bytes */ - - /* Allocate background buffer */ - bkg_buf_size = src->shared->u.array.nelem * MAX(src->shared->size, dst->shared->size); - if (NULL == (bkg_buf = H5FL_BLK_CALLOC(array_seq, bkg_buf_size))) - HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, - "memory allocation failed for type conversion"); - } /* end if */ - /* Perform the actual conversion */ tmp_conv_ctx.u.conv.recursive = true; for (size_t elmtno = 0; elmtno < nelmts; elmtno++) { @@ -4037,13 +4050,15 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, memmove(dp, sp, src->shared->size); /* Convert array */ - if (H5T_convert_with_ctx(tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, src->shared->u.array.nelem, - (size_t)0, bkg_stride, dp, bkg_buf) < 0) + if (H5T_convert_with_ctx(priv->tpath, tsrc_cpy, tdst_cpy, &tmp_conv_ctx, + src->shared->u.array.nelem, (size_t)0, (size_t)0, dp, bp) < 0) HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL, "datatype conversion failed"); - /* Advance the source & destination pointers */ + /* Advance the source, destination, and background pointers */ sp += src_delta; dp += dst_delta; + if (bp) + bp += bkg_delta; } /* end for */ tmp_conv_ctx.u.conv.recursive = false; @@ -4071,10 +4086,6 @@ H5T__conv_array(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, HDONE_ERROR(H5E_DATATYPE, H5E_CANTCLOSEOBJ, FAIL, "can't close temporary datatype"); } - /* Release the background buffer, if we have one */ - if (bkg_buf) - bkg_buf = H5FL_BLK_FREE(array_seq, bkg_buf); - FUNC_LEAVE_NOAPI(ret_value) } /* end H5T__conv_array() */ diff --git a/test/dtypes.c b/test/dtypes.c index 836d6d89eb0..ea589583e65 100644 --- a/test/dtypes.c +++ b/test/dtypes.c @@ -73,9 +73,9 @@ } \ } while (0) -static const char *FILENAME[] = {"dtypes0", "dtypes1", "dtypes2", "dtypes3", "dtypes4", - "dtypes5", "dtypes6", "dtypes7", "dtypes8", "dtypes9", - "dtypes10", "dtypes11", NULL}; +static const char *FILENAME[] = {"dtypes0", "dtypes1", "dtypes2", "dtypes3", "dtypes4", + "dtypes5", "dtypes6", "dtypes7", "dtypes8", "dtypes9", + "dtypes10", "dtypes11", "dtypes12", NULL}; #define TESTFILE "bad_compound.h5" @@ -6464,6 +6464,217 @@ test__Float16(void) #endif } +/*------------------------------------------------------------------------- + * Function: test_array_cmpd_vl + * + * Purpose: Tests that conversion occurs correctly with an array of + * arrays of compounds containing a variable length sequence. + * + * Return: Success: 0 + * + * Failure: number of errors + * + *------------------------------------------------------------------------- + */ +static int +test_array_cmpd_vl(void) +{ + typedef struct cmpd_struct { + hvl_t vl; + } cmpd_struct; + + int int_wdata[2][3][2] = {{{0, 1}, {2, 3}, {4, 5}}, {{6, 7}, {8, 9}, {10, 11}}}; + cmpd_struct wdata[2][3]; + cmpd_struct rdata[2][3]; + hid_t file; + hid_t vl_tid, cmpd_tid, inner_array_tid, outer_array_tid; + hid_t space_id; + hid_t dset_id; + hsize_t dim1[1]; + char filename[1024]; + + TESTING("array of arrays of compounds with a vlen"); + + /* Create File */ + h5_fixname(FILENAME[12], 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 VL of ints datatype */ + if ((vl_tid = H5Tvlen_create(H5T_NATIVE_INT)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create datatype!\n"); + goto error; + } /* end if */ + + /* Create compound datatype */ + if ((cmpd_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_tid, "vl", HOFFSET(struct cmpd_struct, vl), vl_tid) < 0) { + H5_FAILED(); + AT(); + printf("Can't insert field 'vl'\n"); + goto error; + } /* end if */ + + /* Create inner array type */ + dim1[0] = 3; + if ((inner_array_tid = H5Tarray_create2(cmpd_tid, 1, dim1)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create datatype!\n"); + goto error; + } /* end if */ + + /* Create outer array type */ + dim1[0] = 2; + if ((outer_array_tid = H5Tarray_create2(inner_array_tid, 1, dim1)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create datatype!\n"); + goto error; + } /* end if */ + + /* Create space, dataset */ + 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", outer_array_tid, space_id, H5P_DEFAULT, H5P_DEFAULT, + H5P_DEFAULT)) < 0) { + H5_FAILED(); + AT(); + printf("Can't create dataset\n"); + goto error; + } /* end if */ + + /* Initialize wdata */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) { + wdata[i][j].vl.len = 2; + wdata[i][j].vl.p = int_wdata[i][j]; + } + + /* Write data */ + if (H5Dwrite(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, wdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't write data\n"); + goto error; + } /* end if */ + + /* Initialize rdata */ + (void)memset(rdata, 0, sizeof(rdata)); + + /* Read data */ + if (H5Dread(dset_id, outer_array_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 */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) + if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || + ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { + H5_FAILED(); + AT(); + printf("incorrect read data at [%d][%d]\n", i, j); + goto error; + } + + /* Reclaim memory */ + if (H5Treclaim(outer_array_tid, space_id, H5P_DEFAULT, rdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't reclaim memory\n"); + goto error; + } /* end if */ + + /* Adjust write buffer */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) { + int_wdata[i][j][0] += 100; + int_wdata[i][j][1] += 100; + } + + /* Overwrite dataset with adjusted wdata */ + if (H5Dwrite(dset_id, outer_array_tid, H5S_ALL, H5S_ALL, H5P_DEFAULT, wdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't write data\n"); + goto error; + } /* end if */ + + /* Initialize rdata */ + (void)memset(rdata, 0, sizeof(rdata)); + + /* Read data */ + if (H5Dread(dset_id, outer_array_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 */ + for (int i = 0; i < 2; i++) + for (int j = 0; j < 3; j++) + if (rdata[i][j].vl.len != 2 || ((int *)rdata[i][j].vl.p)[0] != int_wdata[i][j][0] || + ((int *)rdata[i][j].vl.p)[1] != int_wdata[i][j][1]) { + H5_FAILED(); + AT(); + printf("incorrect read data at [%d][%d]\n", i, j); + goto error; + } + + /* Reclaim memory */ + if (H5Treclaim(outer_array_tid, space_id, H5P_DEFAULT, rdata) < 0) { + H5_FAILED(); + AT(); + printf("Can't reclaim memory\n"); + goto error; + } /* end if */ + + /* Close */ + if (H5Dclose(dset_id) < 0) + goto error; + if (H5Tclose(outer_array_tid) < 0) + goto error; + if (H5Tclose(inner_array_tid) < 0) + goto error; + if (H5Tclose(cmpd_tid) < 0) + goto error; + if (H5Tclose(vl_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_array_cmpd_vl() */ + /*------------------------------------------------------------------------- * Function: test_encode * @@ -9700,6 +9911,7 @@ main(void) nerrors += test_bitfield_funcs(); nerrors += test_opaque(); nerrors += test_set_order(); + nerrors += test_array_cmpd_vl(); nerrors += test__Float16();