* CI: prune CODEOWNERS avalanche on non-draft PR open
GitHub auto-assigns every touched-area CODEOWNER when a non-draft PR
opens. The previous additive-only logic saw all areas already covered
and returned the full auto-assigned set as confirmedRequested, so the
checklist @-mentioned every owner before the final reviewer set was
known.
On opened/reopened (non-draft), now behave like the draft case: prune
to a single load-balanced pick per area via removeUnselected before
posting the checklist, requesting any pick not already on the PR.
* CI: guard reviewer-exclusion update against bot-triggered removals
The bot's own CODEOWNERS cleanup and draft-handling calls fire
review_request_removed events that self-trigger another workflow run.
Without this guard, that run would interpret the bot's own removal as a
deliberate human decision and permanently add the login to the exclusion
set, blocking that owner from ever being auto-assigned to the PR again.
Add PullRequest inline fragment to the project items GraphQL query
so PRs in the GitHub Project are counted alongside issues when
calculating release blocker/must-do progress.
review-checklist.js previously tried to enforce a single load-balanced
reviewer per area by actively stripping anyone else who showed up, using a
checklist-comment-existence heuristic (isOpeningRace) to decide when it was
safe to do so. Walking through real PR timelines surfaced several cases
where that heuristic and that stripping behavior did the wrong thing:
- An old PR's first encounter with the bot (or one whose checklist comment
was deleted) was misdiagnosed as a brand-new PR, forcibly reselecting a
human-curated reviewer list.
- A CODEOWNER for areas this PR doesn't touch (e.g. a project lead added by
hand for their judgment) was swept by the same logic as junk
auto-assignment, with no protection for an existing approval.
- A reviewer who isn't an owner of any touched area was invisible in the
checklist and untouched by any cleanup path either way.
- A PR that picked up reviewers while non-draft, then was converted to
draft, had those reviewers wiped out by the next push — converted_to_draft
isn't in this workflow's trigger list, so there's no event at the actual
transition to distinguish "noise from this PR's creation" from "a real
assignment from before it became a draft."
- A reviewer removed via the PR UI could be silently re-added by GitHub's
own CODEOWNERS engine on a later push, since there was no memory of the
removal across runs.
Redesign around one principle: the bot should never automatically strip a
reviewer who's already on the PR, manually added or auto-assigned, except
where the policy explicitly calls for it. Concretely:
- coordinateReviewers is now purely additive for non-draft PRs: chooseReviewers
runs against the *real* existingRequested, so it naturally skips any area
that already has someone and only fills gaps. This is idempotent and safe
on every event, eliminating the need for the old race-detection heuristic
entirely (isOpeningRace, the new-PR/synchronize distinction, and the
post-request 15s delayed re-clear are all removed as dead weight).
- Pruning — used only where the bot still clears reviewers — is scoped to
touchedAreaOwners (owners of areas this PR touches, plus catch-all "*"
owners) instead of the repo-wide allCodeOwners, so a CODEOWNER for
unrelated areas is never touched.
- Draft handling clears auto-assigned reviewers only at the literal
opened/reopened-as-draft moment — the one point where "CODEOWNERS noise
from this PR's creation" and "this PR's actual reviewer state" are
mechanically the same thing. Every other event while draft leaves existing
reviewers alone.
- An explicit review_request_removed persists that login to an exclusion
list embedded as a second hidden marker in the checklist comment (the only
durable storage available), and every run strips anyone in that list who
reappears — covering GitHub re-assigning them on a later push. A direct
review_requested for that exact login overrides the exclusion, since it's
the strongest available signal that someone individually decided that
person should be back right now.
- buildBody adds a catch-all "Additional reviewers" line for anyone
requested who isn't an owner of any touched area, so they're visible and
their approval is shown (informationally — it doesn't gate any area's
sign-off), and accepts any owner's approval for an area's sign-off rather
than only the specifically-assigned one's.
12 new tests cover the buildBody changes and the parseExcluded/serializeExcluded
round-trip; all existing pure-function tests are unchanged.
Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org>
* Add stale PR/issue policy with assignee alerts instead of auto-close
Ready PRs/issues use actions/stale to label inactivity (30/60 days).
Draft PRs get a longer 90-day window and only reset on an explicit
"still working on this" comment, since pushes/CI activity alone
shouldn't make an abandoned draft look fresh. Nothing is auto-closed:
once a staleness label has persisted past its alert threshold, a
custom script pings the assignee (falling back to requested
reviewers, then the author) to decide whether to keep it open or
close it.
* Scope stale policy to PRs only, not issues
Issue staleness is disabled (days-before-issue-stale: -1) and the
alert script now skips any non-PR item defensively, since this
workflow is meant to address PRs sitting unmerged/unreviewed, not
issue triage.
* Draft stale window 60 days (was 90); drop dead pull_request branch
alert-stale.js's pickAlertTargets is now only ever called with PR
items (filtered upstream in runAlertStale), so the pull_request
check around the requested-reviewers lookup was dead code.
* Set persist-credentials: false on checkout steps
Fixes two zizmor notes: these checkouts only need to read local
script files for github-script's require(), so there's no reason
to persist the GITHUB_TOKEN in git config afterward.
* Use a keep-alive checkbox instead of a magic comment phrase for drafts
Checking a box in the bot's own stale-notice comment is more
discoverable than requiring an exact phrase, and the live checkbox
state can be read straight off that comment's current body on each
run instead of scanning new comments for a regex match.
---------
Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org>
* Fix stack buffer overflows in h5repack_parse.c (issue #6433)
Five sites in parse_filter() wrote to fixed 16-byte stack buffers
(scomp[16], stype[16]) via unbounded loop indices with no prior bounds
check, causing a stack-buffer-overflow on malformed -f arguments:
Site 1: scomp[k] - filter name token before '='
Site 2: stype[m] - SZIP pixels_per_block digit sequence
Site 3: stype[m] - SOFF scale_factor digit sequence
Site 4: stype[q] - UD legacy per-field digit sequence
Site 5: stype[m] - all-other-filters digit sequence
Replace all five bare array writes with a single PARSE_BUF_WRITE macro
that checks the index against sizeof(buf)-1 before writing, then frees
obj_list and exits with an error message on overflow — consistent with
all other error handling in parse_filter().
Confirmed with AddressSanitizer: overflow is eliminated and invalid
over-long arguments produce a clean diagnostic exit instead of UB.
* Fix stack buffer overflows in h5repack_parse.c (issue #6433)
All five overflow sites in parse_filter() are now protected via a
PARSE_BUF_WRITE macro that takes an explicit buf_sz argument (passed as
sizeof(buf) at every call site) to avoid silent pointer-decay if the
buffers are ever refactored to heap allocations.
In the SZIP and SOFF inner loops the previous code wrote bare
`smask[l] = c` with no bounds check, and relied on `i = len - 1` to
stop the *outer* loop without also breaking the *inner* `u` loop.
That left one additional iteration where `l` could equal 2 and
`smask[2]` would be written by the next outer iteration before the
outer loop condition was re-evaluated. Both sites now use
PARSE_BUF_WRITE and add an explicit `break` to exit the inner loop
as soon as the two-character mask is complete.
The NULL guard around free(obj_list_ptr) is removed: free(NULL) is a
safe no-op per C99 §7.20.3.2.
* CI: fix buildBody to accept any owner approval, not only the assigned reviewer
PR #6446 narrowed the approver lookup to effectiveReviewers (the confirmed-
requested subset), inadvertently breaking the case where a non-assigned owner
approves. Restore the original intent: any CODEOWNER's approval signs off an
area; fall back to effectiveReviewers only when no CODEOWNER is assigned.
* text updates
* CI: add tests for non-CODEOWNER reviewer sign-off path (#6446)
Three tests cover the nonOwnerReviewers fallback introduced in #6446:
- pending mention when a non-owner is manually assigned
- non-owner approval signs off an area with no CODEOWNER assigned
- non-owner approval does NOT sign off when a CODEOWNER was assigned
* CI: restore review_requested/review_request_removed triggers
These were dropped in #6453 when ready_for_review was added, breaking
automatic checklist updates on manual reviewer changes.
Both events fall through to the preserve-existing path in
coordinateReviewers when a checklist already exists, so the checklist
body is simply rebuilt with the current requested_reviewers set.
Also extend the opening-race detection (previously called isFirstSyncRace)
to cover review_requested: for any PR, CODEOWNERS auto-assignment fires
review_requested shortly after opened, and with cancel-in-progress: true
that run can cancel the opened run. If no checklist exists yet, treat the
review_requested event as a new-PR event and run full selection.
* CI: show only one reviewer per checklist area
Each area's pending mention now shows the primary (first) effective reviewer
rather than all confirmed-requested owners joined with commas.
* CI: revert to showing all confirmed reviewers per checklist area
A manually added person who is also a CODEOWNER for that area should be
shown alongside the load-balanced pick, not hidden. "One reviewer per area"
only describes chooseReviewers' default selection, not a display constraint.
* CI: skip reviewer removal when they have already submitted a review
GitHub's API rejects removeRequestedReviewers for users who have already
reviewed; detect that case upfront via listReviews and report it clearly
instead of surfacing a cryptic API error.
* CI: defer reviewer assignment for draft PRs until ready for review
Add ready_for_review to the pull_request_target trigger so the workflow
fires when a draft is promoted. In the script, skip requestReviewers
(and the CODEOWNERS auto-assignment cleanup second pass) while the PR is
a draft; still assign the PR author and strip any reviewers GitHub
auto-assigned. When the PR is marked ready, treat it like a fresh open
and run the full load-balanced reviewer selection.
* CI: fix synchronize race — treat first synchronize as new PR when opened was cancelled
When a fork PR is created, GitHub fires both opened and synchronize.
With cancel-in-progress: true the synchronize run often wins, and the
opened run (which clears CODEOWNERS auto-assignments) is cancelled before
it completes. Detect this: if synchronize fires but no checklist comment
exists yet, run the full new-PR path (enforceSelection + load-balanced
reviewer assignment) instead of silently carrying forward all auto-assigned
reviewers.
* CI: enforce reviewer selection before posting @mentions
On synchronize, run enforceSelection against the ideal load-balanced
pick before building the checklist body so the @mentions are never
sent until the reviewer list is actually correct. Re-fetch the PR
after cleanup so confirmedRequested reflects reality, not the pre-
cleanup snapshot.
For workflow_run (review submitted), reviewer assignment is intentionally
left unchanged but @mentions are filtered to the ideal selection so
CODEOWNERS extras don't generate spurious notifications on comment edits.
* CI: preserve manually added reviewers on synchronize
Reverting the enforceSelection call on the non-first-run synchronize
path. There is no API way to distinguish CODEOWNERS auto-assignments
from manually added reviewers, so enforcing the load-balanced selection
on every synchronize would silently remove intentional additions.
The initial cleanup (opened or first-synchronize via openedWasSkipped)
already produces a correct reviewer list; subsequent synchronize and
workflow_run events carry it forward unchanged.
* CI: pin actions/github-script to commit hash in remove-reviewer.yml
zizmor requires actions to be pinned to a commit hash rather than a tag.
Use the same pinned hash (ed597411d8f924073f98dfc5c65a23a2325f34cd, v8.0.0)
already used in review-checklist.yml.
* CI: refactor review-checklist.js for readability
Extract the reviewer coordination logic out of the monolithic run()
function into named module-level helpers:
checklistExists() — single responsibility: does a checklist
comment already exist on the PR?
removeUnselected() — remove CODEOWNERS auto-assignments not in
the load-balanced selection set
requestReviewers() — request each reviewer individually so one
bad login cannot block the rest
removeUnselectedAfterDelay() — 15-second wait + re-fetch + re-enforce
for GitHub's async auto-assignment race
coordinateReviewers() — top-level dispatcher; the four event paths
(read-only, synchronize-normal, new-PR/draft,
new-PR/non-draft) are now explicit branches
with labelled comments instead of nested ifs
run() is now a straight pipeline of 8 numbered steps with no nested
async functions. Behaviour is unchanged.
* CI: extract convertGlobToRegex and use github.paginate consistently
Extract glob-to-regex conversion into a standalone helper and replace
manual pagination loops for listFiles and listReviews with github.paginate,
matching the existing style used for listComments and pulls.list.
* CI: restrict remove-reviewer workflow to main repo only
Adds a github.repository guard so the job does not run in forks,
preventing unintended resource consumption and command execution
against fork PRs.
* CI: restrict test-maven-packages workflow to main repo only
Adds github.repository guards to all three jobs so the workflow
does not evaluate in forks, preventing spurious "workflow file issue"
failures on push events in forked repositories.
* Revert "CI: restrict test-maven-packages workflow to main repo only"
This reverts commit cc1208226d.
* CI: fix template injection in workflow_dispatch run: blocks
Move workflow_dispatch inputs that appeared directly in run: scripts
into step-level env: blocks, then reference them as plain shell
variables. This is the pattern zizmor recommends and eliminates the
injection vector.
Only workflow_dispatch inputs are fixed here — workflow_call inputs
in reusable files are not attacker-controlled (they come from the
calling workflow) so no changes are needed there.
Files changed:
- publish-branch.yml: local_dir, target_dir in aws s3 sync
- java-implementation-test.yml: java_versions, platforms, test_mode
- maven-staging.yml: test_maven_deployment, java_implementation,
platforms, use_snapshot_version
- maven-build-test.yml: test_deployment, java_implementation,
platforms, test_examples
- test-maven-packages.yml: version, repository_url throughout
- test-binary-installation.yml: maven_repository, maven_version,
install_method
* CI: move remaining GHA expressions from run: blocks to env: vars
* CI: exclude pure reusable workflows from zizmor template-injection scan
Pure reusable workflows (workflow_call only, no workflow_dispatch/pull_request/
push/schedule/release triggers) can never be triggered directly by external
users. Template-injection findings against inputs.* in those files are false
positives — inputs arrive from the trusted calling workflow, not from attackers.
Scanning only files with user-facing triggers keeps Security tab alerts
meaningful and prevents developers from dismissing real findings.
* Update Visual Studio versions in workflows
* Revert change to Windows ARM workflows
* Pin Intel OneAPI workflows to windows-2025
* Update more instances of issues
* review-checklist: show non-CODEOWNER reviewer when no owner is assigned
If the only assigned CODEOWNER is removed and a non-CODEOWNER is manually
added in their place, that person previously got no checklist mention and
their approval did not check the box.
For any area with no CODEOWNER in the requested set, fall back to
non-CODEOWNER reviewers (anyone assigned who is not an owner of any
touched area). They are shown in the mention and their approval counts
as sign-off for that area.
* review-checklist: update checklist on reviewer add/remove
Adding or removing a reviewer did not trigger a workflow run, leaving
the checklist comment stale until the next push.
Add review_requested and review_request_removed to the pull_request_target
activity types so the checklist updates immediately when a reviewer is
manually added or removed.
The ROS3 VFD appended the raw object key to the HTTP request path.
Because the signing configuration disables use_double_uri_encode (the
correct setting for S3), the SigV4 signer uses the request path
verbatim, so keys containing characters that AWS requires to be
percent-encoded -- such as '=' in Hive-style "key=value" partition
prefixes, '+', or spaces -- produced signatures that disagree with
S3's server-side recomputation. S3 rejects such requests with
SignatureDoesNotMatch, surfaced as a bodyless HTTP 403 that is
indistinguishable from a permissions error on a HEAD request, even
though other S3 clients (AWS CLI, boto3, s3fs) could read the same
objects.
* review-checklist: show all requested owners per area in checklist
buildBody was using area.owners.find() — picking the first CODEOWNERS-listed
owner in the requested set. This broke reviewer swaps (removing @A and adding
@B would show @C, the next CODEOWNERS entry, not @B) and also failed to
reflect GitHub's CODEOWNERS auto-assignment, which requests all owners.
Switch to area.owners.filter() so every requested owner for an area is
mentioned in that row. Approval logic is unchanged: any owner approval
still checks the box.
* review-checklist: enforce one load-balanced reviewer per area
GitHub's CODEOWNERS auto-assignment requests all owners of touched files
when a PR opens, before the workflow runs. The script was then seeing them
already assigned and skipping its own selection entirely.
On opened/reopened: select one load-balanced reviewer per area (ignoring
GitHub's pre-assigned set), remove any auto-assigned CODEOWNERS not in the
selection, then add the chosen reviewer. Only code owners are removed —
manually-added non-owner reviewers are left untouched.
On synchronize: keep existing assignments (reviewer may have already started).
confirmedRequested now starts empty and is populated only with the script's
selection, so the checklist only mentions owners that were explicitly chosen.
* review-checklist: retry reviewer cleanup to handle GitHub auto-assign race
GitHub's CODEOWNERS auto-assignment can fire after the workflow starts,
re-adding extra reviewers after we remove them. Add a 15-second wait
followed by a second cleanup pass on opened/reopened events.
Extract the removal loop into enforceSelection() so both passes share
the same logic.
* zizmor: suppress template-injection for PR #6356 workflow files
Add zizmor_config.yml to ignore template-injection findings in the
setup-jextract action and maven/ctest workflow files introduced in
PR #6356, where inputs are caller-controlled but not user-controlled.
* review-checklist: don't re-assign reviewers on synchronize
On synchronize, chooseReviewers saw no owner assigned (because the
reviewer was manually removed) and re-added one via requestReviewers,
overriding the manual removal.
Reviewer assignment now only happens on opened/reopened. Synchronize
only updates the checklist display based on whoever is currently
assigned — manual removals are respected.
* zizmor: skip upload-sarif failure on fork PRs
* zizmor: move config out of workflows dir to fix GitHub Actions parse error
GitHub Actions parses all .yml files in .github/workflows/ as workflow
files; zizmor_config.yml there caused "unexpected value 'rules'" because
rules: is not valid workflow syntax. Moved to .github/zizmor.yml and
updated the --config path in zizmor.yml accordingly.
* review-checklist: retry on transient 401 from GitHub API
GitHub's API intermittently returns 401 on write operations
(issues.addAssignees, issues.createComment) even when the token has
Issues: write and PullRequests: write — read-only calls succeed in the
same run. The github-script action excludes 401 from retries by default.
Removing 401 from retry-exempt-status-codes and setting retries: 3
handles these transient failures with exponential backoff.
* review-checklist: fix checklist mention when non-requested owner approves
The mention in each checklist row was derived from the approver (if any),
so when a different area owner happened to approve first, the mention
changed from the assigned reviewer to the approver. Fix by decoupling
sign-off detection from display: signedOff now uses .some() so any
owner's approval checks the box, while the mention always shows the
confirmed-requested reviewer(s) via .filter(). Also shows multiple
reviewers when one is manually added alongside the load-balanced
selection.
* review-checklist: show approver name when signed off, requested reviewer(s) when pending
When an area is signed off, replace the mention with the approver so the
checklist shows who actually reviewed it (which may differ from whoever
was load-balanced as the requested reviewer). When pending, show all
confirmed-requested reviewers — the one load-balanced pick normally, but
two if a reviewer was manually added alongside it.
* zizmor: remove config file and --config flag
The ignored files (ctest.yml, maven-deploy.yml, maven-staging.yml,
setup-jextract/action.yml) had template-injection findings on
inputs.* references, which are not attacker-controllable. Rather than
suppressing them via a config file, let all findings surface in the
Security tab and address them individually if needed.
* CI: use pull_request_target so review-checklist runs on fork PRs
pull_request events from forks receive a read-only GITHUB_TOKEN and
are skipped by the job condition, so no checklist comment is posted
and no reviewers are assigned. Switching to pull_request_target fixes
this: GitHub always executes the workflow from the base repo
(HDFGroup/hdf5) with a full write token, regardless of whether the PR
comes from a fork. The fork's code is never checked out or executed.
The cross-repo guard (head.repo.full_name == github.repository) is
removed since it is no longer needed — pull_request_target only fires
for PRs targeting this repo's branches.
* CI: replace pull_request_target with safe two-workflow pattern for fork PRs
pull_request_target carries a GitHub security warning because it grants
write access to the privileged workflow at trigger time, which is risky
if the checkout is ever changed to use the fork's head ref.
Replace it with the recommended two-workflow pattern:
- review-checklist-gather.yml fires on pull_request (read-only, safe
for forks) and uploads the PR number as a short-lived artifact.
- review-checklist.yml fires on workflow_run when the gather job
completes, downloads the artifact for the PR number, then posts the
checklist comment and assigns reviewers with a full write token.
It never executes any code from the fork.
The script gains a prNumber parameter so the main workflow can supply
the PR number it read from the artifact; falls back to the event
payload for pull_request_review triggers.
* review-checklist: fix prAuthor crash and stale comment for workflow_run context
In the workflow_run path context.payload.pull_request is undefined, so
reading .user.login from it throws a TypeError. Use prData.user.login
instead — prData is already fetched from the API at step 5.
Also remove the stale comment saying fork PRs require pull_request_target;
they are now handled via the two-workflow pattern.
* CI: revert to pull_request_target, drop two-workflow pattern
The workflow_run approach adds a second hop before the checklist posts
and introduced a context.payload.pull_request crash in the gather path.
pull_request_target is simpler and equally safe here: GitHub always
executes the workflow from HDFGroup/hdf5:develop with a full write
token — the fork's code is never checked out or executed.
* CI: add zizmor workflow for GitHub Actions security analysis
zizmor is a static analyser that catches security issues specific to
GitHub Actions: pull_request_target misuse, expression injection,
unpinned action SHAs, and overly broad permissions.
Runs on push to develop and on PRs that touch .github/, uploads SARIF
to the Security tab (free for public repos via Advanced Security), and
annotates PR diffs inline with any findings.
* CI: run zizmor via pip instead of third-party action
Replace zizmorcore/zizmor-action (third-party) with a direct pip install
of zizmor==1.25.2. SARIF output is still uploaded to the Security tab via
github/codeql-action/upload-sarif (first-party). The only actions used
are from actions/ and github/, both trusted.
* CI: fix codeql-action pin to commit SHA (was tag object SHA)
* CI: add persist-credentials: false to review-checklist checkout
* CI: hash-pin zizmor pip install; suppress expected pull-request-target finding
- zizmor.yml: pin zizmor wheel to its SHA256 hash via --require-hashes so
the install is fully reproducible and passes zizmor's unpinned-tools audit
- review-checklist.yml: add zizmor: ignore[pull-request-target] comment so
zizmor does not flag its own host workflow; document why the usage is safe
* CI: update actions to Node.js 24 ahead of June 16 deprecation
- actions/checkout: v4.2.2 → v5.0.1 (node20 → node24)
- actions/github-script: v7.0.1 → v8.0.0 (node20 → node24)
- github/codeql-action: v3.36.2 → v4.36.2 (node20 → node24; v3 deprecated Dec 2026)
* CI: fix pip hash-pinning syntax — hash must be in requirements file
* CI: zizmor step continue-on-error so SARIF upload always runs
* review-checklist: skip pull_request_review on fork PRs (read-only token)
GitHub only grants a read-only GITHUB_TOKEN for pull_request_review events
when the PR originates from a fork, so the comment post fails with 403.
Restrict the review trigger to same-repo PRs; pull_request_target already
handles fork PRs for the open/sync/reopen case with a full write token.
* review-checklist: route reviews through workflow_run for fork PR support
pull_request_review grants only a read-only token for fork PRs, causing
a 403 when posting the checklist comment. Fix with a two-path design:
- pull_request_target handles open/sync/reopen for all PRs (full token,
immediate)
- A new gather workflow (review-checklist-gather.yml) fires on
pull_request_review, saves the PR number as an artifact, and exits.
The main workflow then fires via workflow_run with a full write token.
The script gains prNumber and isReview parameters. isReview suppresses
reviewer assignment (which only applies on open/sync/reopen). prAuthor
is sourced from prData.user.login since context.payload.pull_request is
not populated in the workflow_run context.
* review-checklist: simplify to pull_request_target only, drop gather workflow
pull_request_review triggers a fork workflow approval gate for first-time
contributors, which blocks every PR. There is no pull_request_review_target
equivalent in GitHub Actions.
Instead, rely on pull_request_target (opened/synchronize/reopened) only.
Approval boxes are still evaluated correctly: computeApprovals() re-reads
all existing reviews on every push, so boxes auto-check the next time the
author pushes after a reviewer approves.
Also restores the simpler script signature (no prNumber/isReview params)
and sources prAuthor from prData.user.login which works in all contexts.
* review-checklist: auto-check approval boxes on review via workflow_run
Add a minimal gather workflow (review-checklist-gather.yml) triggered by
pull_request_review that completes immediately with a read-only token. The
main workflow now also triggers via workflow_run on gather completion, giving
it the full write token it needs to update the checklist comment.
In workflow_run context, the script looks up the PR by head SHA (since
workflow_run.pull_requests is empty for fork PRs) and skips reviewer
assignment — only approval box state is updated.
Requires the repo "Fork pull request workflows from outside collaborators"
setting to be "Require approval for first-time contributors", which is
already the current setting.
---------
Co-authored-by: H. Joe Lee <hyoklee@hdfgroup.org>
* Updated h5dump --xml
The location of the files HDF5-File.xsd and HDF5-File.dtd no longer exists, and
the files were added to the repo (PR #5490). Before the --xml is actually removed,
running the h5dump tests will fail. This PR updated h5dump and the expected output
to use the correct files' location and actually completed the ticket HELP-2668.
* Added missing expected files from the previous commit
* Fixed typo
* Made corrections per review feedbacks
* Committing clang-format changes
* Missing files from previous commit
* Use valid URL for xmlns
---------
Co-authored-by: Larry Knox <lrknox@hdfgroup.org>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
* Add per-area review checklist action and restructure CODEOWNERS
CODEOWNERS:
- Replace 11-person global catch-all with specific path rules per area,
assigning reviewers based on their actual strengths
- Global fallback is now @fortnern only for uncovered root files
- Remove @derobins, @epourmal, @qkoziol, @mkitti per team discussion
review-checklist GitHub Action (.github/workflows/review-checklist.yml):
- Posts a per-area sign-off checklist on every PR to develop (non-forks only)
- Reviewer lists and path patterns derived directly from CODEOWNERS
- Assigns ONE reviewer per area using fewest-open-PRs load balancing
- Complex changes (≥ 300 lines or any public/developer header modified)
always go to the first (senior) owner listed; routine changes are
load-balanced across all owners (500-line threshold for test/)
- Cohesion: reuses an already-assigned reviewer for related areas where
owner lists overlap, avoiding e.g. src/ and test/ going to different people
- Skips auto-assign if an area owner is already manually requested
- Checklist auto-checks when an owner approves; tracks latest review state
so a subsequent "request changes" unchecks the box
The previous regex only matched *public.h and *develop.h, missing hdf5.h
itself (the umbrella header), all VFD driver headers included by hdf5.h
(H5FDcore.h, H5FDmpio.h, H5FDsubfiling.h, etc.), and VOL connector headers
(H5VLconnector.h, H5VLnative.h, etc.). Changes to any of these now
correctly trigger senior-owner assignment.
* Fix memory safety vulnerabilities in high-level and VFD code
H5FDstdio (src/H5FDstdio.c):
- Fix five error paths in H5FD_stdio_open() that called fclose(f) after
free(file): the correct resource to close is file->fp. Reorder to
fclose before free to match standard cleanup idiom and prevent file
descriptor leaks under memory-pressure failures.
H5VLnative (src/H5VLnative.c):
- Add assert(obj) and assert(file) to H5VL_native_get_file_struct() to
catch NULL-pointer programming errors early in debug builds.
H5LT (hl/src/H5LT.c):
- Add NULL check after strdup() in H5LTtext_to_dtype(); push H5E_NOSPACE
so the HDF5 error stack is populated on OOM.
- Fix H5Tclose(super) leak in H5T_ENUM, H5T_VLEN, H5T_ARRAY, H5T_COMPLEX
branches of H5LT_dtype_to_text(): super was only closed on the success
path; any failure between H5Tget_super and the final realloc_and_append
leaked the type ID. Super is now closed immediately after use.
- Refactor the repeated "get super-type text and append" pattern (four
near-identical ~15-line blocks) into static helper append_dtype_super_text().
Pushes H5E_NOSPACE on internal calloc failure.
- Rewrite realloc_and_append() doc comment to document the asymmetric
ownership contract (callee frees buf on realloc failure in
library-managed mode; no free in user-buf mode).
- Move the buf == NULL guard to before the _no_user_buf branch so both
modes short-circuit identically.
H5TB (hl/src/H5TB.c):
- Clarify H5TBget_field_info() else-branch comment: the two-branch copy
structure is an efficiency optimization (copy name_len+1 bytes rather
than HLTB_MAX_FIELD_LEN-1), not a backward-compatibility concern.
CHANGELOG (release_docs/CHANGELOG.md):
- Add entries for the stdio VFD leak fix, VOL NULL checks, and H5LT
memory-safety improvements.
* HL: add realloc_and_append invariant comments per fortnern review
Document the failure-path contract for realloc_and_append: buf is
passed by value so the caller's pointer is never written by the
function; failure is signaled solely through a NULL return; in
library-managed mode the underlying memory is freed on failure so
callers must not access the original pointer afterward.
Also annotate the point after a successful realloc where the
function can no longer fail.
* HL: reposition 'cannot fail' comment before buf assignment
The comment should mark the transition out of the failure zone —
after the only exit path (goto out), before buf = tmp_realloc.
Also drop the redundant else since the if body always exits.
* HL: fix nested comment syntax error in realloc_and_append Note
* cmake: improve HDF5_BUILD_PARALLEL_TOOLS documentation and MFU error message
The option description for HDF5_BUILD_PARALLEL_TOOLS was too terse to be
useful — it did not mention the required MFU, CIRCLE, or DTCMP external
libraries, nor that HDF5_ENABLE_PARALLEL must also be ON. Expand it with
the dependency list and a link to the mpiFileUtils project.
- CMakeBuildOptions.cmake: rephrase HDF5_BUILD_PARALLEL_TOOLS description
to "Build MPI-enabled HDF5 tools" (shorter, forward-compatible)
After PR #6352 made h5repack default to H5F_LIBVER_V18 as the low bound,
the existing metadata block size option check broke because with V18+
the larger meta_block_size shrinks the output file rather than growing
it. Loop the check over every H5F_LIBVER_* low bound and assert the
size ordering appropriate to each regime.
All #cmakedefine01 CMAKE_H5_* blocks used a five-line pattern:
#cmakedefine01 CMAKE_H5_HAVE_FOO
#if CMAKE_H5_HAVE_FOO == 0
#undef H5_HAVE_FOO
#else
#define H5_HAVE_FOO
#endif
This is exactly what #cmakedefine H5_HAVE_FOO does: it emits
#define H5_HAVE_FOO (no value) when the CMake variable is truthy and
/* #undef H5_HAVE_FOO */ when falsy. The CMAKE_H5_* intermediate
variables in fortran/src/CMakeLists.txt were only needed to feed these
blocks and are no longer required.
Replace all such blocks with #cmakedefine H5_HAVE_FOO, using the H5_*
variable directly. MPI_LOGICAL_KIND retains its value via
#cmakedefine H5_MPI_LOGICAL_KIND @H5_MPI_LOGICAL_KIND@.
This also fixes a real bug: H5_FORTRAN_C_BOOL_IS_UNIQUE was emitted as
#define H5_FORTRAN_C_BOOL_IS_UNIQUE 0 when C_BOOL and default LOGICAL
are the same kind (e.g. Apple PowerPC ABI). #ifdef only tests whether
a macro is defined, not its value, so the guard in H5_test_buildiface.F90
was always true and verify_c_bool was written into tf_gen.F90 regardless,
causing an "Ambiguous interfaces" build failure on that platform.
* Added CMAKE_CFLAGS="-Mnovect" for the workflow with nvhpc 26.3.0 where the issue occurs instead of editing CMake code specifically for test/dt_arith.c that would be applied to all nvhpc versions.
H5O__dtype_decode_helper() reads vlen.type from the file without
validation. With corrupted HDF5 files (e.g. from fuzzing), this field
can have an invalid value that is neither H5T_VLEN_SEQUENCE nor
H5T_VLEN_STRING, which later triggers assert(0) in H5T__vlen_set_loc()
(debug builds) or a NULL pointer dereference / SEGV in release builds.
Fix by:
1. Adding a validation check in H5O__dtype_decode_helper() immediately
after reading the vlen.type field, returning an error if the value
is invalid.
2. Adding a NULL file pointer check in H5T_set_loc() before calling
H5T__vlen_set_loc() when loc == H5T_LOC_DISK, so the low-level
assert(file) invariant is never violated.
This fixes the root cause at the decode level where the bad value
enters the system, as requested in review of #6378 and #6385.
Found by OSS-Fuzz via the matio fuzzer (ClusterFuzz testcase
5366895365914624).
- H5TB: strcmp replaces strncmp in H5TBfind_field so that field names
that are a prefix of a requested name (or vice-versa) are no longer
matched. HLTB_MAX_FIELD_LEN (255) is now public in H5TBpublic.h and
exposed to Fortran as HLTB_MAX_FIELD_LEN_F in H5TBff.F90.
H5TBget_field_info documents the buffer-size requirement and truncation
behaviour. H5TBget_field_info guards against overflow on long names.
- H5IM: H5IMis_image and H5IMis_palette refactored into a shared helper
(H5IM__class_attr_equals). The helper now reads both fixed-length and
variable-length CLASS string attributes, using H5Treclaim for VL memory.
strcmp replaces strncmp for exact-match semantics.
- H5DS: H5DSis_scale and H5DS_is_reserved both support variable-length
CLASS string attributes via H5Aget_space/H5Aread/H5Treclaim. The
fixed-length path retains the 16-byte size guard. strcmp is used
throughout for exact comparison.
- Tests: new test functions test_is_scale_class_prefix,
test_is_reserved_class_prefix, and test_class_prefix cover fixed-length
prefix/exact/wrong-value cases and variable-length string cases.
test_table.c adds write and read field-name prefix rejection cases and
boundary-length truncation verification. All malloc calls are NULL-checked.
- CHANGELOG updated with a summary of all fixes.
Before creating the namespaced `hdf5::<name>` alias, query
ALIASED_TARGET so we point the new alias at the real target. When
the target is not an alias the behaviour is unchanged.
config/HDF5Use{ZLIB,Libaec}.cmake: resolve ALIASED_TARGET before re-aliasing
Both ninja and curl are pre-installed on the GitHub macOS runners,
causing noisy "already installed" warnings. Drop those brew steps
entirely (or just remove ninja/curl where other packages like graphviz
or libaec are still needed).
The earlier block (line 164-167) adds _GNU_SOURCE to
CMAKE_REQUIRED_DEFINITIONS and via add_definitions for MinGW/Cygwin.
However, the later 'MinGW and Cygwin' block overwrites
CMAKE_REQUIRED_DEFINITIONS using CURRENT_TEST_DEFINITIONS (which is
undefined/empty), discarding _GNU_SOURCE. This causes subsequent
configure checks like vasprintf to not see _GNU_SOURCE in their test
definitions.
Use CMAKE_REQUIRED_DEFINITIONS instead of CURRENT_TEST_DEFINITIONS
to append to the existing definitions (which already include
_GNU_SOURCE) rather than replacing them.
Fixes#5885
feat: add optional digital signature verification for HDF5 filter plugins
Introduce an opt-in plugin signing and verification system that allows
HDF5 deployments to require cryptographically signed filter plugins before
loading them. Disabled by default (HDF5_REQUIRE_SIGNED_PLUGINS=OFF).
New tool: h5sign
- Signs plugin shared libraries by appending an RSA signature and a
14-byte footer (algo_id | sig_len | 8-byte magic | format_ver) to the
binary without modifying the original content.
- Supports SHA-512 (default), SHA-256, SHA-384, and their PSS variants
(-a/--algorithm flag).
- Detects already-signed plugins; --force strips the old signature and
re-signs.
- Security hardened: keeps the file descriptor open through hashing and
appending (no TOCTOU window), enforces a 2048-bit minimum RSA key size,
rolls back partial writes on failure, and rejects paths that are not
regular files.
Verification (H5PLsig.c)
- At plugin load time, reads the footer, validates the magic and format
version, then checks the RSA signature against all public keys found in
the KeyStore directory.
- File is hashed once; per-key verification operates on the pre-computed
digest (no redundant I/O for multi-key keystores).
- Plugins whose signature hash appears in revoked_signatures.txt are
rejected regardless of key validity.
- Runtime debug output via HDF5_DEBUG=pl.
KeyStore management
- Trusted public keys are PEM files in a directory specified by
HDF5_PLUGIN_KEYSTORE_DIR (build time) or HDF5_PLUGIN_KEYSTORE (env var).
- HDF5_LOCK_PLUGIN_KEYSTORE cmake option disables the env-var override for
security-hardened deployments.
Test infrastructure
- h5signverifytest: positive, negative, tamper, re-sign, and revocation
test cases.
- CTest fixture-based dependency graph (FIXTURES_SETUP/FIXTURES_REQUIRED)
replaces fragile DEPENDS chains so tests remain correct under -R filtering.
- Dedicated signed-plugins.yml CI workflow; full test suite scoped to
H5SIGN and H5PLUGIN-signature tests to avoid unrelated flaky failures.
- Cross-platform: Linux, macOS, and Windows (MSVC-compatible, BIO-based
OpenSSL I/O, HDsleep/HDsetenv portability wrappers).
Documentation: docs/PLUGIN_SIGNATURE_README.md covers usage, footer
format, revocation file format, FAQ, and troubleshooting.
Co-authored-by: Glenn Song <gsong@hdfgroup.org>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
* CI: remove EOL OS versions from workflows
- freebsd.yml: drop FreeBSD 13.5 (EOL Jan 2026); keep 14.3 and 15.0
- openbsd.yml: bump 7.5 → 7.8 (7.5 EOL ~Nov 2024); drop pinned gcc version
- i386.yml: replace Alpine 3.16 x86 (EOL May 2024) with i386/debian:bookworm (EOL Jun 2028)
* CI: install cmake from bookworm-backports for i386 workflow
* CI: fix OpenBSD workflow for 7.8
- Update vmactions/openbsd-vm v1.3.4 → v1.4.0 (supports 7.8)
- Drop GCC; use OpenBSD's built-in Clang (cc/c++) which is always in PATH
- Remove LD_LIBRARY_PATH workaround that was only needed for egcc
* CI: remove gmake from OpenBSD workflow
* Replace link in Glossary.dox with added Chessboard.svg file.
* Exclude checking links on server "web.cels.anl.gov" that rejects
automated link checker.
Verify uncompressed chunks are the right size after being uncompressed (reverse filtered)
Verify that the buffer returned from the filter callback is large enough to hold the returned data size
Fix bug in deflate filter that caused it to report the wrong buffer size
Fix bug in chunk copy code that could cause a background buffer overflow
Fix bug in chunk copy code that could cause a double free if the filter realloced the data buffer
Other general cleanup
BLOSC2_GIT_BRANCH was "main" causing the inline plugin build to fetch
unreleased c-blosc2 that breaks test output comparisons. Pin to the
tagged release matching HDF5_BLOSC2_VERSION so the git and tgz paths
are consistent.
The value for File_with_compression.h5 should read `345 seconds`
instead of `0.37 seconds`.
Co-authored-by: Kazuyoshi Furutaka (work) <furutaka.kazuyoshi@jaea.go.jp>
Scot Breitenfeld
Larry Knox
vchoi-hdfgroupandH. Joe Lee
jhendersonHDF
Aleksandar Jelenak
Neil Fortner
dependabot[bot]
Dave Allured
tbeu
Kazuyoshi FurutakaandKazuyoshi Furutaka