Fix leak of internal IDs registered during compound datatype conversion #4459
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jhendersonHDF
added
Merge - To 1.14
Priority - 1. High 🔼
jhendersonHDF
requested review from
lrknox,
derobins,
byrnHDF,
fortnern,
qkoziol,
vchoi-hdfgroup,
bmribler,
glennsong09,
mattjala and
brtnfld
as code owners
|
Will add some more fixes to this PR after testing, based on feedback in h5py/h5py#2419 |
jhendersonHDF
force-pushed
the
H5T__unlock_cb_segfault
branch
2 times, most recently
from
37c6033 to
a9a1638
Compare
…conversion Also fixes issues with handling of partially initialized datatypes during library shutdown
|
Before the type conversion optimization changes in the 1.14.4 release, the previous behavior of the library's compound datatype conversion function was to always register IDs for the members of the source and destination compound datatypes once during conversion path initialization and cache those IDs. After those changes, the registration of IDs was deferred until after the conversion path for each of the source compound members being converted was determined. That way, the library could determine if it needs to register IDs based on whether any of the conversion functions used is an application conversion function or whether a conversion exception function was provided. However, this created a leak of these IDs when the compound conversion function recalculates cached data after the library's table of conversion functions is modified. Since this resulted in multiple IDs pointing to the same |
jhendersonHDF
commented
May 7, 2024
There was a problem hiding this comment.
The changes in this file are for making the library handle unlocking/closing of partially initialized datatypes more gracefully
jhendersonHDF
commented
May 7, 2024
| @@ -1874,7 +1873,6 @@ H5T__close_cb(H5T_t *dt, void **request) | |||
|
|
|||
| /* Sanity check */ | |||
| assert(dt); | |||
| assert(dt->shared); | |||
There was a problem hiding this comment.
Handled further down when dt is passed to H5T_close
jhendersonHDF
changed the title
brtnfld
added
Priority - 0. Blocker ⛔
byrnHDF
approved these changes
May 10, 2024
bmribler
reviewed
May 10, 2024
| * since the only information that needs to be recalculated is the conversion | ||
| * paths used. | ||
| */ | ||
| if (priv->src_memb_id[i] == H5I_INVALID_HID) { |
There was a problem hiding this comment.
Is it a guarantee that the memb_id's are initialized to H5I_INVALID_HID?
There was a problem hiding this comment.
Yes, they are initialized a bit above around line 174 and 180
lrknox
approved these changes
May 10, 2024
jhendersonHDF
added a commit
to jhendersonHDF/hdf5
that referenced
this pull request
May 13, 2024
…conversion (HDFGroup#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown
jhendersonHDF
added a commit
to jhendersonHDF/hdf5
that referenced
this pull request
May 13, 2024
…conversion (HDFGroup#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown
jhendersonHDF
added a commit
to jhendersonHDF/hdf5
that referenced
this pull request
May 13, 2024
…conversion (HDFGroup#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown
jhendersonHDF
added a commit
to jhendersonHDF/hdf5
that referenced
this pull request
May 13, 2024
…conversion (HDFGroup#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown
qkoziol
pushed a commit
to qkoziol/hdf5
that referenced
this pull request
May 13, 2024
…conversion (HDFGroup#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown
byrnHDF
pushed a commit
to byrnHDF/hdf5
that referenced
this pull request
May 14, 2024
…conversion (HDFGroup#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown
lrknox
pushed a commit
to lrknox/hdf5
that referenced
this pull request
May 20, 2024
…conversion (HDFGroup#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown
lrknox
added a commit
that referenced
this pull request
May 23, 2024
* win32defs: Fix Wundef warning (#4467) * Refactor error handling code to eliminate internal ID calls (#4453) All calls to the H5I routines are now made in API routines (sometimes in FUNC_ENTER/LEAVE_* macros), except for some calls to H5E_clear_stack() within the library, but I'm planning to remove those over time. Also, made all the library internal error messages into static const variables, instead of malloc'ing them, which means that they can just be referenced and not copied. Several new and updated auto-generated header files were necessary to enable this. * CMake: Fix mingw/fortran build (#4466) * Update for blosc2 in plugins and prefix hdf5 cmake varnames (#4468) * Fix an issue where compound datatype member IDs can be leaked during conversion (#4459) Also fixes issues with handling of partially initialized datatypes during library shutdown * H5Group: Fix operator= (#4473) Closes #4472 * Fix github issue #2523: doxygen -- fix grammatically incorrect sentence alias (#4474) * Remove env step not used by CI in testing (#4476) * Add H5fortkit dependecy for H5Rff.F90 (#4482) * Properly clean up cache when failing to load an object header (#4477) * Properly clean up cache when failing to load an object header * Don't check message type a second time in H5G__open_oid if the first attempt returns error * Add more asserts to H5O__assert() to avoid segfaults --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> * Add a missing image from the original document (#4490) * Disable EOF checks for SWMR readers in more cases. (#4489) Fixes a race condition where the reader opens the file and sets its EOF from the file's size (from the stat() call in the driver open callback). Then, before the reader can read the file's superblock, a SWMR writer races in, extends the file, and closes the file, writing an updated superblock with the 'writer' and 'SWMR writer' flags in the superblock off (appropriately). Then the reader proceeds to read the superblock, and flags the EOF as wrong. Taking out the check for the 'writer' and 'SWMR writer' flags will cause SWMR readers to avoid flagging the file as incorrect. * Remove unnecessary fortran install (#4498) * Only one version of binaries is produced for platforms (#4496) * Fix for github issue #2220. (#4497) Document the limitation in the Passthrough Conncector section of the VOL Connector Author Guide. The limitation is posted by Neil in the github issue on Dec 22, 2022. * Release asset tarballs with no version filenames (#4494) * Improve spec. reading superblock into cache (a little) by using v2 size (#4491) * Improve spec. reading superblock into cache (a little) by using v2 size Instead of reading the absolute minimal possible, use the likely value of a v2+ superblock w/8-byte addresses & lengths. * Fix for github Issue #1388 can't delete renamed dense attribute with corder tracking enabled (#4462) * Fix for github issue #1388: can't delete renamed dense attribute with corder tracking enabled The problem occurs in step 3(b) below which will delete the attribute with corder x from the creation order index v2 B-tree. The rename sequence in H5A__dense_rename() occurs in the following order: 1) The old attribute with corder x was removed from the creation order index v2 B-tree 2) The new renamed attribute was inserted via H5A__dense_insert(): (a) insert the attribute with new name j into the name index v2 B-tree (b) insert the attribute with corder x into the creation order index v2 B-tree 3) The old attribute was removed via H5A__dense_remove(): (a) remove the attribute with old name k from the name index v2 B-tree (b) remove the attribute with coorder x from the creation order index v2 B-tree Fix: deactivate the "corder_bt2_addr" field so that H5A__dense_remove() won't delete the attribute with corder x from the creation order index v2 B-tree. * Fix/revert a libtool sed hack (#4501) * Revert "Remove Autotools sed hack (#3848)" This reverts commit 8b3ffde. * Fix libtool sed cleanup on MacOS Convert sed -i line to sed > libtool.bak && mv libtool.bak libtool to avoid non-portable -i option. * Update src/H5public.h * Set H5 specific vars immediately if legacy find (#4512) * Correct find process vars (vs in-line build) * Correct SZIP find * Everything is libaec 1.0.6 or newer * Correct option help text
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.