[iassiour]

Motivation

Two contract test helpers build bare relative paths, so they resolve against the current working directory. An installed test tree run by an unprivileged user has no write permission there (/opt/rocm/share/hip/catch_tests is root:root drwxr-xr-x), so the write fails before the API under test is meaningfully exercised.

Nothing in the failure names the filesystem, so it reads as a genuine runtime failure of the API under test:

test_hip_graph_debug_contract.cc:73:  REQUIRE( false )
  Error: OS call failed or operation not supported on this OS   Code: 304

test_hip_library_file_contract.cc:81: REQUIRE( out.is_open() )

Technical Details

graph_debug and library_file now write through temp_directory_path(), which is writable by definition and honours TMPDIR. Names are qualified by PID so concurrent test binaries do not collide in the shared temp directory.

Paths go through the in-tree fs:: alias from hip_test_filesystem.hh rather than std::filesystem directly. That header exists to fall back to <experimental/filesystem>, so using std::filesystem:: unconditionally would not build on toolchains that only have the experimental one.

No CMake change: catch/CMakeLists.txt:45 sets CMAKE_CXX_STANDARD 17, and hip_add_exe_to_target() already links stdc++fs on UNIX (catch/cmake/hip-tests.cmake:91). Cleanup is unchanged — the tests already delete by the returned path.

Scope

The IPC socket paths (mempool_common.hh, hip_vmm_common.hh) have the same root cause — they bind() an AF_UNIX socket at a bare relative sun_path and get EACCES for the same reason — but that fix carries its own design discussion around socket naming and is split out to keep this one reviewable:

• branch users/iassiour/hip-tests-ipc-socket-paths

This PR is the two contract test files only.

Issue Tracking

JIRA ID: ROCM-31233

Test Plan

1. Run the unmodified contract binaries from a writable cwd and a read-only one (mode 555), to confirm cwd permissions are the cause.
2. hipcc -fsyntax-only -std=c++17 -Wall -Wextra on the patched files.

Test Result

Same binary, only cwd differs:

cwd	ContractGraphDebugTest	ContractLibraryFileTest
writable	exit=0	exit=0
read-only	exit=42, 2/2 assertions failed	exit=42, 2/3 failed

Patched files compile clean, no warnings.

Not covered: the patched binaries were not built and run end to end — CI covers that here.

Submission Checklist

• Look over the contributing guidelines at https://github.com/ROCm/rocm-systems/blob/develop/CONTRIBUTING.md.

[therock-pr-bot]

✅ All Policy Checks Passed

Check	Status	Details
📝 PR Description	✅ Pass	—
⛔ Forbidden Files	✅ Pass	—
🧪 Unit Test	✅ Pass	PR does not contain code files — Unit Test auto-passed
🚫 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?

[therock-pr-bot]

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

• ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

[Copilot]

🟡 Changes recommended

Temporary artifact names are not atomically or reliably unique across concurrent test processes.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Updates HIP contract tests to write generated artifacts to the system temporary directory instead of the current working directory.

Changes:

• Uses std::filesystem::temp_directory_path().
• Adds artifact naming tokens and required headers.
• Preserves existing cleanup behavior.

File summaries

File	Summary
projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc	Writes code objects under the temporary directory.
projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc	Writes graph debug output under the temporary directory.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

---

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

[Copilot]

🟡 Changes recommended

Explicit socket-path validation uses perror without setting errno, producing misleading failure messages.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (4)

projects/hip-tests/catch/unit/memory/mempool_common.hh:657

• Because the temp-directory prefix can make this path exceed sun_path, the length check below is now reachable. That branch returns without closing server_fd or deleting handle, so a long TMPDIR leaks both resources; clean up the allocated socket/handle before returning (and report the local length error without perror).

    std::string nameStr = ipcSocketPath(getpid());
    const char *name = nameStr.c_str();

projects/hip-tests/catch/unit/memory/mempool_common.hh:742

• The same newly reachable over-length-path branch in openSocket() returns without closing sock or deleting handle. With a sufficiently long TMPDIR, every client construction leaks these resources; make the failure path release them before returning.

    std::string nameStr = ipcSocketPath(getpid());
    const char *name = nameStr.c_str();

projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh:90

• Because the temp-directory prefix can make this path exceed sun_path, the length check below is now reachable. That branch returns without closing server_fd or deleting handle, so a long TMPDIR leaks both resources and leaves the test process in a bad state; clean up the allocated socket/handle before returning (and report the local length error without perror).

    std::string nameStr = ipcSocketPath(getpid());
    const char *name = nameStr.c_str();

projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh:152

• The same newly reachable over-length-path branch in openSocket() returns without closing sock or deleting handle. With a sufficiently long TMPDIR, every client construction leaks these resources; make the failure path release them before returning.

    std::string nameStr = ipcSocketPath(getpid());
    const char *name = nameStr.c_str();

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (4)

projects/hip-tests/catch/unit/memory/mempool_common.hh:681

