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

Prevent H5Pset* routines from modifying default plists #5172

Merged
merged 7 commits into from
Dec 17, 2024

Conversation

mattjala
Copy link
Contributor

@mattjala mattjala commented Dec 9, 2024

This prevents the connector in H5VL_def_conn_s and the one used by H5P_DEFAULT/H5P_FILE_ACCESS_DEFAULT from diverging.

@mattjala mattjala added Merge - Develop Only This cannot be merged to previous versions of HDF5 (file format or breaking API changes) Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Component - C Library Core C library issues (usually in the src directory) Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub labels Dec 9, 2024
@mattjala mattjala force-pushed the h5p_set_vol_default_plist branch from 8bd42de to 2d5b05f Compare December 9, 2024 22:09
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.

In principal, yes, but rejecting the default property list of each type should apply to all the 'set' routines, for all types of property lists.

And, you don't need to reject H5P_DEFAULT, since H5P_object_verify() will already fail for that value.

@qkoziol
Copy link
Contributor

qkoziol commented Dec 10, 2024

I suggest that you put the appropriate checking inside H5P_object_verify()

@jhendersonHDF
Copy link
Collaborator

In principal, yes, but rejecting the default property list of each type should apply to all the 'set' routines, for all types of property lists.

In this case I'd call it a "not letting perfect be the enemy of good enough" situation. I asked Matt to create this PR due to the issue I encountered specifically with changing the VOL on the default FAPL. Fixing all the cases (and testing them) would be a decently larger effort.

And, you don't need to reject H5P_DEFAULT, since H5P_object_verify() will already fail for that value.

I'd actually prefer to explicitly reject H5P_DEFAULT and H5P_FILE_ACCESS_DEFAULT first as an invalid argument check and not rely on H5P_object_verify() doing that, in case someone updates it in the future to accept H5P_DEFAULT and another parameter to allow it to translate to the correct property list.

@qkoziol
Copy link
Contributor

qkoziol commented Dec 10, 2024

I understand your perspective, but I think the problem should be easy to address in a few hours of work: add a new 'is_lib_default' flag to the property list structure, which is only set to true for the actual default property lists, and is set to false when one of those property lists is copied.

Then add a parameter to H5P_object_verify(), or someplace else that's central to the 'set' calls, that indicates that H5P_object_verify() is being called from a 'set' routine. Then fail in H5P_object_verify() when both are true.

Probably only ~50 lines of actual code. (and 600 lines of trivial changes to calls to H5P_object_verify :-)

@mattjala mattjala changed the title Prevent H5Pset_vol from changing default VOL Prevent H5Pset* routines from modifying default plists Dec 10, 2024
@mattjala mattjala requested a review from qkoziol December 10, 2024 18:53
@mattjala mattjala force-pushed the h5p_set_vol_default_plist branch from cdea6c2 to 0261418 Compare December 10, 2024 20:00
src/H5Pfapl.c Outdated Show resolved Hide resolved
test/plists.c Outdated Show resolved Hide resolved
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.

Please merge the new and old tests together, so there's only one property list test.

Otherwise, looks good - thanks for taking the time to implement a holistic approach that addresses the problem at its root.

@mattjala mattjala force-pushed the h5p_set_vol_default_plist branch from f9c2b8d to 8c7dff4 Compare December 11, 2024 21:18
@lrknox lrknox merged commit 914640e into HDFGroup:develop Dec 17, 2024
68 checks passed
mattjala added a commit to mattjala/hdf5 that referenced this pull request Dec 17, 2024
* Prevent H5Pset_vol from changing default VOL
* Prevent public routines from modifying default plists
* Allow default backing FAPL for onion VFD
* Require FAPL in H5Pset_vol()
* Merge into existing H5P tests
mattjala added a commit to mattjala/hdf5 that referenced this pull request Dec 17, 2024
* Prevent H5Pset_vol from changing default VOL
* Prevent public routines from modifying default plists
* Allow default backing FAPL for onion VFD
* Require FAPL in H5Pset_vol()
* Merge into existing H5P tests
@mattjala mattjala deleted the h5p_set_vol_default_plist branch December 24, 2024 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - C Library Core C library issues (usually in the src directory) Merge - Develop Only This cannot be merged to previous versions of HDF5 (file format or breaking API changes) Priority - 3. Low 🔽 Code cleanup, small feature change requests, etc. Type - Bug / Bugfix Please report security issues to [email protected] instead of creating an issue on GitHub
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants