Repository navigation
Conversation
A dataset's datatype is shared by all open handles of the dataset, and H5T_set_loc() stamps the file of the handle that opened it first into every variable-length type it can reach, also below compound members and array base types. H5T_patch_vlen_file() repairs that file before reading or writing data, but only for a variable-length type at the top level. After the file of the first handle was closed, reading through another handle therefore crashed in H5F_addr_decode() for a variable-length string in a compound type (h5py/h5py#2920). Walk the same subtypes as H5T_set_loc() instead. Based on the analysis and patch in https://github.com/beitler/hdf5-vlen. Co-authored-by: anton <2046752+beitler@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
H5D_get_create_plist() and the storage initialization of H5D__alloc_storage() and H5D__set_extent() convert the fill value with the dataset's shared datatype, without repairing the file of its variable-length types first as H5Dread() and H5Dwrite() do. After the file of the handle that opened the dataset first was closed, this crashed in H5F_addr_decode() for a dataset with a variable-length fill value. netCDF-C calls H5Dget_create_plist() when opening a file, so it crashed when opening a netCDF-4 file with string variables again after closing one of several handles to it. Add a test that uses variable-length data through a dataset handle after closing another handle of the same dataset, for these cases and for the variable-length type in a compound type from the previous commit. Co-authored-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review ChecklistThis PR touches the following areas. Each needs a sign-off
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Any chance we could get this into a 2.2.1 release? We already had multiple people complaining about segfaults. |
Currently, we're looking toward having a 2.3.0 release soon. We don't have any plans at the moment to have a 2.2.1 release though. |
| * in H5T_set_loc(), so that they stay part of the same blob. */ | ||
| if (dt->shared->parent && dt->shared->parent->shared->force_conv && | ||
| H5T_IS_COMPOSITE(dt->shared->parent->shared->type) && | ||
| (dt->shared->parent->shared->type != H5T_REFERENCE)) |
There was a problem hiding this comment.
Since the H5T_REFERENCE case below is a no-op, it probably doesn't need to be explicitly skipped here (looks to just be a copy-paste from H5T_set_loc() where that case actually does something). Since the H5T_ARRAY case below doesn't have this, either drop it here or make both have it (with a preference for the former)
| assert(file); | ||
|
|
||
| if ((dt->shared->type == H5T_VLEN) && dt->shared->u.vlen.file != file) | ||
| dt->shared->u.vlen.file = file; |
There was a problem hiding this comment.
From the header comments, usages of the similar H5T_patch_file() and looking at this from a design perspective as well, it seems that the original intention for these functions was for patching only the most top-level datatype's file pointer and carrying this hack down points to a design issue to me. Since cases where H5T_patch_vlen_file() needs to be called appear to keep being missed, it would seem better for variable-length types to keep a reference to the top-level file pointer and read "upward" instead of having to propagate changes downward. @fortnern any thoughts?
Thanks for your honest review. I have by no means a good understanding of this codebase but I agree that the current approach seems very fragile. The approach in this PR is basically whack-a-mole. May I propose to clean up this PR and ship it as is since it fixes real issues. And then continue with a follow up issue that discusses the approach to fix the design? |
Sure, I'd just generally like for this to get more review because the previous assumption seemed to be that the vlen file pointer only needed to be patched at the start of I/O calls and as that appears to not be the case here, I'd like to look for additional places it needs to be done. |
This branch has not been deployed
5 participants
Describe your changes
A dataset's datatype is shared by all open handles of the dataset, and its variable-length types store the file of the handle that opened the dataset first. After that file handle is closed, e.g. when the same file was opened twice, converting variable-length data through another handle of the dataset crashes in
H5F_addr_decode().H5T_patch_vlen_file()repairs this, but only for a top-level variable-length type and only before reading or writing data.H5T_patch_vlen_file()also repairs variable-length types nested in compound and array types, walking the same subtypes asH5T_set_loc(). This fixes reading a vlen string in a compound type (Segfault on interleaved reads on same dataset from different file handles h5py/h5py#2920); the patch is from the analysis in https://github.com/beitler/hdf5-vlen.H5D_get_create_plist(),H5D__alloc_storage()andH5D__set_extent()callH5T_patch_vlen_file()before converting the fill value. Without it,H5Dget_create_plist()on a vlen string dataset with a fill value crashed, which netCDF-C calls when opening a netCDF-4 file with string variables (Close files of garbage collected file managers once their lock is free pydata/xarray#11692), as didH5Dset_extent(), which writes the fill value into new storage.test_vl_closed_handleintest/tvlstr.cfor all three cases; it segfaults without the changes insrc/.Issue ticket number (GitHub or JIRA)
Fixes #6721 (see also h5py/h5py#2920 and pydata/xarray#11692)
Checklist before requesting a review
[This is Claude Code on behalf of Michael Niklas]
🤖 Generated with Claude Code