• When the temp-directory prefix makes the socket path exceed sun_path, this new early return occurs after socket() and handle = new ipcHdl; neither resource is released. A long TMPDIR therefore leaks a file descriptor and heap allocation on every server setup attempt. Close server_fd and delete/reset handle before returning (and apply the same cleanup to the client-side length check).

    size_t len = strlen(name);
    if (len > (sizeof(servaddr.sun_path) - 1)) {
      fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n");
      return -1;

projects/hip-tests/catch/unit/memory/mempool_common.hh:747

• This new over-length-path return is reached after creating both handle and sock, so an invalid TMPDIR leaks the socket descriptor and heap allocation. Release both resources (and reset handle) before returning; the server-side check above needs the corresponding cleanup as well.

    if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) {
      fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n");
      return -1;
    }

projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh:116

• When the temp-directory prefix makes the socket path exceed sun_path, this new early return occurs after socket() and handle = new ipcHdl; neither resource is released. A long TMPDIR therefore leaks a file descriptor and heap allocation on every server setup attempt. Close server_fd and delete/reset handle before returning (and apply the same cleanup to the client-side length check).

    size_t len = strlen(name);
    if (len > (sizeof(servaddr.sun_path) - 1)) {
      fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n");
      return -1;

projects/hip-tests/catch/unit/virtualMemoryManagement/hip_vmm_common.hh:157

• This new over-length-path return is reached after creating both handle and sock, so an invalid TMPDIR leaks the socket descriptor and heap allocation. Release both resources (and reset handle) before returning; the server-side check above needs the corresponding cleanup as well.

    if (strlen(name) > (sizeof(cliaddr.sun_path) - 1)) {
      fprintf(stderr, "Socket failure: Cannot bind provided name to socket. Name too large\n");
      return -1;
    }

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One or more issues must be addressed before approval.

Review details

Suppressed comments (2)

projects/hip-tests/catch/contract/graph_debug/test_hip_graph_debug_contract.cc:35

• The shared-temp filename is still only keyed by device and getpid(). That is not unique when binaries run in different PID namespaces (a common setup for installed/containerized tests), and a stale file can also be reused after PID recycling; one run can therefore truncate or read another run's DOT artifact. Reserve a unique temp filename atomically (or add a collision-resistant token) before passing it to hipGraphDebugDotPrint.

  return (std::filesystem::temp_directory_path() /
          ("hip_contract_graph_debug_" + std::to_string(device) + "_" +
           std::to_string(getpid()) + ".dot"))
      .string();

projects/hip-tests/catch/contract/library_file/test_hip_library_file_contract.cc:90

• The code-object path is still only keyed by the caller-provided suffix and getpid(), so separate PID namespaces or PID reuse can select the same file in the shared temp directory. Since ofstream truncates an existing path, concurrent tests can overwrite each other's code object. Create/reserve a collision-resistant temporary path atomically and retain that path for the HIP load and cleanup.

  const std::string path =
      (std::filesystem::temp_directory_path() /
       ("hip-contract-library-file-" + std::string(suffix) + "-" +
        std::to_string(getpid()) + ".code"))
          .string();

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Lite

[mangupta]

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

[Copilot]

Copilot review overview

🟡 Changes recommended

Critical symlink risks and stale socket cleanup issues remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2

Open (2)

• Predictable temp path enables symlink race · New
• Predictable temp file path enables symlink redirection · New

[Copilot]

Copilot review overview

🟡 Changes recommended

The documented IPC socket-path fixes are absent, leaving unprivileged installed IPC tests unresolved.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Predictable temp file path enables symlink redirection
• Predictable temp path enables symlink race
• IPC helpers still use relative PID-only socket paths · New

[Copilot]

Copilot review overview

🟢 Approval recommended

No unresolved review comments remain, and the changes are narrowly scoped.

Review effort: Lite
Findings: 2 · 1

Open (3)

• Predictable temp file path enables symlink redirection
• Predictable temp path enables symlink race
• IPC helpers still use relative PID-only socket paths

[Copilot]

Copilot review overview

🟡 Changes recommended

Both files need atomically reserved paths and cleanup on failure paths.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 4

Open (4)

• Reserve unique export paths atomically · New
• Atomically create and reserve unique artifact files · New
• Predictable temp file path enables symlink redirection
• Predictable temp path enables symlink race

Resolved since last review (1)

• IPC helpers still use relative PID-only socket paths

[Copilot]

Copilot review overview

🟡 Changes recommended

The temporary-path helper must atomically reserve unique files before use.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 5

Open (5)

• TempPath does not atomically reserve temporary files · New
• Atomically create and reserve unique artifact files
• Reserve unique export paths atomically
• Predictable temp file path enables symlink redirection
• Predictable temp path enables symlink race

[Copilot]

Copilot review overview

🔵 Needs a closer look

Temporary paths must be exclusively reserved before writing.

Review effort: Lite
Findings: None

Resolved since last review (5)

• TempPath does not atomically reserve temporary files
• Atomically create and reserve unique artifact files
• Reserve unique export paths atomically
• Predictable temp file path enables symlink redirection
• Predictable temp path enables symlink race

[systems-assistant]

TheRock Submodule Bump Activity

Newest first

• 2026-09-29 — Included in TheRock via ROCm/TheRock#8567.