Skip to content

fix(hrr): keep capture archives private and refuse planted links - #12253

Merged
andraghetti merged 32 commits into
developfrom
users/andraghetti/hrr-capture-private-archive
Oct 9, 2026
Merged

andraghetti merged 32 commits into
developfrom
users/andraghetti/hrr-capture-private-archive

Conversation

@andraghetti

@andraghetti andraghetti commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

An HRR capture archive holds the recorded process's host buffers, kernel arguments and code objects. The capture writer created it with default file modes (files 0644, directories limited only by the umask), opened its files without O_NOFOLLOW, used predictable temporary file names, and reused an existing per-process directory, events.bin and manifest without checking what they were. Its directory helpers also called the throwing std::filesystem functions from hip::init, the capture shims and atexit, so an output path that could not be created made the application's first HIP call fail.

This change keeps the archive private to the user who captured it, makes the writer refuse symbolic links and a per-process directory or events.bin that does not belong to that user, keeps it from truncating or waiting on anything else found inside the archive, and turns a failure to set up the archive into a disabled capture with a message on stderr.

Technical Details

POSIX paths in projects/clr/hipamd/src/hrr/hip_capture_writer.cpp:

  • ensure_dir creates the missing directories of the archive, from the base directory down, with mkdir(..., 0700) and any missing parents above it with 0777 minus the umask, as on develop, and returns false with errno set instead of throwing. Every file the writer creates is 0600.
  • claim_private_dir opens a directory with O_NOFOLLOW, requires it to be a directory owned by the effective user, and tightens an existing one to 0700. It runs on pid-<pid>, blobs/ and code_objects/ when the archive is opened, and once for each blobs/<xx> prefix. A blob whose prefix directory is refused is not written, and the archive is marked incomplete.
  • open_events_file reuses an existing events.bin only if it is a regular file with a single link owned by the effective user, refuses anything else already there, and creates a missing one with O_EXCL. open() resumes an archive based on the size it reports, and the resume scan reads through a duplicate of that descriptor instead of opening the path again. A resume that cannot cut the trailer or a torn tail off events.bin disables capture instead of appending after it.
  • open_private_fd opens manifest.json and writer_state.json for writing (through fopen_private) and the crash-time manifest (through HRR_OPEN): O_NOFOLLOW | O_NONBLOCK | O_CLOEXEC, mode 0600, and it truncates only after checking that the opened file is a regular file with a single link owned by the effective user. It uses only async-signal-safe calls, since the crash callback runs it.
  • fopen_read_regular reads writer_state.json and the per-process manifests collected into the root manifest: O_NOFOLLOW | O_NONBLOCK, regular files only.
  • atomic_write_file (blobs, code objects and the root manifest) takes its temporary file from mkostemps instead of <path>.<pid>.tmp. A blob or code object that cannot be written marks the archive incomplete, since the event that refers to it is still written.
  • index_existing_blobs_locked and update_root_manifest use the std::error_code overloads of the directory iterators, check each entry with its own std::error_code, and do not follow symbolic links. update_root_manifest leaves the root manifest as it is when the base directory cannot be read to the end.
  • When the archive cannot be set up, open() prints the reason on stderr, clears the archive paths so that a later fork() does not reopen it in the child, and returns false. hip_capture_init() in hip_capture.cpp then removes the runtime shims again, and atfork_child() does the same in a forked child whose own open() fails.
  • open() removes a stale pid-<pid>/active once pid-<pid> is accepted, before any step that can fail, and creates it, 0600, as its last step; flush() removes it at shutdown. Producers read that file, not events.bin, as capture being active: a same-pid resume that fails after opening events.bin leaves the earlier run's file in place. A marker that cannot be created disables capture.
  • The checks on a directory run when the archive is opened. Later opens resolve the path again, and O_NOFOLLOW covers only its last component, so the file header says so and projects/hrr/DESIGN.md says every directory on the path to the archive should belong to the capturing user or to root, and any that other users can write to should have the sticky bit.

