Fix h5ls SIGSEGVs in Nbit and Fletcher32 filter decode paths#6497
Fix h5ls SIGSEGVs in Nbit and Fletcher32 filter decode paths#6497mattjala wants to merge 6 commits into
Conversation
This PR fixes three related issues which could cause crashes when crafted files were provided to h5ls. As it happens, all three are reachable from any reader via H5Z_pipeline() during a chunked H5Dread(), so the root issues affect the library itself. 1. Nbit NULL/short parameter array (HDFGroup#6489, "Null_Pointer") H5Z__filter_nbit() dereferenced cd_values[0] before validating the parameter array. A file storing the filter with zero client-data values produced cd_values=NULL / cd_nelmts=0 and a NULL dereference. Fix is to reject a NULL cd_values or a cd_nelmts < 5 (the header at indices 0..4 is always read) before any access. 2. Nbit compressed-input buffer over-read (HDFGroup#6489, "Heap_Corruption_1") The H5Z__nbit_decompress* family walked the compressed input via index *j with no bound against the input size, and nbytes was never even passed in. Malformed parameters (e.g. d_nelmts=67108876 over a 31-byte chunk) drove *j past the end, reading out of bounds in H5Z__nbit_decompress_one_byte(). Fix is thread the input size through the whole decompress call chain, convert the void helpers (_one_byte, _one_nooptype, _one_atomic) to return herr_t, and bounds-check *j against buffer_size before every buffer[*j] read, propagating a filter error up the stack. 3. Fletcher32 checksum length underflow (HDFGroup#6490, "Heap_Corruption_2") The read path computed `src_nbytes = nbytes - FLETCHER_LEN` with no lower-bound check. A corrupt 0-byte chunk underflowed src_nbytes to SIZE_MAX-3, which was passed straight to H5_checksum_fletcher32(), causing an out-of-bounds read. Fix is to reject nbytes<FLETCHER_LEN up front, since a valid Fletcher32 buffer must contain at least the trailing 4-byte checksum. All three now fail gracefully with an error instead of crashing. Fixes HDFGroup#6489 Fixes HDFGroup#6490
Added regression tests for the core library to guard against the patched issues. These files were produced with the new generator test/gen_bad_filters.c, which writes each dataset via the public API (using earliest library version bounds so the pipeline message lands in a version-1, un-checksummed object header) and then patches the specific bytes.
Review ChecklistThis PR touches the following areas. Each needs a sign-off
|
There was a problem hiding this comment.
Pull request overview
This PR hardens the HDF5 filter decode/read paths against crafted-file crashes by adding parameter/length validation to the N-Bit and Fletcher32 filters, and introduces regression tests (plus a generator) to ensure these malformed filter metadata cases fail gracefully instead of SIGSEGV/OOB reads.
Changes:
- Add early validation in the N-Bit decode path and thread compressed-buffer size through the N-Bit decompression helpers to prevent out-of-bounds reads.
- Add a minimum-length guard in the Fletcher32 read path to prevent checksum-length underflow.
- Add regression tests and a generator for malformed-filter test files, and register the new reference
.h5files in the test CMake setup.
Reviewed changes
Copilot reviewed 5 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/H5Znbit.c |
Adds N-Bit parameter validation and bounds-checking during decompression to prevent crashes/OOB reads. |
src/H5Zfletcher32.c |
Rejects too-short Fletcher32 buffers to avoid size_t underflow and OOB reads. |
test/dsets.c |
Adds a regression test that verifies reads fail cleanly for crafted bad-filter metadata files. |
test/gen_bad_filters.c |
Adds a generator to create/patch malformed filter metadata files used by the regression test. |
test/CMakeTests.cmake |
Registers the new bad-filter .h5 reference files and adds the generator target. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@mattjala This should probably have a CHANGELOG.md entry. |
|
This same set of changes also fixes #6488, since the root cause in both causes in the fletcher32 checksum-length underflow |
Fixes two crafted-file crashes in h5dump (GitHub issues HDFGroup#6486 and HDFGroup#6487). A third report (HDFGroup#6488) is the Fletcher32 checksum-length underflow already fixed for h5ls in PR HDFGroup#6497, so it is not addressed again here. Issue HDFGroup#6486 (tools) - render_bin_output() SIGSEGV with "-b" on a variable-length string dataset. The H5T_STRING branch reused the local "size" variable both as the per-element stride and as the string length returned by strlen(), so after the first element the stride became that string's length and subsequent elements were read from misaligned addresses, dereferencing a garbage pointer. This affected any multi-element vlen-string dataset, not just crafted files. A separate str_len variable is now used for the write length so "size" stays the datatype stride. As this is a tools bug, the regression test is a tools test: tools/test/h5dump adds the tbinvlstr case (binary dump of a vlen-string dataset, file tbinvlstr.h5, generated by h5dumpgentest.c/gent_binvlstr), which crashes on develop and passes with the fix. Issue HDFGroup#6487 (library) - H5Pget_fill_value() SIGSEGV via H5T_path_find(). A crafted version-1/2 fill value message can have "fill defined" set with a negative size field; H5O_fill_old_decode() leaves fill.size negative and fill.buf/type NULL instead of normalizing to the "undefined" sentinel. That value is neither 0 nor -1, so H5P_get_fill_value() fell through to H5T_path_find() with a NULL source type and crashed. H5P_get_fill_value() now rejects a NULL fill datatype and returns an error, keeping the dataset itself readable. h5dump's XML fill-value renderer (xml_dump_fill_value) is also hardened: it checks the buffer allocation and the H5Pget_fill_value() return value and emits an empty fill value element instead of dereferencing an unallocated or unretrievable buffer. Regression test (library): test/fillval.c test_bad_fill_value() reads a crafted file (generated by test/gen_bad_fill.c, committed as test/testfiles/bad_fill_value.h5) and verifies that the dataset data is still readable while H5Pget_fill_value() fails cleanly instead of crashing. Verified: both PoCs exit cleanly with the fix (SIGSEGV without it); the committed fixtures crash the pre-fix library/tools and pass post-fix; the fillval suite, the new tbinvlstr h5dump test, and the non-XML h5dump suite pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This PR fixes three related issues which could cause crashes when
crafted files were provided to h5ls. As it happens, all three are reachable from
any reader via H5Z_pipeline() during a chunked H5Dread(), so the root issues
affect the library itself.
Nbit NULL/short parameter array (h5ls: SIGSEGV in Nbit filter processing when dumping datasets #6489, "Null_Pointer")
H5Z__filter_nbit() dereferenced cd_values[0] before validating the
parameter array. A file storing the filter with zero client-data
values produced cd_values=NULL / cd_nelmts=0 and a NULL dereference.
Fix is to reject a NULL cd_values or a cd_nelmts < 5 (the header at indices
0..4 is always read) before any access.
Nbit compressed-input buffer over-read (h5ls: SIGSEGV in Nbit filter processing when dumping datasets #6489, "Heap_Corruption_1")
The H5Z__nbit_decompress* family walked the compressed input via
index *j with no bound against the input size, and nbytes was never
even passed in. Malformed parameters (e.g. d_nelmts=67108876 over a
31-byte chunk) drove *j past the end, reading out of bounds in
H5Z__nbit_decompress_one_byte().
Fix is thread the input size through the whole decompress call chain,
convert the void helpers (_one_byte, _one_nooptype, _one_atomic)
to return herr_t, and bounds-check *j against buffer_size before every
buffer[*j] read, propagating a filter error up the stack.
Fletcher32 checksum length underflow (h5ls: SIGSEGV in H5_checksum_fletcher32 due to underflowed checksum length #6490, "Heap_Corruption_2")
The read path computed
src_nbytes = nbytes - FLETCHER_LENwith nolower-bound check. A corrupt 0-byte chunk underflowed src_nbytes to
SIZE_MAX-3, which was passed straight to H5_checksum_fletcher32(),
causing an out-of-bounds read.
Fix is to reject nbytes<FLETCHER_LEN up
front, since a valid Fletcher32 buffer must contain at least the
trailing 4-byte checksum.
All three now fail gracefully with an error instead of crashing.
Also adds regression tests for the core library to guard against
the patched issues. The files used in the tests were produced with the new generator test/gen_bad_filters.c,
which writes each dataset via the public API (using earliest library
version bounds so the pipeline message lands in a version-1,
un-checksummed object header) and then patches the specific bytes.
Fixes #6489
Fixes #6490
Fixes #6488
Fixes #6492