format: check a chunk layout against its dataspace, as upstream CVE-2026-19025 does (0.5.2) - #15
Merged
Merged
Conversation
libhdf5 now refuses a chunked layout whose dimensionality is not the dataspace rank plus one as H5O__layout_decode decodes it (#6508, CVE-2026-19025). The port decodes the two messages together in Hdf5Reader::classify_object and ReopenWalk::plan, so it checks there.
read_slice_into_unconverted summed starts[d] + counts[d] unchecked, so a caller-supplied start near u64::MAX wrapped back inside the extent and read unrelated bytes (a panic under overflow checks). The writer and the region reference path already did the checked sum each on their own; the one helper now owns the rule, and the dataset-level read_slice applies it before it sizes the destination, so an oversized count is refused as a selection instead of a failed allocation.
The slice path added the layout's contiguous address to each run offset unchecked; a crafted address near u64::MAX wrapped to an offset inside the file and served those bytes as the selection. The external-file path did the same with the EFL slot offset. Both sums are now checked, as the whole-dataset path's already was.
H5D__compact_init refuses a compact dataset whose payload is not exactly the extent's element count times the stored element size; the port took the payload as given, so the slice path indexed a short one past its end. The compact rule joins the chunk-rank rule in the one check the reader's classify_object and the writer's ReopenWalk::plan already make between a layout and its sibling messages, with DataspaceMessage::element_count supplying H5S_GET_EXTENT_NPOINTS.
read_vlen_objects reserved the extent's element count before it knew how many references the image held, and collect_fa_chunk_entries reserved, multiplied and read from the fixed array header's element count as given; either claim, crafted, aborted the process on allocation. The vlen reservation is now bounded by the image, the fixed-array count is refused unless its elements fit in the file before it sizes anything, and the free-space section count is bounded by its block the same way.
read_chunk_runs_into summed a run's file offset and length unchecked to ask whether the next run continues it; an index entry a few bytes short of u64::MAX made that sum overflow before the read at that address could fail on its own.
lzf_decompress reserved the declared chunk size before decoding a byte, so a crafted cd_values[2] aborted the process on allocation. The stream bounds its own expansion at 88x, and h5py's lzf_filter.c treats the declared size as a buffer hint it grows past on E2BIG, so the reservation is now the smaller of the two and the output stays unbounded by it.
decompress took the chunk's declared size and the pipeline's block and scanline parameters as given: the output was reserved up front, the RSI size multiplied unchecked, and BitReader fabricated zero bits past the end of the input, which decode as zero blocks for as long as the declaration asks. The limits are SZ_MAX_PIXELS_PER_BLOCK and SZ_MAX_PIXELS_PER_SCANLINE (szlib.h), applied to compress as well so the encoder cannot write what the decoder refuses; the reservation is fallible; and a stream that runs out before the declared output is the error libaec's short output would have been, while one that ends inside its last RSI still completes.
The band buffers of a multi-frame-chunk dataset live in SwmrWriter, so Hdf5Writer's finalize, which Drop runs when close was never called, cannot see them, and every frame in one is already counted into the extent: a dropped writer left them reading back as fill. SwmrWriter now has a Drop that drains them ahead of the field drop, and close finalizes through Hdf5Writer::close_in_place because a type with Drop cannot move the writer out to close it.
flush published the extent, which counts every frame a band buffer holds, but wrote no chunk for the band until it completed, so a reader saw the frames of the partial band as fill. write_band now takes whether the buffer is kept: a flush writes the band zero-padded and keeps its frames, the completing append (or close) writes the same chunks whole and clears it, so the band's position never depends on what a flush wrote.
The checked sum on the external file list path landed in 5210a26 with no test of its own; this one patches the slot offset to u64::MAX - 4 and selects past the gap, which overflowed at reader.rs:1753 before the fix.
report_path stores the probe as a path under the repo root, so a run made in a worktree writes the same report line as one made in the main checkout instead of leaving the worktree's path behind after it is gone.
A flush writes the partial band and the completing append rewrites it; for a filtered dataset a rewrite whose compressed size changed moves to a new block and, under SWMR, keeps the old one (place_chunk), so each flush of such a band costs its chunks in file space for the rest of the session.
A mapped page a truncation takes away faults the process on its next touch, and nothing in this process can guard against that once the map is out; the shared lock is what keeps a writer that honours locks from opening the file meanwhile, and a SWMR writer, which streams with its lock released, never shrinks a file. for_read_only therefore maps only with lock_held, and release_lock drops the map with the lock, so a policy that waived the lock reads through the descriptor, where a truncated file is an UnexpectedEof.
filter_szip_ec and filter_szip_nn put libaec-written chunks through the crate's decoder and the crate's encoder through libhdf5's, which no case did before; the rust arm stores the four cd_values H5Pset_szip stores for i32 chunks of 16 and marks the filter optional as H5Pset_szip does.
…tent grew A completing write that failed left its band buffered while the append returned the error; the next append pushed a fourth frame past the frames_per_chunk == test that triggers the write, and the next write_band indexed the frame past the band's end, a panic that Drop turned into an abort. write_band now writes each complete band whole and drains it, and the band still filling per Tail, so the retry any later write makes is the same chunks again; the append grows the extent before it buffers, so the buffer's first frame is always where the extent says.
finalize_for_swmr writes every dataset header with an extent that counts the buffered frames, and nothing wrote their band until the first flush, so a reader attaching between the two read them as fill; the rule flush and close follow applies here too.
delete_dataset takes the chunk index with the dataset but cannot see the band buffer, which is SwmrWriter's, so every later band write failed on the buffered frames of a dataset that no longer has chunks. The skip is the one flush already applies to a deleted dataset's header and index.
close returned the drain's error before close_in_place, so the writer was never marked closed and Hdf5Writer's Drop finalized it anyway, printing its own message over the error the caller was handling; the close path is now taken whatever the drain did, with both errors reported if both fail.
A map is taken only under the shared lock, so an environment that sets HDF5_USE_FILE_LOCKING=FALSE would have left read_mapped refused.
The H5D__efl_write port summed slot.offset + skip unchecked while 5210a26 checked the same sum on the read side; the file's offset reaches it verbatim through ReopenWalk, so a crafted slot landed bytes at the wrapped offset of the raw data file.
get_bits and get_fs fabricated zero bits past the data, and decode only noticed at a block boundary, so a block that began in the encoder's byte padding decoded the fabricated zeros as a zero-block run and filled a declared output the stream never covered. A read past the end is now StreamEnd; decode_block discards the block it was in, and decode finishes if the samples completed so far reach the declared output, else errors.
…ason H5File::dataset is refused in write mode for any dataset, so the two reopen tests asserted nothing about the mismatch; dataset_writer is the open that reaches the preserved object and names why it was kept.
element_count ran first, so a selection the extent does not admit was refused for its size rather than its bounds; read_slice already ordered the two the other way. The extent is the writer's current one.
io::hyperslab::check_hyperslab and push_box_runs each carried the checked start-plus-count rule; the changelog claimed one helper owned it. The rule now lives in the format layer, which the io layer already depends on, and HyperslabError converts per layer: invalid data for a box a file supplied, an invalid request for a caller's slice.
A share of the map handed to a zero-copy view outlived the handle whose descriptor held the shared lock; on Linux the kernel kept the flock for as long as the mapping referenced the file, on XNU the close releases it, leaving the view's pages open to a truncation. LockedMap pairs the map with a duplicate descriptor so the lock's lifetime is the map's on every platform, and release_lock refuses read-only handles, the one path that could take the lock out from under a live share.
Two targets were wrong: fs_addr is indexed by FreeSpaceManager::message_slot, not FreeSpaceClass, and DatasetBuilder lives at crate::dataset, not the root. The rest linked private items or a test; they are plain code spans now, and the attribute message's byte layout is fenced so its angle brackets stop reading as HTML.
The LockedMap tests are behind the mmap feature and the matrix ran the default features only, so the platforms the lock keeper exists for never ran them: Linux holds a flock while the file stays mapped, XNU and BSD release it on close, and Windows' LockFileEx follows the handle. The LockedMap doc now names the std contract the design rests on.
The doc claimed the builder has no setter for the allocation time, but DatasetBuilder::early_allocation is H5Pset_alloc_time(EARLY), and a chunked dataset the writer lays out whole reads back Early.
CI's stable is rustc 1.98, whose chunks_exact_to_as_chunks lint fails the clippy job on 36 sites, none from this branch. The arrays as_chunks yields let each closure take the chunk as *c instead of rebuilding it. Local stable is 1.97, so the lint was checked with nightly clippy.
physwkim
changed the title
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The upstream window eeba6ab8..44426bfc touches only H5Olayout.c among the C files this crate cites (HDFGroup/hdf5#6508); the rest of the branch is the review that followed, one commit per finding, with the reasons in the commit bodies and the user-visible effects under Unreleased in CHANGELOG.md. Verified with the full workspace under default, mmap and all-features (clippy -D warnings, nextest, doctests) and both oracle variants (A 113/113, B 112 plus vds_late_layout UNSUPPORTED-API by design); the OS matrix now runs the mmap tests, so this PR's CI is the first run of LockedMap's lock keeping on macOS and Windows. Releases 0.5.2 on merge.