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 h5py testing failure due to invalid datatype IDs #4321

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Fix h5py testing failure due to invalid datatype IDs #4321

merged 2 commits into from
Apr 4, 2024

Conversation

jhendersonHDF
Copy link
Collaborator

Fixes an issue where invalid datatype IDs are passed to application conversion functions in the case where the top-level conversion function is a library-internal function that operates on a container-like datatype, but one or more of the base datatype members are converted with an application conversion function.

@jhendersonHDF
Fix h5py testing failure due to invalid datatype IDs
9b1b65a 
Fixes an issue where invalid datatype IDs are passed to application conversion
functions in the case where the top-level conversion function is a library-internal
function that operates on a container-like datatype, but one or more of the
base datatype members are converted with an application conversion function.
@jhendersonHDF jhendersonHDF added Merge - To 1.14 This needs to be merged to HDF5 1.14 Priority - 0. Blocker ⛔ This MUST be merged for the release to happen Component - C Library Core C library issues (usually in the src directory) Component - Testing Code in test or testpar directories, GitHub workflows Type - Bug Please report security issues to [email protected] instead of creating an issue on GitHub labels Apr 4, 2024
jhendersonHDF
jhendersonHDF commented Apr 4, 2024
View reviewed changes
src/H5T.c
@@ -3394,6 +3394,7 @@ H5T__create(H5T_class_t type, size_t size)
/* Copy the default string datatype */
if (NULL == (dt = H5T_copy(origin_dt, H5T_COPY_TRANSIENT)))
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "unable to copy");
dt->shared->type = type;
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 logic for H5Tcreate with H5T_INTEGER, H5T_FLOAT, H5T_TIME and H5T_STRING weren't setting the new datatype's type to the given type, like the other cases below. This meant that newly-created types in any combination of these four categories could compare equal to each other if their sizes are the same.

