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

Fix uninitialized bytes in cmpd_dset test #4072

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

derobins
Copy link
Member

@derobins derobins commented Mar 5, 2024

Compound fill values were set to the integer -1, causing valgrind to flag 'uninitialized bytes' errors.

This is just a problem with the cmpd_dset test and not a core library problem.

Compound fill values were set to the integer -1, causing valgrind
to flag 'uninitialized bytes' errors.

This is just a problem with the cmpd_dset test and not a core
library problem.
@derobins derobins added Priority - 0. Blocker ⛔ This MUST be merged for the release to happen 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 Mar 5, 2024
@fortnern
Copy link
Member

fortnern commented Mar 5, 2024

Are you sure we're not losing something by changing the fill value from -1 to 0, which is more likely to be there "accidentally" if the library didn't fill the dataset due to a bug. I see using an int fill value for a compound type is wrong, but the fact that it was initialized to -1 implies it cares about the fill value being something other than 0. Maybe the code was copied from a different test where it matters more?

@derobins
Copy link
Member Author

derobins commented Mar 5, 2024

Are you sure we're not losing something by changing the fill value from -1 to 0, which is more likely to be there "accidentally" if the library didn't fill the dataset due to a bug. I see using an int fill value for a compound type is wrong, but the fact that it was initialized to -1 implies it cares about the fill value being something other than 0. Maybe the code was copied from a different test where it matters more?

It looks like a copy-paste error. I thought about initializing everything to -1, but that looked messy and the test clearly doesn't care about the fill value since it was initialized to "-1 + garbage bytes".

@derobins derobins merged commit e4d9b63 into HDFGroup:develop Mar 5, 2024
@derobins derobins added the Merge - To 1.14 This needs to be merged to HDF5 1.14 label Mar 6, 2024
lrknox pushed a commit to lrknox/hdf5 that referenced this pull request Mar 12, 2024
Compound fill values were set to the integer -1, causing valgrind
to flag 'uninitialized bytes' errors.

This is just a problem with the cmpd_dset test and not a core
library problem.
derobins added a commit that referenced this pull request Mar 13, 2024
* Do not enable szip for sanitizer runs (#4057)

* Add note to H5Tset_fields about needing to set datatype precision first (#4059)

* Offset of a floating-point type also needs to be accounted for

* Clarify ordering of H5Tset_precision and H5Tset_fields

* Fix issue where H5Tset_fields does not account for datatype offsets (#4061)

H5Tset_fields did not account for any offset in a floating-point datatype,
causing it to fail when a datatype's precision is correctly set such that
it doesn't include the offset bits.

* Ignore UserPresets and Use only C compiler for sanitizers (#4066)

* Remove user presets file

* Only use C compiler for sanitzers

* Rename incorrectly named option (#4067)

* Rename incorrectly named option

* Restore the correct uses of USING_MEMCHECKER

* Update release note

* Fix a memory leak in the cmpd_dset test (#4071)

This was due to not freeing a test buffer. It was not a core
library memory leak.

* Fix uninitialized bytes in cmpd_dset test (#4072)

Compound fill values were set to the integer -1, causing valgrind
to flag 'uninitialized bytes' errors.

This is just a problem with the cmpd_dset test and not a core
library problem.

* Update INSTALL files (#4052)

* Add NEWSLETTER and merge abi reports and add sha256sums (#4055)

* Fix uninitialized bytes in selection I/O test (#4073)

This was due to a complex type fill value being set to -1 instead
of a proper complex value. This was a test problem and not a core
library issue.

* fix path for S3 build path in CI (#4076)

* Correct paths for 1.14 and add lines missing from release_docs/INSTALL_CMake.txt.

---------

Co-authored-by: Allen Byrne <[email protected]>
Co-authored-by: jhendersonHDF <[email protected]>
Co-authored-by: Dana Robinson <[email protected]>
@derobins derobins deleted the cmpd_dset_uninit_bytes branch March 27, 2024 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
None yet
Development

Successfully merging this pull request may close these issues.

5 participants