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.

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

Fixed H5Ovisit2 change of behavior between 1.10.11 and v1.14.4.3 #4958

Closed
wants to merge 2 commits into from

Conversation

bmribler
Copy link
Contributor

In H5O_get_info() of 1.10.11:

HDmemset(oinfo, 0, sizeof(*oinfo))

set oinfo->type to 0, which was also the value of H5O_TYPE_GROUP, so oinfo->type accidentally had a correct value when the object was a group.

If oinfo->type were initialized to H5O_TYPE_UNKNOWN as it should be, the application would have behaved the same way as with 1.14.3, whose H5O_get_info() called H5O__reset_info2() that set oinfo->type to H5O_TYPE_UNKNOWN. So, it was a case of coincidentally correct behavior in 1.10.11.

The fix is to assign the correct value to oinfo->type early on instead of only when the requested info is H5O_INFO_BASIC. For that matter, probably all or some of the fields being set in the H5O_INFO_BASIC block should be changed the same way to prevent similar potential errors.

(Fixes GH #4941)

bmribler and others added 2 commits October 15, 2024 15:05
In H5O_get_info() of 1.10.11:
    HDmemset(oinfo, 0, sizeof(*oinfo))
set oinfo->type to 0, which was also the value of H5O_TYPE_GROUP, so
oinfo->type accidentally had a correct value when the object was a group.

If oinfo->type were initialized to H5O_TYPE_UNKNOWN as it should be, the
application would have behaved the same way as with 1.14.3, whose H5O_get_info()
called H5O__reset_info2() that set oinfo->type to H5O_TYPE_UNKNOWN.
So, it was a case of coincidentally correct behavior in 1.10.11.

The fix is to assign the correct value to oinfo->type early on instead
of only when the requested info is H5O_INFO_BASIC.  For that matter, probably
all or some of the fields being set in the H5O_INFO_BASIC block should be
changed the same way to prevent similar potential errors.

(GH HDFGroup#2136)
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.

The previous code for this routine is correct. The root cause of this issue is the visit.fields parameter set in H5Ovisit2 / H5Ovisit_by_name2 should have the H5O_INFO_BASIC flag set.

Note that it's possible that the user may not want the information from H5O_INFO_BASIC, so the code in H5O_get_info may need to be refactored info a new H5O_get_info_real, and the object header protected in H5O__visit, so that there's less overhead in getting both the information that H5O__visit needs and the information the application wants in the object info struct.

Happy to have a short call if you'd like to talk about this.

@bmribler
Copy link
Contributor Author

Yeah, the user's app passed H5O_INFO_META_SIZE into H5Ovisit2.
So, can't H5O_get_info get the object type to use but won't give it back to the H5Ovisit2?
Sure, that'd be great, Quincey. Thanks!

@bmribler bmribler closed this Oct 28, 2024
@bmribler bmribler deleted the fix_GH-2136 branch October 30, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants