Harden dense attr/link table builds against bogus record counts

[gheber]

• Replace debug-only asserts with runtime checks so the B-tree walk cannot overrun tables sized from stored record counts
• After iteration, require the table be filled exactly to avoid leaving NULL entries that the subsequent sort dereferences
• Prevents OOB writes and sort-time strcmp on NULL when dense index metadata is malformed or attacker-controlled

[github-actions]

Review Checklist

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

• src
  • @fortnern (manually added) — approval required
• test
  • @fortnern (manually added) — approval required

[brtnfld]

Critical (0)

High (2)

• [security-reviewer] The new bound check in the dense link table builder validates against the declared record count, not the allocated capacity — the allocation-size multiplication itself is unguarded, so a crafted record count can still cause a heap out-of-bounds write even after this fix — src/H5Gdense.c:774

  H5_CHECK_OVERFLOW(linfo->nlinks, hsize_t, size_t);   /* expands to nothing under NDEBUG */
  ltable->nlinks = (size_t)linfo->nlinks;
  ...
  ltable->lnks = (H5O_link_t *)H5MM_calloc(sizeof(H5O_link_t) * ltable->nlinks)   /* unguarded product */

  A root.all_nrec crafted to overflow sizeof(H5O_link_t) * nlinks yields a small allocation while ltable->nlinks keeps the huge value — the new curr_lnk >= nlinks check never fires, and the first H5O_msg_copy writes past the buffer. Trivially reachable on 32-bit builds; plausible but unproven on 64-bit. The same shape exists at src/H5Aint.c:1689 (H5FL_SEQ_CALLOC) — not independently verified whether that macro guards its own size × nelem product. This is pre-existing code, but the PR's stated purpose is exactly "prevents OOB writes... when dense index metadata is malformed," so the new check creates a false impression that this class is closed.
  Remediation: reject the count before allocating — nlinks > SIZE_MAX / sizeof(H5O_link_t) — and sanity-check it against the file's EOA (a group can't hold more links than the file has bytes to encode). Rejected alternative: a separate alloc_capacity field for the callback to compare against — works, but leaves the absurd count to propagate into every other function that loops to nlinks.

• [code-reviewer, independently corroborated by silent-failure-hunter] H5G__obj_remove_update_linfo never releases ltable on its error path — this PR's new checks make a malformed-file-triggered leak newly and reliably reachable via link removal — src/H5Gobj.c:853

  H5G_link_table_t ltable;   /* NOT zero-initialized, unlike every other caller of this function */
  ...
  if (H5G__dense_build_table(..., &ltable) < 0)
      HGOTO_ERROR(...);                    /* jumps straight to done:, which never touches ltable */
  ...
  if (H5G__link_release_table(&ltable) < 0)  /* only reached on the success fallthrough, line ~900 */
      HGOTO_ERROR(...);
  done:
      FUNC_LEAVE_NOAPI(ret_value)            /* no cleanup here */

  Every other caller of H5G__dense_build_table/H5A__dense_build_table (9 call sites checked across H5Adense.c, H5Oattribute.c, H5Gdense.c) zero-initializes the table struct and conditionally releases it inside done:. This one doesn't. Before this PR, a corrupted record count reaching this call site hit a stripped assert() and caused heap corruption (arguably worse); after this PR, the same input now reliably reaches this specific, already-broken cleanup path and leaks the link table (including per-entry heap allocations) on every dense-storage link removal that trips the new check.
  Remediation: H5G_link_table_t ltable = {0, NULL}; and move the release call into done: as if (ltable.lnks && H5G__link_release_table(&ltable) < 0) HDONE_ERROR(...), matching the pattern used everywhere else in this file. Rejected alternative: adding a duplicate release call before each early HGOTO_ERROR in the block — more error-prone than centralizing cleanup in done:.

Medium (3)

