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 assertion failure in H5S__copy_pnt_list() #5352

Draft
wants to merge 2 commits into
base: develop
Choose a base branch
from
+50 −4
Draft

Fix assertion failure in H5S__copy_pnt_list() #5352

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4ba5c1a
Fix assertion failure in H5S__copy_pnt_list()
jhendersonHDF Mar 5, 2025
d9e2032
Add release note
jhendersonHDF Mar 5, 2025
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
Original file line number Diff line number Diff line change
@@ -514,6 +514,14 @@ Bug Fixes since HDF5-2.0.0 release
===================================
Library
-------
- Fixed an assertion failure in H5S__copy_pnt_list()

If a dataspace with a point selection in it had its extent reset with
the H5Sset_extent_none() function, any copy operations on that dataspace
would result in an assertion failure in debug builds of the library
due to the dataspace having 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
5 changes: 4 additions & 1 deletion src/H5Spoint.c
View file
Original file line number Diff line number Diff line change
@@ -800,12 +800,15 @@ H5S__copy_pnt_list(const H5S_pnt_list_t *src, unsigned rank)

/* Sanity checks */
assert(src);
assert(rank > 0);

/* Allocate room for the head of the point list */
if (NULL == (dst = H5FL_CALLOC(H5S_pnt_list_t)))
HGOTO_ERROR(H5E_DATASPACE, H5E_CANTALLOC, NULL, "can't allocate point list node");

/* If source dataspace has no extent, just return new point list node */
if (rank == 0)
HGOTO_DONE(dst);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@qkoziol I'm considering revising this. While it makes sense to me to return an empty point list node when the dataspace's extent has 0 dimensions (as we can't even tell how to copy the points over in that case), this causes problems elsewhere where the library expects space->select.sel_info.pnt_lst->head to be non-NULL. I have a fix for that as well, but I'm thinking of just having H5Scopy() / H5Sselect_copy() return failure for point and hyperslab selection dataspaces with a 0 dimension extent, since they both don't support H5S_SCALAR or H5S_NULL dataspaces and can only end up here when H5Sset_extent_none() is called on them. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would be fine. Another solution might be to have H5Sset_extent_none() clear the selection also (since you can't select anything within a "none" extent"). Generically, I would suggest reseting the selection any time that the extent's rank changes (also).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seems like a better solution to me. I suppose it makes sense to reset the selection to 'all' since that's what dataspaces start with?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, back to 'all' for all changes in rank would be consistent with dataspace creation.


curr = src->head;
new_tail = NULL;
while (curr) {
41 changes: 38 additions & 3 deletions test/th5s.c
View file
Original file line number Diff line number Diff line change
@@ -3360,10 +3360,10 @@ test_h5s_bug3(void)
hid_t space3 = H5I_INVALID_HID;

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

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

/* Select a single, different element in each dataspace */
start[0] = 0;
@@ -3382,7 +3382,7 @@ test_h5s_bug3(void)
* wasn't a hyperslab.
*/
space3 = H5Scombine_select(space1, H5S_SELECT_AND, space2);
CHECK(space3, FAIL, "H5Scombine_select");
CHECK(space3, H5I_INVALID_HID, "H5Scombine_select");

/* Close dataspaces */
ret = H5Sclose(space1);
@@ -3393,6 +3393,40 @@ test_h5s_bug3(void)
CHECK(ret, FAIL, "H5Sclose");
} /* test_h5s_bug3() */

/****************************************************************
**
** test_h5s_bug4(): Test copying a dataspace with a point
** selection after the dataspace's extent has
** been reset with H5Sset_extent_none().
**
****************************************************************/
static void
test_h5s_bug4(void)
{
hsize_t dims[] = {10};
hsize_t points[] = {0};
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_elements(space_id, H5S_SELECT_SET, 1, points);
CHECK(ret, FAIL, "H5Sselect_elements");

ret = H5Sset_extent_none(space_id);
CHECK(ret, FAIL, "H5Sset_extent_none");

space_copy_id = H5Scopy(space_id);
CHECK(space_id, H5I_INVALID_HID, "H5Scopy");

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

/*-------------------------------------------------------------------------
* Function: test_versionbounds
*
@@ -3577,6 +3611,7 @@ 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_bug4(); /* Test bug in point selection copying */
test_versionbounds(); /* Test version bounds with dataspace */
} /* test_h5s() */