Skip to content

Fix NULL pointer dereferences in H5HL_protect and H5HL_unprotect - #6377

Open
tbeu wants to merge 7 commits into
HDFGroup:developfrom
tbeu:fix/H5HL-null-deref
Open

Fix NULL pointer dereferences in H5HL_protect and H5HL_unprotect#6377
tbeu wants to merge 7 commits into
HDFGroup:developfrom
tbeu:fix/H5HL-null-deref

Conversation

@tbeu

@tbeu tbeu commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

When reading corrupted HDF5 files, H5HL_protect can encounter a heap where prfx->heap is NULL or heap->dblk_image is NULL. Similarly, H5HL_unprotect can encounter a heap where heap->prfx or heap->dblk has been set to NULL by H5HL__prfx_dest during cache eviction of corrupted entries.

In both cases the code proceeded to dereference NULL pointers, causing a segfault. This was found by OSS-Fuzz via the matio project's fuzzer.

Changes

  • H5HL_protect: Add NULL checks for prfx->heap and heap->dblk_image after loading the local heap prefix from the cache. Return an error via HGOTO_ERROR instead of crashing.
  • H5HL_unprotect: Replace the assert(heap->dblk) with a proper NULL check and HGOTO_ERROR. Add an equivalent NULL check for heap->prfx in the single_cache_obj branch.

ASAN report (from OSS-Fuzz)

https://issues.oss-fuzz.com/504827191

=================================================================
==250==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x57025b1b892d bp 0x7ffea4182170 sp 0x7ffea4182150 T0)
==250==The signal is caused by a READ memory access.
==250==Hint: address points to the zero page.
# 0 0x57025b1b892d in H5AC_unpin_entry hdf5/src/H5AC.c:1438:28
# 1 0x57025b51aed5 in H5HL_unprotect hdf5/src/H5HL.c:436:25
# 2 0x57025b49f6d3 in H5G__stab_lookup hdf5/src/H5Gstab.c:832:17
# 3 0x57025b496916 in H5G__obj_lookup hdf5/src/H5Gobj.c:1086:13
# 4 0x57025b4a356b in H5G__traverse_real hdf5/src/H5Gtraverse.c:572:13
# 5 0x57025b4a26e9 in H5G_traverse hdf5/src/H5Gtraverse.c:845:9
# 6 0x57025b47ab2e in H5G_loc_find hdf5/src/H5Gloc.c:423:9
# 7 0x57025b2ae07d in H5D__open_name hdf5/src/H5Dint.c:1474:9
# 8 0x57025bd53dd3 in H5VL__native_dataset_open hdf5/src/H5VLnative_dataset.c:331:25
# 9 0x57025bd07a64 in H5VL__dataset_open hdf5/src/H5VLcallback.c:2053:25
# 10 0x57025bd074a2 in H5VL_dataset_open hdf5/src/H5VLcallback.c:2088:30
# 11 0x57025b27e0c5 in H5D__open_api_common hdf5/src/H5D.c:361:25
# 12 0x57025b27da7e in H5Dopen2 hdf5/src/H5D.c:400:22

@bmribler

bmribler commented Apr 26, 2026

Copy link
Copy Markdown
Collaborator

@tbeu Thank you for the PR. Please see my comment in PR #6378. Similar to that PR, we need to investigate why/where these values are NULL and flag the errors there, in addition to these checks.

@tbeu

tbeu commented Apr 26, 2026

Copy link
Copy Markdown
Contributor Author

The issue occurs with corrupted HDF5 files on fuzzing. It is not possible to detect / prevent the error outside hdf5, i.e. as consumer of hdf5 lib.

@tbeu
tbeu force-pushed the fix/H5HL-null-deref branch from 29146df to 59ff4be Compare April 28, 2026 18:39
@tbeu
tbeu force-pushed the fix/H5HL-null-deref branch 3 times, most recently from 929f097 to 4b735bd Compare May 7, 2026 19:16
@tbeu
tbeu force-pushed the fix/H5HL-null-deref branch 2 times, most recently from 5fecc19 to 60538fa Compare May 18, 2026 21:15
@tbeu
tbeu force-pushed the fix/H5HL-null-deref branch 2 times, most recently from 5aad41a to 12c3822 Compare June 10, 2026 17:03
@github-actions

github-actions Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Review Checklist

This PR touches the following areas. Each needs a sign-off
from its listed owners before merging.

  • src
  • test
  • *.cmake
    • @mattjala (manually added) — approval required
  • release_docs@hyoklee

Additional reviewers (not owners of a touched area): @derobins, @qkoziol

@lrknox
lrknox removed their request for review June 11, 2026 21:20
@tbeu
tbeu force-pushed the fix/H5HL-null-deref branch from 12c3822 to a0db16d Compare June 16, 2026 18:01
@github-actions github-actions Bot added the stale label Jul 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