projects/hrr/DESIGN.md, projects/hrr/README.md and projects/hrr/producers/README.md describe who can read an archive and what happens when it cannot be set up, and the Init / Shutdown section of DESIGN.md says what a failed open() does in the process and in a forked child.

On Windows the file modes, temporary file names and resume logic are unchanged; the non-throwing directory handling, the errno on failure, the incomplete mark for a blob or code object that cannot be written and the removal of the runtime shims apply there too. Windows was not built for this change.

Behaviour change: an archive is readable only by the user who captured it, so a capture taken as root in a container needs chown or sudo before another user can read it. A pid-<pid> directory owned by another user, or an events.bin with more than one hard link, now disables capture. A per-process manifest.json or writer_state.json that is not a regular file with a single link is left as it is and not written. A blob or code object that cannot be written marks the archive incomplete.

Issue Tracking

JIRA ID: AIRUNTIME-2829

Test Plan

  • Built amdhip64 with -DHIP_HRR_CAPTURE_ENABLED, hrr-playback, hrr-unit-tests and hrr-integration-tests on Linux for gfx950, and ran both suites on an MI350X. Built the same targets without HIP_HRR_CAPTURE_ENABLED and ran hrr-unit-tests.
  • Compiled hip_capture_writer.cpp with -Wall -Wextra, with and without HIP_HRR_CAPTURE_ENABLED, and hrr_capture_access_test.cc.
  • Regenerated the HRR API files with gen_hrr_api_args.py --check-hrr-coverage and compared them with the committed ones.
  • projects/hrr/tests/integration/hrr_capture_access_test.cc. Unit_HRR_CaptureArchiveIsPrivate clears the umask, captures into a directory whose parent does not exist yet and then again into a pid-<pid> created 0755, and checks that every directory in the archive is 0700, every file 0600 and the parent the capture created 0777. Unit_HRR_CaptureRefusesPlantedLinks plants a symbolic link at pid-<pid>/events.bin, at pid-<pid> or at pid-<pid>/blobs, or a hard link at pid-<pid>/events.bin, and checks that capture reports itself disabled and leaves the link target untouched; it also plants a hard link at pid-<pid>/manifest.json and checks that the linked file is untouched while events.bin is written, both after a clean exit and after the workload aborts and leaves the manifest to the crash callback. Unit_HRR_CaptureDoesNotBlockOnPlantedFifo plants a FIFO at pid-<pid>/manifest.json and checks that the workload exits within a deadline. The three were also run against a capture runtime built from this branch's base, develop c716bb08bf.
  • Unit_HRR_CaptureActiveMarker checks that pid-<pid>/active exists while a capture runs and is gone after it, that a stale one is removed when the archive is refused, and that a resume which cannot create it, or cannot cut the trailer off events.bin (a seccomp filter in the workload fails that ftruncate), disables capture and appends nothing. producers/pytorch/tests/test_hrr_torch_regions.py checks that the producer ignores an events.bin without the marker.
  • By hand, against both runtimes: a pid-<pid> directory owned by another user, an events.bin with a second hard link, an output directory under /proc, an archive resumed by a process with the same pid, a symbolic link at every blobs/<xx> prefix, blob and code-object files that cannot be created in their directory (an LD_PRELOAD shim fails every temporary file under blobs/, then under code_objects/, with ENOSPC), a forked child whose own open() fails (the child runs with RLIMIT_NOFILE at 3), and a base directory that cannot be listed (mode 0333, run as root without CAP_DAC_OVERRIDE) during the second of two captures.
  • By hand, with this change only, as root without CAP_DAC_OVERRIDE: entries whose own check fails ahead of the real archives in the base directory (dangling pid-* links, pid-* entries at mode 0000 and 0400, and names too long for the full path), and entries in blobs/ whose check fails before an archive is resumed.

Test Result

Run as root in a ROCm 7.13 container with umask 0022.

Check With this change Without it
hrr-unit-tests pass, 98 assertions in 23 test cases, with and without HIP_HRR_CAPTURE_ENABLED not run
hrr-integration-tests pass, 682 assertions in 57 test cases not run
Build without HIP_HRR_CAPTURE_ENABLED builds not run
hip_capture_writer.cpp and hrr_capture_access_test.cc with -Wall -Wextra no warnings in the HRR sources not run
Generated HRR API files identical to the committed ones not run
Unit_HRR_CaptureArchiveIsPrivate pass fail: with the umask cleared, directories 0777, events.bin 0644 and the other files 0666, and the pid-<pid> created 0755 stays 0755
Unit_HRR_CaptureRefusesPlantedLinks pass, all six sections fail in all six: no message that capture is disabled, the link targets are written to, and the hard-linked files are rewritten, including by the crash callback
Unit_HRR_CaptureDoesNotBlockOnPlantedFifo pass fail: the workload does not exit and is killed after 120 s
pid-<pid> owned by another user capture disabled with a message, nothing written 19 entries written into it
events.bin with a second hard link capture disabled with a message, the other file unchanged the other file is rewritten
Output directory under /proc capture disabled with a message, workload exits 0 hipSetDevice fails and so does the workload
Archive resumed by the same pid 89 + 89 = 178 events, complete, directories 0700 and files 0600 178 events, complete, directories 0755 and files 0644
Symbolic link at every blobs/<xx> no blob written through them, archive marked incomplete 7 blobs written through them, archive complete
No blob file can be created archive marked incomplete archive complete without its blobs
No code-object file can be created archive marked incomplete archive complete without its code object
Forked child whose open() fails capture disabled with a message, the child's runtime shims removed no message, the child keeps the shims
Base directory that cannot be listed, second capture root manifest from the first capture kept, workload exits 0 the workload aborts at exit
Entries whose own check fails, ahead of the real archives in the base and in blobs/ both archives in the root manifest; on resume the 7 existing blobs are found and none is written again not run

Submission Checklist

@andraghetti
andraghetti requested review from a team as code owners September 25, 2026 10:37
Copilot AI lite review requested due to automatic review settings September 25, 2026 10:37
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Unresolved critical security findings remain in archive resume and filesystem handling.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4 High severity

Open (4)
What changed in this PR

Hardens HRR capture archives against unauthorized access and planted filesystem links, while disabling capture cleanly when setup fails.

Changes:

  • Uses restrictive permissions and safer file/directory handling.
  • Adds archive privacy and planted-link integration tests.
  • Removes capture shims after initialization failure.
File Summary
projects/​hrr/​tests/​integration/​hrr_capture_access_test.cc Adds archive privacy and planted-link tests.
projects/​hrr/​tests/​integration/​CMakeLists.txt Registers the integration test source.
projects/​clr/​hipamd/​src/​hrr/​hip_capture.cpp Removes shims when capture setup fails.
projects/​clr/​hipamd/​src/​hrr/​hip_capture_writer.cpp Implements private archive, resume, and temporary-file handling.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@andraghetti
andraghetti force-pushed the users/andraghetti/hrr-capture-private-archive branch from 627a03e to 6fc08c4 Compare September 25, 2026 11:36

@danielbolsson danielbolsson 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.

Thanks, this is a solid hardening pass. Notes below, most important first.

Blocking

  1. The crash-path manifest write can still truncate through a hard link. emergency_finalize opens pid-<pid>/manifest.json with HRR_OPEN, which still has O_TRUNC and no inode check. O_NOFOLLOW stops symlinks but not hard links. That breaks the guarantee the new file header and fopen_private describe, and the one the hard-link test section claims (the test only covers the clean-shutdown path). See the inline comment on HRR_OPEN.

Non-blocking
2. g_blob_prefix_claimed is read and written in write_blob without holding g_file_mu or g_blob_mu, so concurrent blob writes race on a plain bool. In practice it's harmless because claiming is idempotent, but it's still undefined behaviour and TSan will report it. std::atomic<bool> with relaxed loads and stores fixes it.
3. When open() fails, the fork handler is still armed and g_base_dir is already set, so every later fork() calls writer::open(g_base_dir) in the child. That either creates an orphan pid-<child> directory containing a header-only events.bin, even though capture is off, or prints the Capture disabled message again on each fork. Clearing g_base_dir/g_output_dir on the failure returns stops this.
4. fopen_private (and HRR_OPEN) open without O_NONBLOCK, so a FIFO planted at manifest.json or writer_state.json blocks flush()/checkpoint() indefinitely. Adding O_NONBLOCK lets the existing S_ISREG check reject it.
5. The setup-failure message uses errno, but ensure_dir does not set it reliably. On Windows, create_directories(path, ec) doesn't touch errno, so the message can end in "(Success)". On POSIX, the !S_ISDIR branch returns after a successful stat. Set errno explicitly before each return false, as claim_private_dir already does.
6. The atfork_child comment still says it runs fs::create_directories. mkostemps is used without including <cstdlib>: this only compiles because <string> pulls it in transitively.
7. Tests:

  • The link at pid-<pid> and hard-link sections would also pass if capture never ran. Asserting the Capture disabled stderr line, or that pid-<pid>/events.bin exists in the hard-link case, would make them conclusive.
  • perms_of masks with perms::mask. The base directory is never fchmoded, so under a setgid TMPDIR it's 02700 and Unit_HRR_CaptureArchiveIsPrivate fails.
  • The CMake comment mentions only symbolic links, but the test also covers hard links.

@andraghetti
andraghetti force-pushed the users/andraghetti/hrr-capture-private-archive branch from 6fc08c4 to 0fce132 Compare September 28, 2026 13:58

Copilot AI 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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

@danielbolsson danielbolsson 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.

Approving. The hardening is sound: the inode check before truncation (instead of O_TRUNC), the async-signal-safe open_private_fd on the crash path, the resume scan through a dup of the validated fd, and open_failed() clearing the archive paths all hold up under tracing. Nothing below blocks; please consider these for this PR or a follow-up.

Suggestions

  1. Fork child keeps its shims when open() fails. hip_capture_writer.cpp:624-625 (atfork_child). The rationale at hip_capture.cpp:1618-1620 applies to the child too, and this PR adds new ways for the child's open to fail. Suggested fix: if (!dir.empty() && !writer::open(dir.c_str())) hip_capture_uninstall();
  2. Remaining TOCTOU window on pid-<pid>/ (and on blobs/, code_objects/). claim_private_dir (:321) validates through an O_NOFOLLOW dirfd, closes it, and every later open resolves the full path again (:358/:360, :381, :1110/:1115, :1008). If the base dir is attacker-writable and not sticky, swapping pid-<pid> for a symlink after the claim moves events.bin into another directory, and every fstat check still passes. A local PoC confirmed this. Fixing it properly means keeping the dirfds and using openat/renameat, which is fine as a follow-up. For now, please soften the header comment (:24-27) to "last path component", or warn when the base dir is group/other-writable without the sticky bit.
  3. ensure_dir (:276) now creates every missing ancestor as 0700, not only the archive. Example: HIP_HRR_CAPTURE_OUTPUT=/scratch/shared/run1 creates /scratch/shared as owner-only. Please confirm that is intended, or apply 0700 only from the base dir down.
  4. Docs. projects/hrr/DESIGN.md (archive layout, init/shutdown), README.md (capture env table) and producers/README.md don't mention the 0700/0600 model, the same-uid requirement, or that a refused archive disables capture.
  5. The seek-failure path (:853-858) only calls LogPrintfError. Its two siblings also print [HRR capture] Capture disabled: ... to stderr, so it should too.
  6. A failed blob or code-object write still leaves a "complete" archive. At :1152-1158 (new), :1169-1174 and :1201-1205 the event references a blob that was never written. Consider mark_incomplete(...) in those branches.
  7. Minor:
    • open_failed() could also clear g_manifest_path.
    • index_existing_blobs_locked (:584, :591) and update_root_manifest (:725) follow symlinks. Use symlink_status.
    • A few comments are stale: :1086 still says "fwrite", :318-320 only mentions the pid dir, and the TU list in CMakeLists.txt:36-38 is out of date.