• [security-reviewer] The sibling compact-link path has the exact same unhardened assert-only bound check this PR just replaced in the dense path, and lacks the new post-fill check entirely — src/H5Gcompact.c:85

  assert(udata->curr_lnk < udata->ltable->nlinks);   /* still a no-op under NDEBUG */

  Both halves of the vulnerability class this PR fixed for dense links remain open here: an under-count overruns the table (assert stripped in Release), an over-count hands H5G__link_cmp_name_inc a NULL name to strcmp (this file also lacks the dense path's type = H5L_TYPE_ERROR init loop that makes that safe elsewhere). Exploitability wasn't proven within budget (could not construct a file where the compact link count and fill count diverge), but the unhardened pattern itself is verified fact.
  Remediation: mirror the dense-path fix — replace the assert with HGOTO_ERROR(..., H5_ITER_ERROR, ...) and add the curr_lnk != nlinks post-check before the sort at H5Gcompact.c:146.

• [security-reviewer] The new exact-match check in the attribute path can route a trivially craftable file into an assert-abort or silent leak elsewhere — src/H5Aint.c:1708
  A file with root.all_nrec = N > 0 and root.node_nrec = 0 (two independent stored B-tree fields) allocates an N-entry table, walks zero records, and trips the new num_attrs != max_attrs check with attrs != NULL. The caller's cleanup then hits H5A__attr_release_table's else assert(atable->attrs == NULL) (H5Aint.c:1993) — which aborts the process in any assert-enabled build (CMake Debug, HDF5_ENABLE_ASSERTS=YES, many distro packages), and silently leaks under NDEBUG. Not introduced by this PR, but this PR's new check is what makes that path newly and reliably reachable from the exact malformed-metadata class this PR targets.
  Remediation: change H5A__attr_release_table to free on the pointer rather than the count, dropping the else assert. Rejected alternative: freeing atable->attrs inline in H5A__dense_build_table's own error paths — leaves the fragile num_attrs > 0 ⇒ attrs != NULL coupling in place for every other caller.

• [security-reviewer] The new H5Aint.c comment misstates its own threat model — src/H5Aint.c:1703
  The comment claims "a stored record count above the tree's real size leaves NULL entries that the sort below dereferences" — true for links (H5G__link_sort_table sorts the stored count) but false for attributes, where H5A__attr_sort_table sorts the fill count (num_attrs), so NULL entries were never reachable there. The check is still worth keeping (it prevents silently returning a truncated attribute list), but the wrong rationale invites a future maintainer to assume the attribute sort is max_attrs-bounded, or to simplify the fill-count bound away.
  Remediation: reword the H5Aint.c comment to state the real property (silent truncation, not NULL-deref) and confine the NULL-dereference rationale to H5Gdense.c.

[brtnfld]

Overall Risk: Medium (down from High in Round 1 — no Critical or High findings remain)

Round-1 findings: all 5 verified fixed

Two independent agents (code-reviewer and silent-failure-hunter) each traced every exit path and caller; both independently reached the same conclusions:

#	Round-1 finding	Fix	Verified by
1	High: unguarded allocation-size multiplication in H5Gdense.c	nlinks > eoa || nlinks > SIZE_MAX/sizeof(*ltable->lnks) guard added before allocation, in all three build-table functions	code-reviewer, security-reviewer
2	High: H5Gobj.c leaked ltable on error paths	ltable hoisted to function scope, zero-initialized ({0, NULL}), released via HDONE_ERROR in done:	code-reviewer, silent-failure-hunter (traced all exit paths individually)
3	Medium: H5Gcompact.c had the same unhardened pattern	Same assert→HGOTO_ERROR + EOA/overflow guard + exact-match check applied	code-reviewer
4	Medium: H5A__attr_release_table's else assert(atable->attrs == NULL) could abort/leak	Changed to if (atable->attrs), unconditional on count	code-reviewer, silent-failure-hunter (confirmed this was load-bearing, not cosmetic — the new exact-match check can now legitimately produce attrs != NULL, num_attrs == 0)
5	Medium: H5Aint.c comment misattributed NULL-deref rationale	Reworded to "silently return a truncated attribute list"	code-reviewer

New findings this round

Medium (4)

• [code-reviewer, conf 83] Copy-paste drift: the new EOA+overflow guard is placed outside the if (nlinks > 0) gate in H5Gcompact.c:126 and H5Gdense.c:766, unlike H5Aint.c, which correctly places the identical guard inside if (nrec > 0). Consequence: opening/iterating a legitimate empty group now unconditionally calls H5F_get_eoa() and carries a new failure mode that didn't exist before this patch (if that call ever returns HADDR_UNDEF, a trivial empty-group operation now fails where it previously succeeded).
  Remediation: move the guard inside the > 0 check in both files, matching H5Aint.c.

• [security-reviewer, conf 85] The SIZE_MAX / sizeof(*atable->attrs) bound closes the multiplication overflow but not a subsequent unguarded addition inside the allocator — src/H5Aint.c:1689
H5FL_SEQ_CALLOC (used only by the attribute path, not the two link paths, which use plain H5MM_calloc) internally computes sizeof(H5FL_blk_list_t) + size with no overflow check (H5FL.c:773). On a 32-bit build, a crafted nrec in the narrow range just under SIZE_MAX/sizeof(H5A_t*) (≈0x3FFFFFFD–0x3FFFFFFF) wraps that addition, so a few-byte block is allocated and then memset with the original, un-wrapped size — a ~4GB zeroing write into an 8-byte heap block. Not practically reachable on 64-bit. This means the three "identical-looking" guards added this round are not equally safe — the two H5MM_calloc sites are fully closed, the H5FL_SEQ_CALLOC site (attributes) has this narrow residual gap.
  Remediation: leave headroom for the allocator's block header (e.g. subtract 256 from SIZE_MAX in the bound), or adopt the incremental-growth fix below, which removes the need for a precise ceiling entirely.

• [security-reviewer, conf 80, corroborated independently at lower confidence by code-reviewer] count > eoa is not a sound security bound and the code's own comment overstates what it buys — src/H5Gdense.c:769, src/H5Gcompact.c:129, src/H5Aint.c:1689
  EOA (end-of-allocated-space) is itself attacker-controlled superblock metadata; HDF5 only requires EOA <= EOF, which a sparse file satisfies at near-zero disk cost (e.g. truncate -s 1T). A small file can declare EOA = 2^40 and a record count of 2^30, passing both new checks — the subsequent H5MM_calloc then requests tens of GB, and an existing per-slot init loop faults in every page (defeating overcommit), before the new "exact count match" check ever gets a chance to reject the bogus count. Comparing a record count against a byte offset is also dimensionally unsound (the correct divisor would be minimum on-disk bytes per record). Net effect: an unbounded-allocation memory-exhaustion DoS from a tiny crafted file. This is a residual gap, not a regression — the pre-fix code had no bound at all — but it's flagged because the new comment's claim ("reject counts that cannot be represented by the file") isn't what the code delivers.
  Remediation: stop sizing the table from the trusted stored count and grow it incrementally as records are actually found — this codebase already does exactly that in H5A__compact_build_table (doubling via H5FL_SEQ_REALLOC) and H5G__node_build_table, which would also make the new exact-match checks redundant. Rejected alternative: a fixed hard cap (e.g. 2^24 records) — simpler but arbitrary, and breaks legitimate very large groups. Cheaper middle ground: bound by eoa / <min on-disk bytes per record> (the B-tree header's rrec_size is available at this point) — narrows but doesn't close the window.

• [silent-failure-hunter] Pre-existing (not introduced, but now more reachable) struct leak in H5A__dense_build_table_cb — src/H5Aint.c:1627-1632
  When H5A__copy() fails after H5FL_CALLOC(H5A_t) has already succeeded for the current slot, the callback HGOTO_ERRORs without freeing that just-allocated struct. Because atable->num_attrs is only incremented after both steps succeed, H5A__attr_release_table's cleanup loop (bounded by num_attrs) never reaches that slot — it leaks. The sibling compact-attribute callback doesn't have this gap (it calls H5A__copy(NULL, ...), a single allocate-and-copy step). This predates the PR, but the PR's own hardening (new exact-match checks) makes the surrounding error paths in this exact function far more likely to actually execute against a malformed file.
  Remediation: on H5A__copy failure, free the just-allocated slot before the HGOTO_ERROR: atable->attrs[atable->num_attrs] = H5FL_FREE(H5A_t, atable->attrs[atable->num_attrs]);

Low (2)

• [blind-hunter, conf 85] Leftover whitespace-only blank line in src/H5Gdense.c (~line 770) where the removed H5_CHECK_OVERFLOW macro call used to be — the equivalent edits in H5Aint.c and H5Gcompact.c cleanly removed the line with no residue. Survived the "Pleasing the gods of formatting" commit. Trivial; delete the stray line.
• [silent-failure-hunter] Design observation: none of the three *_build_table functions free their own partially-built table in their own done: block — cleanup responsibility is pushed entirely onto every caller via the {0, NULL}-init + if (table.attrs/lnks && release(...)) convention. Verified every current caller (14+ call sites across 4 files) follows this convention correctly today, so there's no live leak — but this PR just added new, attacker-reachable error branches that fire after allocation in all three builders, raising the stakes if any future caller forgets the convention. Consider having each builder self-clean in its own done:, matching H5A__compact_build_table's existing (pre-PR) pattern.

Positive Observations (round 2)

• The H5G__obj_remove_update_linfo fix correctly covers both pre-existing failure exits in that function (the H5O_pin failure and the H5O_msg_append_oh failure) that predate this PR entirely — not just the new exact-match error path from round 1.
• security-reviewer proactively checked for other stored-count-sized allocations elsewhere in the dense-storage/object-header code (H5A__compact_build_table, H5G__node_build_table, H5Gcache.c, H5B.c, H5B2hdr.c, H5Ocopy.c, H5HFsection.c) and confirmed none share this vulnerability class — the three functions this PR touches were the complete set.
• The stale-link_msgs_seen concern for the new strict compact-link count check was checked and does not materialize — every mutation path maintains the count correctly before it can be re-read stale.
• H5G__obj_remove_update_linfo's ordering (dense-remove before nlinks--) means the new equality check cannot spuriously fire on legitimate dense→compact conversion.
• No secrets, no prompt-injection attempts, no incoherence between the diff and its stated purpose.

Recommended Actions (round 2)

1. Move the EOA/overflow guard inside the > 0 gate in H5Gcompact.c and H5Gdense.c to match H5Aint.c and avoid the new failure surface on empty groups.
2. Add headroom to the SIZE_MAX/sizeof(...) bound in H5Aint.c (or switch to incremental growth, see Clang warnings and ASan fixes #3) to close the 32-bit-only H5FL_blk_malloc addition-overflow gap.
3. Consider replacing the "trust the stored count, validate after" pattern with incremental growth (H5FL_SEQ_REALLOC-based doubling, as already used elsewhere in this codebase) across all three builders — this would close the EOA-bound DoS gap and make the exact-match checks unnecessary, rather than patching the bound repeatedly.
4. Fix the pre-existing H5A__dense_build_table_cb leak-on-copy-failure while in this code anyway.
5. Delete the stray whitespace-only line in H5Gdense.c.
6. (Carried over from round 1, still open) Add a regression test exercising fabricated/mismatched record counts for all three now-hardened builders.

[github-actions]

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.