This pull request has had no activity for 30 days and has been marked stale. Push a commit or comment to keep it open, or it will be flagged for maintainer review.

@bmribler

Copy link
Copy Markdown
Collaborator

Hi @tbeu, how would you like to proceed with this PR?

Comment thread src/H5HL.c Outdated

@mattjala mattjala left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to my other comments, this should include a regression test case using the file from the OSS report and a CHANGELOG.md entry describing the problem that was fixed.

tbeu added 3 commits August 16, 2026 08:43
When reading corrupted HDF5 files, H5HL_protect can encounter a heap
where prfx->heap is NULL or heap->dblk_image is NULL. Similarly,
H5HL_unprotect can encounter a heap where heap->prfx or heap->dblk has
been set to NULL by H5HL__prfx_dest during cache eviction of corrupted
entries. In both cases, the code proceeded to dereference NULL pointers,
causing a segfault (found by OSS-Fuzz via matio).

Replace the assert in H5HL_unprotect with proper NULL checks and
HGOTO_ERROR, and add equivalent guards in H5HL_protect, so corrupted
files are rejected gracefully instead of crashing.
…sion test + changelog

- Remove the NULL checks in H5HL_protect for prfx->heap and heap->dblk_image.
  prfx->heap is set in H5HL__prfx_new and only cleared in H5HL__prfx_dest (which
  frees the prefix in the same call), so the check is unreachable and could leak
  prfx via the done block. heap->dblk_image is NULL by design whenever the data
  block is not contiguous with the prefix, so that check was a false positive.
- Keep the H5HL_unprotect NULL checks for heap->prfx and heap->dblk, which are the
  actual fix for the reported crash (OSS-Fuzz 504827191).
- Add a regression test (corrupt_heap_unprotect) in test/lheap.c using the
  minimized fuzzer file heap_corrupt_prfx.h5, asserting the corrupted file fails
  gracefully instead of crashing in H5HL_unprotect.
- Document the fix in release_docs/CHANGELOG.md.
Replace the file-based traversal (which does not deterministically reach
the cache-eviction path that nulls heap->prfx) with a white-box check that
protects a local heap, simulates the corrupted/evicted condition by nulling
the heap's prfx/dblk pointers, and verifies H5HL_unprotect() returns an
error instead of dereferencing NULL. Keep a best-effort open/traverse of the
OSS-Fuzz minimized file (issue 504827191) as a smoke test.

Verified: without the fix the test aborts (H5AC_unpin_entry(NULL)); with the
fix it passes.
@tbeu
tbeu force-pushed the fix/H5HL-null-deref branch from 05ee0fa to ecdc3cb Compare August 16, 2026 07:26
@tbeu
tbeu requested a review from hyoklee as a code owner August 16, 2026 07:26
@tbeu
tbeu requested a review from mattjala August 16, 2026 07:26
@tbeu

tbeu commented Aug 16, 2026

Copy link
Copy Markdown
Contributor Author

Review remarks are addressed.

@tbeu

tbeu commented Aug 17, 2026

Copy link
Copy Markdown
Contributor Author

https://issues.oss-fuzz.com/504827191

It has been published meanwhile.

Comment thread test/lheap.c Outdated
Comment thread test/lheap.c Outdated
Comment thread test/lheap.c
Comment thread test/lheap.c Outdated
Comment thread test/lheap.c Outdated
Comment thread test/lheap.c Outdated
Comment thread test/lheap.c Outdated
Comment thread test/lheap.c Outdated
Comment thread test/lheap.c Outdated
@github-project-automation github-project-automation Bot moved this from To be triaged to In progress in HDF5 - TRIAGE & TRACK Aug 17, 2026
@tbeu
tbeu requested a review from mattjala August 17, 2026 18:01
Comment thread test/lheap.c Outdated
Comment thread test/lheap.c Outdated
- Use H5L_info2_t in visitor (matches H5L_iterate2_t signature)
- Use H5O_info2_t with H5Oget_info_by_name3
- Replicate matio's exact traversal: H5Literate2 + H5Gopen2/H5Dopen2 +
  H5Lvisit2 recursion, H5Aiterate2 attribute reads, skip #refs#/#subsystem#
- Keep white-box deterministic NULL-pointer guard (fuzzer file does not
  deterministically crash via public API alone; original crash required
  matio fuzzer's specific cache-pressure environment)
- Add CORRUPT_HEAP_TESTFILE define back for white-box test
@tbeu
tbeu requested a review from mattjala August 18, 2026 18:01
@tbeu
tbeu force-pushed the fix/H5HL-null-deref branch from 1c479d2 to cd5a632 Compare August 19, 2026 04:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-decision Stale past the alert threshold -- needs a maintainer decision to keep open or close

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants