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

Fix two assertion failures in hyperslab code #5355

Merged
merged 1 commit into from
Mar 7, 2025
Merged

Fix two assertion failures in hyperslab code #5355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions release_docs/RELEASE.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,20 @@ Bug Fixes since HDF5-2.0.0 release
===================================
Library
-------
- Fixed an assertion failure in H5S__hyper_make_spans()

Calling H5Sselect_hyperslab() on dataspaces with invalid extents could
result in an assertion failure in debug builds of the library if the
dataspace has an extent with a rank value of 0. This has been fixed by
converting the assertion failure into a normal error check.

- Fixed an assertion failure in H5S__hyper_new_span_info()

Calling H5Scopy() on hyperslab selection dataspaces with invalid extents
could result in an assertion failure in debug builds of the library if
the dataspace has an extent with a rank value of 0. This has been fixed
by converting the assertion failure into a normal error check.

- Fixed an error in H5Ddebug

H5Ddebug would fail for any chunked dataset with a chunk index, due to its
Expand Down
41 changes: 29 additions & 12 deletions src/H5Shyper.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2827,9 +2827,11 @@ H5S__hyper_new_span_info(unsigned rank)
FUNC_ENTER_PACKAGE

/* Sanity check */
assert(rank > 0);
assert(rank <= H5S_MAX_RANK);

if (rank == 0)
HGOTO_ERROR(H5E_DATASPACE, H5E_BADVALUE, NULL, "dataspace has invalid extent");

/* Allocate a new span info node */
if (NULL == (ret_value = (H5S_hyper_span_info_t *)H5FL_ARR_CALLOC(hbounds_t, rank * 2)))
HGOTO_ERROR(H5E_DATASPACE, H5E_CANTALLOC, NULL, "can't allocate hyperslab span info");
Expand Down Expand Up @@ -3204,7 +3206,7 @@ H5S__hyper_free_span(H5S_hyper_span_t *span)
static herr_t
H5S__hyper_copy(H5S_t *dst, const H5S_t *src, bool share_selection)
{
H5S_hyper_sel_t *dst_hslab; /* Pointer to destination hyperslab info */
H5S_hyper_sel_t *dst_hslab = NULL; /* Pointer to destination hyperslab info */
const H5S_hyper_sel_t *src_hslab; /* Pointer to source hyperslab info */
herr_t ret_value = SUCCEED; /* Return value */

Expand All @@ -3215,11 +3217,11 @@ H5S__hyper_copy(H5S_t *dst, const H5S_t *src, bool share_selection)
assert(dst);

/* Allocate space for the hyperslab selection information */
if (NULL == (dst->select.sel_info.hslab = H5FL_MALLOC(H5S_hyper_sel_t)))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code here and below was directly manipulating the destination dataspace, which could leave things in a bad state if failure occurs. This function had to be reworked to use the local dst_hslab and only manipulate the destination dataspace after things have succeeded.

if (NULL == (dst_hslab = H5FL_MALLOC(H5S_hyper_sel_t)))
HGOTO_ERROR(H5E_DATASPACE, H5E_CANTALLOC, FAIL, "can't allocate hyperslab info");
dst_hslab->span_lst = NULL;

/* Set temporary pointers */
dst_hslab = dst->select.sel_info.hslab;
src_hslab = src->select.sel_info.hslab;

/* Copy the hyperslab information */
Expand All @@ -3229,25 +3231,38 @@ H5S__hyper_copy(H5S_t *dst, const H5S_t *src, bool share_selection)

/* Check if there is hyperslab span information to copy */
/* (Regular hyperslab information is copied with the selection structure) */
if (src->select.sel_info.hslab->span_lst != NULL) {
if (src_hslab->span_lst != NULL) {
if (share_selection) {
/* Share the source's span tree by incrementing the reference count on it */
dst->select.sel_info.hslab->span_lst = src->select.sel_info.hslab->span_lst;
dst->select.sel_info.hslab->span_lst->count++;
dst_hslab->span_lst = src_hslab->span_lst;
dst_hslab->span_lst->count++;
} /* end if */
else
else {
/* Copy the hyperslab span information */
dst->select.sel_info.hslab->span_lst =
H5S__hyper_copy_span(src->select.sel_info.hslab->span_lst, src->extent.rank);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wasn't previously checking for errors here, which would hide problems when trying to copy a dataspace with an extent of 0 dims.

dst_hslab->span_lst = H5S__hyper_copy_span(src_hslab->span_lst, src->extent.rank);
if (NULL == dst_hslab->span_lst)
HGOTO_ERROR(H5E_DATASPACE, H5E_CANTCOPY, FAIL, "unable to copy hyperslab span information");
}
} /* end if */
else
dst->select.sel_info.hslab->span_lst = NULL;
dst_hslab->span_lst = NULL;

/* Copy the unlimited dimension info */
dst_hslab->unlim_dim = src_hslab->unlim_dim;
dst_hslab->num_elem_non_unlim = src_hslab->num_elem_non_unlim;

dst->select.sel_info.hslab = dst_hslab;

done:
if (ret_value < 0) {
if (dst_hslab) {
if (dst_hslab->span_lst && H5S__hyper_free_span_info(dst_hslab->span_lst) < 0)
HDONE_ERROR(H5E_DATASPACE, H5E_CANTFREE, FAIL, "unable to free hyperslab span information");

H5FL_FREE(H5S_hyper_sel_t, dst_hslab);
}
}

FUNC_LEAVE_NOAPI(ret_value)
} /* end H5S__hyper_copy() */

Expand Down Expand Up @@ -8603,12 +8618,14 @@ H5S__hyper_make_spans(unsigned rank, const hsize_t *start, const hsize_t *stride
FUNC_ENTER_PACKAGE

/* Check args */
assert(rank > 0);
assert(start);
assert(stride);
assert(count);
assert(block);

if (rank == 0)
HGOTO_ERROR(H5E_DATASPACE, H5E_BADVALUE, NULL, "dataspace has invalid extent");

/* Start creating spans in fastest changing dimension */
for (i = (int)(rank - 1); i >= 0; i--) {
hsize_t curr_low, curr_high; /* Current low & high values */
Expand Down
25 changes: 19 additions & 6 deletions src/H5Sselect.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,26 +218,39 @@ H5Sselect_copy(hid_t dst_id, hid_t src_id)
herr_t
H5S_select_copy(H5S_t *dst, const H5S_t *src, bool share_selection)
{
herr_t ret_value = FAIL; /* Return value */
H5S_t tmp_space;
bool copied_space = false;
herr_t ret_value = FAIL; /* Return value */

FUNC_ENTER_NOAPI(FAIL)

/* Check args */
assert(dst);
assert(src);

/* Release the current selection */
if (H5S_SELECT_RELEASE(dst) < 0)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of this PR was fixing H5Scopy() to return failure for datasets with a NULL extent (rank value of 0). However, the previous code didn't really account for failure and would release the selection in the destination space before determining if the selection copy portion actually succeeded or not. This could result in segfaults later on when the application tries to close the destination dataspace. This reworks things so the copy is made into a temporary dataspace and only releases the selection in the destination dataspace once things have succeeded.

HGOTO_ERROR(H5E_DATASPACE, H5E_CANTRELEASE, FAIL, "unable to release selection");
tmp_space = *dst;

/* Copy regular fields */
dst->select = src->select;
tmp_space.select = src->select;

/* Perform correct type of copy based on the type of selection */
if ((ret_value = (*src->select.type->copy)(dst, src, share_selection)) < 0)
if ((ret_value = (*src->select.type->copy)(&tmp_space, src, share_selection)) < 0)
HGOTO_ERROR(H5E_DATASPACE, H5E_CANTCOPY, FAIL, "can't copy selection specific information");
copied_space = true;

/* Release the current selection */
if (H5S_SELECT_RELEASE(dst) < 0)
HGOTO_ERROR(H5E_DATASPACE, H5E_CANTRELEASE, FAIL, "unable to release selection");

*dst = tmp_space;

done:

if (ret_value < 0) {
if (copied_space && H5S_SELECT_RELEASE(&tmp_space) < 0)
HGOTO_ERROR(H5E_DATASPACE, H5E_CANTRELEASE, FAIL, "unable to release selection");
}

FUNC_LEAVE_NOAPI(ret_value)
} /* end H5S_select_copy() */

Expand Down
93 changes: 93 additions & 0 deletions test/th5s.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3393,6 +3393,97 @@ test_h5s_bug3(void)
CHECK(ret, FAIL, "H5Sclose");
} /* test_h5s_bug3() */

/****************************************************************
**
** test_h5s_bug6(): Test calling H5Sselect_hyperslab() on a
** dataspace with a NULL extent such that an
** assertion failure happens when the library
** attempts to create new span information for
** the dataspace.
**
****************************************************************/
static void
test_h5s_bug6(void)
{
hsize_t start[] = {0};
hsize_t count[] = {1};
herr_t ret = SUCCEED;
hid_t space_id = H5I_INVALID_HID;

space_id = H5Screate(H5S_SIMPLE);
CHECK(space_id, H5I_INVALID_HID, "H5Screate");

ret = H5Sselect_hyperslab(space_id, H5S_SELECT_SET, start, NULL, count, NULL);
CHECK(ret, FAIL, "H5Sselect_hyperslab");

/* OR in another piece */
start[0] = 3;

H5E_BEGIN_TRY
{
ret = H5Sselect_hyperslab(space_id, H5S_SELECT_OR, start, NULL, count, NULL);
}
H5E_END_TRY
VERIFY(ret, FAIL, "H5Sselect_hyperslab");

ret = H5Sclose(space_id);
CHECK(ret, FAIL, "H5Sclose");
} /* test_h5s_bug6() */

/****************************************************************
**
** test_h5s_bug7(): Test calling H5Scopy() on a dataspace with
** a NULL extent such that an assertion failure
** happens when the library attempts to create
** new span information for the dataspace.
**
****************************************************************/
static void
test_h5s_bug7(void)
{
hsize_t dims[] = {10};
hsize_t start[] = {0};
hsize_t count[] = {1};
herr_t ret = SUCCEED;
hid_t space_id = H5I_INVALID_HID;
hid_t space_copy_id = H5I_INVALID_HID;

space_id = H5Screate_simple(1, dims, NULL);
CHECK(space_id, H5I_INVALID_HID, "H5Screate_simple");

ret = H5Sselect_hyperslab(space_id, H5S_SELECT_SET, start, NULL, count, NULL);
CHECK(ret, FAIL, "H5Sselect_hyperslab");

/* OR in pieces to make irregular hyperslab */
start[0] = 3;
count[0] = 3;

ret = H5Sselect_hyperslab(space_id, H5S_SELECT_OR, start, NULL, count, NULL);
CHECK(ret, FAIL, "H5Sselect_hyperslab");

/* Change dataspace extent to NULL extent */
ret = H5Sset_extent_none(space_id);
CHECK(ret, FAIL, "H5Sset_extent_none");

/* Copy the dataspace - should fail due to invalid dataspace extent */
H5E_BEGIN_TRY
{
space_copy_id = H5Scopy(space_id);
}
H5E_END_TRY
VERIFY(space_copy_id, H5I_INVALID_HID, "H5Scopy");

H5E_BEGIN_TRY
{
ret = H5Sclose(space_copy_id);
}
H5E_END_TRY
VERIFY(ret, FAIL, "H5Sclose");

ret = H5Sclose(space_id);
CHECK(ret, FAIL, "H5Sclose");
} /* test_h5s_bug7() */

/*-------------------------------------------------------------------------
* Function: test_versionbounds
*
Expand Down Expand Up @@ -3577,6 +3668,8 @@ test_h5s(void H5_ATTR_UNUSED *params)
test_h5s_bug1(); /* Test bug in offset initialization */
test_h5s_bug2(); /* Test bug found in H5S__hyper_update_diminfo() */
test_h5s_bug3(); /* Test bug found in H5S__combine_select() */
test_h5s_bug6(); /* Test bug found in H5S__hyper_make_spans() */
test_h5s_bug7(); /* Test bug found in H5S__hyper_new_span_info() */
test_versionbounds(); /* Test version bounds with dataspace */
} /* test_h5s() */

Expand Down
Loading