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

Add check for decoded datatype precision overflow #4315

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Add check for decoded datatype precision overflow #4315

wants to merge 1 commit into from

Conversation

jhendersonHDF
Copy link
Collaborator

Adds a check for the case where a decoded datatype's precision could overflow SIZE_MAX due to the size of a datatype being larger than SIZE_MAX / 8

@jhendersonHDF jhendersonHDF added Merge - To 1.14 Priority - 2. Medium ⏹ It would be nice to have this in the next release Component - C Library Core C library issues (usually in the src directory) Type - Improvement Improvements that don't add a new feature or functionality labels Apr 3, 2024
@jhendersonHDF
Copy link
Collaborator Author

Related to #4309. Adds a check to make sure that dt->shared->size * 8 doesn't overflow SIZE_MAX before we set a decoded datatype's precision or use the previous calculation later on.

@jhendersonHDF
Add check for decoded datatype precision overflow
af51b92 
Adds a check for the case where a decoded datatype's precision
could overflow SIZE_MAX due to the size of a datatype being
larger than SIZE_MAX / 8
jhendersonHDF
jhendersonHDF commented Apr 3, 2024
View reviewed changes
src/H5Tpkg.h
@@ -45,8 +45,7 @@
#define H5T_NAMELEN 32

/* Macro to ease detecting "complex" datatypes (i.e. those with base types or fields) */
#define H5T_IS_COMPLEX(t) \
((t) == H5T_COMPOUND || (t) == H5T_ENUM || (t) == H5T_VLEN || (t) == H5T_ARRAY || (t) == H5T_REFERENCE)
#define H5T_IS_COMPLEX(t) ((t) == H5T_COMPOUND || (t) == H5T_ENUM || (t) == H5T_VLEN || (t) == H5T_ARRAY)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe H5T_REFERENCE was only added to this macro to make the logic in H5T_set_loc work correctly for reference types. Reference types are treated as atomic types, but would be excluded when checking a datatype with the H5T_IS_ATOMIC macro due to the !H5T_IS_COMPLEX check.

derobins
derobins reviewed Apr 3, 2024
View reviewed changes
src/H5T.c
@@ -6347,8 +6349,7 @@ H5T_set_loc(H5T_t *dt, H5VL_object_t *file, H5T_loc_t loc)
* them as part of the same blob)*/
/* (If the force_conv flag is _not_ set, the type cannot change in size, so don't recurse) */
if (dt->shared->parent->shared->force_conv &&
H5T_IS_COMPLEX(dt->shared->parent->shared->type) &&
(dt->shared->parent->shared->type != H5T_REFERENCE)) {
H5T_IS_COMPLEX(dt->shared->parent->shared->type)) {
Copy link
Member

Choose a reason for hiding this comment

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

Weird indenting

qkoziol
qkoziol reviewed Apr 3, 2024
View reviewed changes
Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

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

What about the other places that use H5T_IS_COMPLEX?

@jhendersonHDF
Copy link
Collaborator Author

What about the other places that use H5T_IS_COMPLEX?

When I looked at the other occurrences previously, there are four occurrences in H5T.c, three of which are addressed here. The other one is in H5T_detect_class, which is just trying to call H5T_detect_class recursively on container datatypes and would return right away for H5T_REFERENCE.

There is one occurrence in H5Tvisit.c in H5T_visit, which only seems to be used for recursive calls of H5T__upgrade_version_cb, which returns right away for H5T_REFERENCE.

There are three occurrences in H5Tvlen.c, all in H5T__vlen_reclaim. Those ones are a bit trickier and might need an additional H5T_IS_REF check in case the datatype is something like a variable-length of array of references. I didn't see any memory leaks from this, but it's possible we aren't testing datatypes like this. I'll look closer.

@jhendersonHDF jhendersonHDF marked this pull request as draft April 3, 2024 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qkoziol qkoziol qkoziol left review comments

@derobins derobins derobins approved these changes

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

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

@fortnern fortnern Awaiting requested review from fortnern fortnern 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

At least 2 approving reviews are required to merge this pull request.

Assignees

@jhendersonHDF jhendersonHDF

Labels
Component - C Library Core C library issues (usually in the src directory) Priority - 2. Medium ⏹ It would be nice to have this in the next release Type - Improvement Improvements that don't add a new feature or functionality
Projects
Downstream HDF5 Merges
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jhendersonHDF @derobins @qkoziol @byrnHDF