Tests (hrr_capture_access_test.cc)

  • Unit_HRR_CaptureArchiveIsPrivate depends on the umask: with umask 0077 it passes on a reverted build. Setting umask(0) around the capture fixes that.
  • Nothing covers a hard link at pid-<pid>/events.bin, which is the worst case for the nlink check. Nothing asserts that an existing pid dir gets tightened to 0700 either.

Interaction with #12256. The two PRs touch the same blob-write path. Once both land, the prefix-claim failure path added here has to release the space reservation from #12256, or it leaks. The two PRs also add overlapping cleanup helpers. The test in this PR matches on the "Capture disabled" stderr prefix, so keep that string stable across both.

Checks run:

  • gen_hrr_api_args.py --check-hrr-coverage passes.
  • -fsyntax-only -Wall -Wextra on the writer is clean.
  • Scratch harnesses cover ensure_dir, the fd-offset sharing on resume, and O_NOFOLLOW.

I did not build with HIP_HRR_CAPTURE_ENABLED or run the GPU tests; I relied on the MI350X results reported in the PR.

@mangupta mangupta 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.

🤖 AI-assisted pre-review (for @mangupta)

@andraghetti

andraghetti commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor Author

These are in 90806b83b2:

  1. Done, a forked child whose open() fails removes its runtime shims now.
  2. Done, the header comment says last path component, and DESIGN.md asks for every directory on the path to belong to the capturing user or root, with the sticky bit on any that other users can write to. I've opened AIRUNTIME-2847 for the dirfd and openat rework, as you suggest.
  3. Not intended. Parents above the base directory get 0777 minus the umask again, as on develop, and only the base and what's under it are 0700.
  4. Done, in DESIGN.md under the archive layout, under the capture table in README.md and in producers/README.md, and Init / Shutdown now says what a failed open does in the parent and in a forked child.
  5. Done, it prints the same Capture disabled line on stderr now.
  6. Done, all three branches mark the archive incomplete now.
  7. Done, open_failed() clears g_manifest_path too, both scans use symlink_status, and the three comments are fixed.

Both test notes are done: Unit_HRR_CaptureArchiveIsPrivate clears the umask and checks that a pid-<pid> created 0755 ends up 0700, and Unit_HRR_CaptureRefusesPlantedLinks plants a hard link at pid-<pid>/events.bin.

On #12256, the reservation guard I'm about to push there also covers the prefix-claim return here, and the Capture disabled prefix stays as it is in both. Whichever lands second can keep just one of open_failed() and clear_archive_paths(), since they clear the same three paths now.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Restrictive umasks can leave atomically written files unreadable instead of mode 0600.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)

andraghetti added a commit that referenced this pull request Oct 1, 2026
Hold the space reservation in a guard, so any early return after reserve_space(), such as the one #12253 adds, releases it.
@andraghetti
andraghetti requested a balanced review from Copilot October 1, 2026 16:11

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The active marker remains published when capture stops for low disk space, causing producers to continue writing after capture has ended.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (1)

@mangupta

mangupta commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (fac101ebb4c5) — no new findings at or above the posting threshold.

stop_for_space() closed events.bin while the application ran on, but
left pid-<pid>/active in place, so producers kept writing sidecars to an
archive that records nothing, on a file system already short of space.
@andraghetti

Copy link
Copy Markdown
Contributor Author

Decision record for the finding "Avoid treating a created PID directory as capture activation" (r5382212497, projects/hrr/producers/README.md:40): fixed in 203c5f6. Producers no longer take pid-<pid>/ as proof that capture is on. The writer creates pid-<pid>/active as the last step of a successful open(), and the README tells producers to check that file: a regular file of this user, with one link, reached through no symlink. The writer removes it at shutdown and, since ba65873, when capture stops for lack of space.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

A stale active marker can falsely signal capture after PID reuse.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (1)

A process killed with SIGKILL leaves pid-<pid>/active behind. A later
process with the same pid could find it before its own HIP init and
write sidecars into an archive that is not its own.

