Skip to content

Commit

Permalink
Fix problems with background buffers and array datatypes (HDFGroup#4218)
Browse files Browse the repository at this point in the history 
* Fix bug in array conversion with strided background buffer. Convert some
memmove calls to non-overlapping buffers to memcpy.

* Revert inappropriate use of mempy to memmove in H5T__conv_array

* Add testing

* Add RELEASE.txt note and overwrite test case.
  • Loading branch information
@fortnern
fortnern authored Apr 1, 2024
1 parent 23b78f6 commit 589f523
Show file tree
Hide file tree
Showing 3 changed files with 286 additions and 55 deletions.
8 changes: 8 additions & 0 deletions release_docs/RELEASE.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -746,6 +746,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
Expand Down
115 changes: 63 additions & 52 deletions src/H5Tconv.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 */
Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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;

Expand All @@ -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 */
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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 */
Expand Down Expand Up @@ -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 */
Expand All @@ -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 */
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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:
Expand All @@ -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;
Expand All @@ -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;
}

Expand All @@ -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");
Expand All @@ -4019,31 +4043,22 @@ 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++) {
/* Copy the source array into the correct location for the destination */
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;

Expand Down Expand Up @@ -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() */

Expand Down
Loading

0 comments on commit 589f523

Please sign in to comment.