Fix h5dump SIGSEGVs on vlen binary output and malformed fill value#6507
Draft
mattjala wants to merge 1 commit into
Draft
Fix h5dump SIGSEGVs on vlen binary output and malformed fill value#6507mattjala wants to merge 1 commit into
mattjala wants to merge 1 commit into
Conversation
mattjala
requested review from
bmribler,
fortnern,
glennsong09,
hyoklee,
jhendersonHDF,
lrknox and
vchoi-hdfgroup
as code owners
mattjala
added
Component - C Library
mattjala
added
the
Type - Security
There was a problem hiding this comment.
Pull request overview
Fixes two crafted-file crash paths affecting h5dump output and fill-value handling, and adds regression coverage to prevent reintroducing the issues.
Changes:
- Fix
h5dump -bcrashes on multi-element variable-length string datasets by separating per-element stride (size) from per-string write length. - Harden
H5P_get_fill_value()against malformed fill values that lack a datatype, preventing NULL-type dereferences during type conversion. - Add regression tests and generators for both issues (binary vlen-string dump and malformed fill-value message), plus changelog entries.
Reviewed changes
Copilot reviewed 12 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/test/h5gentest.c | Adds generation of the new tbinvlstr h5dump test file. |
| tools/test/h5dump/h5dumpgentest.h | Declares the new gent_binvlstr() generator. |
| tools/test/h5dump/h5dumpgentest.c | Implements gent_binvlstr() to generate a multi-element VL-string dataset for binary-output testing. |
| tools/test/h5dump/exportfiles/tbinvlstr.exp | Adds expected output for binary dump of the VL-string dataset. |
| tools/test/h5dump/CMakeTests.cmake | Registers the new reference files and adds the tbinvlstr binary-output test. |
| tools/src/h5dump/h5dump_xml.c | Switches fill-value buffer allocation to zero-initialized allocation for safer rendering on failures. |
| tools/lib/h5tools.c | Fixes vlen-string binary output stride/length handling to prevent misaligned reads and crashes. |
| test/gen_bad_fill.c | Adds a generator to create a file with a deliberately malformed fill-value message for regression testing. |
| test/fillval.c | Adds test_bad_fill_value() to ensure malformed fill values fail safely while raw dataset reads still work. |
| test/CMakeTests.cmake | Adds bad_fill_value.h5 to copied reference test files and gen_bad_fill to generator executables. |
| src/H5Pdcpl.c | Rejects fill values with missing datatype to prevent NULL-type dereference during conversion. |
| release_docs/CHANGELOG.md | Documents both crash fixes in the changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
Bot
removed request for
fortnern,
glennsong09,
hyoklee and
jhendersonHDF
Contributor
Review ChecklistThis PR touches the following areas. Each needs a sign-off
|
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>
mattjala
force-pushed
the
h5dump_malformed_crashes
branch
from
1b8518a to
81e489c
Compare
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
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.
This PR fixes two crafted-file crashes in h5dump.
The H5T_STRING branch reused the local "size" variable as both 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. A separate str_len variable is now used for the write length so "size" stays the datatype stride.
The regression test for this change is tbinvlstr in tools/test/h5dump, which attempts to binary dump a vlen-string dataset.
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 isn't 0 or -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 additionally zero- initializes its buffer so a failed read can't feed uninitialized memory to the reference-rendering path.
The regression test for this change is test_bad_fill_value() within test/fillval.c.
Fixes #6486
Fixes #6487