jhendersonHDF
jhendersonHDF commented Apr 4, 2024
View reviewed changes
src/H5T.c
@@ -4578,6 +4579,8 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, bool superset)
/*
* Compound data types...
*/
if (dt1->shared->u.compnd.nmembs == 0 && dt2->shared->u.compnd.nmembs == 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.

Shortcut for comparing two compounds with 0 members. Also avoids a segfault in the debugging code below, but I added a check there to be safe.

jhendersonHDF
jhendersonHDF commented Apr 4, 2024
View reviewed changes
src/H5T.c
@@ -4660,6 +4665,8 @@ H5T_cmp(const H5T_t *dt1, const H5T_t *dt2, bool superset)
/*
* Enumeration data types...
*/
if (dt1->shared->u.enumer.nmembs == 0 && dt2->shared->u.enumer.nmembs == 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.

Shortcut for comparing two enums with 0 members. Also avoids a segfault in the debugging code below, but I added a check there to be safe.

jhendersonHDF
jhendersonHDF commented Apr 4, 2024
View reviewed changes
src/H5Tconv.c
@@ -2134,18 +2143,18 @@ H5T__conv_struct_init(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, co
HGOTO_ERROR(H5E_RESOURCE, H5E_CANTALLOC, FAIL,
"couldn't allocate destination compound member datatype array");

priv->need_ids = (cdata->command == H5T_CONV_INIT && conv_ctx->u.init.cb_struct.func) ||
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the container datatype conversions (compound, vlen, array), the code previously didn't take into account the case where the top-level datatype uses a library-internal conversion function, but one or more of the member types uses an application conversion function. Refactored so that we delay checking for and creating IDs until after we determine the conversion path to be used.

jhendersonHDF
jhendersonHDF commented Apr 4, 2024
View reviewed changes
src/H5Tconv.c
@@ -2701,6 +2722,8 @@ H5T__conv_struct_opt(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, con
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");
if (!bkg)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can hit an assertion below if H5Tconvert is called from an application with NULL for the background buffer. Converted this to an error check.

jhendersonHDF
jhendersonHDF commented Apr 4, 2024
View reviewed changes
src/H5Tconv.c
@@ -2952,6 +2968,10 @@ H5T__conv_enum_init(const H5T_t *src, const H5T_t *dst, H5T_cdata_t *cdata, cons
if (NULL == (priv->dst_copy = H5T_copy(dst, H5T_COPY_ALL)))
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCOPY, FAIL, "unable to copy destination datatype");

/* Nothing more to do if enum has no members */
if (0 == src->shared->u.enumer.nmembs)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Moved the check for 0 enum members down so that the cached private data still gets a copy of the source and destination datatypes in case H5T_cmp is called later.

@jhendersonHDF
Copy link
Collaborator Author

This is a fix after #4113

@lrknox lrknox merged commit 3db6957 into HDFGroup:develop Apr 4, 2024
58 checks passed
lrknox pushed a commit to lrknox/hdf5 that referenced this pull request Apr 4, 2024
@jhendersonHDF @lrknox
Fix h5py testing failure due to invalid datatype IDs (HDFGroup#4321)
db0cf1c 
Fixes an issue where invalid datatype IDs are passed to application conversion
functions in the case where the top-level conversion function is a library-internal
function that operates on a container-like datatype, but one or more of the
base datatype members are converted with an application conversion function.
lrknox added a commit that referenced this pull request Apr 4, 2024
@lrknox
Sync develop changes April 3 - April4 to hdf5_1_14 (#4324)
3d3f15a 
* Remove VS ptable error from Known Problems (#4317)

* Simply check for datatypes with unusual number of unused bits (#4309)

Avoids potential undefined behavior in H5T_is_numeric_with_unusual_unused_bits

* Fix issues with empty or uninitialized link names (#4322)

Converts an assertion in H5G_loc_find into a normal error
check that checks for empty link names

Initializes H5O_link_t structure early in H5G__ent_to_link
to avoid trying to free potentially uninitialized memory

Checks for an empty link name after H5MM_strndup in
H5G__ent_to_link

Fixes GitHub #4307

* Fix h5py testing failure due to invalid datatype IDs (#4321)

Fixes an issue where invalid datatype IDs are passed to application conversion
functions in the case where the top-level conversion function is a library-internal
function that operates on a container-like datatype, but one or more of the
base datatype members are converted with an application conversion function.

* Revise _Float16 configure checks (#4323)

Run configure checks with and without CFLAGS/CMAKE_C_FLAGS since some
compilers work in one case while not working in the other case

Sync CMake configure checks with Autotools
lrknox added a commit to lrknox/hdf5 that referenced this pull request Apr 4, 2024
@lrknox
Sync develop changes April 3 - April4 to hdf5_1_14 (HDFGroup#4324)
6cbf3cb 
* Remove VS ptable error from Known Problems (HDFGroup#4317)

* Simply check for datatypes with unusual number of unused bits (HDFGroup#4309)

Avoids potential undefined behavior in H5T_is_numeric_with_unusual_unused_bits

* Fix issues with empty or uninitialized link names (HDFGroup#4322)

Converts an assertion in H5G_loc_find into a normal error
check that checks for empty link names

Initializes H5O_link_t structure early in H5G__ent_to_link
to avoid trying to free potentially uninitialized memory

Checks for an empty link name after H5MM_strndup in
H5G__ent_to_link

Fixes GitHub HDFGroup#4307

* Fix h5py testing failure due to invalid datatype IDs (HDFGroup#4321)

Fixes an issue where invalid datatype IDs are passed to application conversion
functions in the case where the top-level conversion function is a library-internal
function that operates on a container-like datatype, but one or more of the
base datatype members are converted with an application conversion function.

* Revise _Float16 configure checks (HDFGroup#4323)

Run configure checks with and without CFLAGS/CMAKE_C_FLAGS since some
compilers work in one case while not working in the other case

Sync CMake configure checks with Autotools
@lrknox lrknox mentioned this pull request Apr 4, 2024
Merged
@jhendersonHDF jhendersonHDF deleted the app_conv_ids_fix branch April 4, 2024 17:46
qkoziol pushed a commit to qkoziol/hdf5 that referenced this pull request Apr 8, 2024
@jhendersonHDF @qkoziol
Fix h5py testing failure due to invalid datatype IDs (HDFGroup#4321)
39ba3bf 
Fixes an issue where invalid datatype IDs are passed to application conversion
functions in the case where the top-level conversion function is a library-internal
function that operates on a container-like datatype, but one or more of the
base datatype members are converted with an application conversion function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lrknox lrknox lrknox approved these changes

@byrnHDF byrnHDF byrnHDF approved these changes

@derobins derobins Awaiting requested review from derobins derobins is a code owner

@fortnern fortnern Awaiting requested review from fortnern fortnern is a code owner

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

@bmribler bmribler Awaiting requested review from bmribler bmribler is a code owner

@glennsong09 glennsong09 Awaiting requested review from glennsong09 glennsong09 is a code owner

@mattjala mattjala Awaiting requested review from mattjala mattjala is a code owner

@brtnfld brtnfld Awaiting requested review from brtnfld brtnfld is a code owner

Assignees
No one assigned
Labels
Component - C Library Core C library issues (usually in the src directory) Component - Testing Code in test or testpar directories, GitHub workflows Merge - To 1.14 This needs to be merged to HDF5 1.14 Priority - 0. Blocker ⛔ This MUST be merged for the release to happen Type - Bug Please report security issues to [email protected] instead of creating an issue on GitHub
Projects
Downstream HDF5 Merges
Status: Needs Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jhendersonHDF @lrknox @byrnHDF