The writer now writes one line into the marker: the boot id and the
start time from /proc/self/stat on Linux, the creation time on Windows.
The reference producer, the producer README and DESIGN.md compare that
line with the producer's own process.

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Active-marker documentation remains inconsistent and its reference implementation mishandles unavailable process identity.

Review effort: Balanced
Findings: 2 Low severity

Open (2)
Resolved since last review (1)

Comment thread projects/hrr/DESIGN.md Resolved
Comment thread projects/hrr/producers/README.md Resolved
@mangupta

mangupta commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (c82c8dc60882) — no new findings at or above the posting threshold.

The archive layouts in DESIGN.md still called the marker empty. The README
sample raised when /proc could not be read or a field was malformed; like the
bundled producer, it now reports capture as inactive instead.
…raghetti/hrr-capture-private-archive

# Conflicts:
#	projects/clr/hipamd/src/hrr/hip_capture_writer.cpp

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The fork-failure test does not verify that the newly added dispatch-shim uninstall actually occurs.

Review effort: Balanced
Findings: 1 Low severity

Open (1)
Resolved since last review (2)

@mangupta

mangupta commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (f7ca3d5775c7) — no new findings at or above the posting threshold.

Unit_HRR_CaptureDisabledInForkedChild only checked the message and the
removed pid-<pid>, so a child that kept a capture shim in either dispatch
table still passed. Its child now calls through both tables and counts its
clock reads: a capture shim reads the clock to timestamp its record.

hrr_clock_hook.cc defines clock_gettime for the test binary and runs a
per-thread hook on each read.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

A refused active marker can produce a misleading “Success” diagnostic because validation failures do not set errno.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Preserve errno when rejecting invalid marker inodes

projects/​clr/​hipamd/​src/​hrr/​hip_capture_writer.cpp:418

When the inode fails the regular-file, link-count, or ownership check, no syscall sets errno; after the descriptor is closed this can return -1 with errno == 0 (or an unrelated stale value). publish_active_marker() then reports Capture disabled: ... (Success), hiding why a planted marker was refused. Record EPERM for validation failures and preserve the errno from each failing syscall before closing the descriptor.

open_private_fd refused a hard link, a non-regular file or a file owned
by another user without setting errno. The open() before the check had
succeeded, so "Capture disabled: cannot create .../active" printed
whatever an earlier call had left in errno. It now sets EPERM for a
refused inode and keeps errno from a failed fstat, fchmod or ftruncate,
as open_events_file already does.

Unit_HRR_CaptureActiveMarker runs a new workload that plants a hard link
at pid-<pid>/active while the archive opens, through a statvfs hook in
hrr_disk_space_test.cc. It checks that the message gives EPERM and that
the linked file keeps its contents.
@andraghetti

Copy link
Copy Markdown
Contributor Author

Preserve errno when rejecting invalid marker inodes (medium): fixed in 35dc92b.

open_private_fd now sets errno before it returns -1. A refused inode gives EPERM; a failed fstat, fchmod or ftruncate keeps its own errno across close(). So the "capture disabled" message names the real reason.

Test: a new section in Unit_HRR_CaptureActiveMarker runs Unit_HRR_CaptureMarkerRefused_Direct. That workload plants a hard link to a file outside the archive at pid-<pid>/active while the archive opens, after the writer removed any stale marker. The test checks three things: capture is disabled, the message ends in /active (Operation not permitted), and the linked file keeps its contents. With the writer change reverted, the EPERM check fails.

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

The security-sensitive filesystem and fork behavior is extensive, and the Windows paths were not built or tested.

Review effort: Balanced
Findings: None

@mangupta

mangupta commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (35dc92ba9c29) — no new findings at or above the posting threshold.

…hive

# Conflicts:
#	projects/hrr/tests/integration/CMakeLists.txt
@mangupta

mangupta commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (1b89720406a5) — no new findings at or above the posting threshold.

…hive

# Conflicts:
#	projects/clr/hipamd/src/hrr/hip_capture_writer.cpp
@mangupta

mangupta commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-assisted pre-review (for @mangupta)

Re-reviewed the updated head (377a34cb3d98) — no new findings at or above the posting threshold.

@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants