Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix problems with background buffers and array datatypes #4218

Merged
merged 12 commits into from
Apr 1, 2024
Merged

Fix problems with background buffers and array datatypes #4218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
801b62c
Fix issues with background buffers with array datatypes
fortnern Mar 22, 2024
a6a7750
Committing clang-format changes
github-actions[bot] Mar 22, 2024
8483433
Spelling
fortnern Mar 22, 2024
ae0f2f2
Fix bug in array conversion with strided background buffer. Convert some
fortnern Mar 29, 2024
78b3f4b
Committing clang-format changes
github-actions[bot] Mar 29, 2024
06da757
Revert inappropriate use of mempy to memmove in H5T__conv_array
fortnern Mar 29, 2024
aba6fef
Add testing
fortnern Mar 29, 2024
7559255
Committing clang-format changes
github-actions[bot] Mar 29, 2024
585861e
Add RELEASE.txt note and overwrite test case.
fortnern Mar 29, 2024
56ccef7
Merge branch 'array_bkg_fix' of github.com:fortnern/hdf5 into array_b…
fortnern Mar 29, 2024
fd257de
Committing clang-format changes
github-actions[bot] Mar 29, 2024
5a56602
Merge branch 'develop' into array_bkg_fix
fortnern Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
115 changes: 63 additions & 52 deletions src/H5Tconv.c
View file
Open in desktop
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
Loading