Skip to content

util: make permission walks symlink-safe#2358

Merged
MarcusSorealheis merged 3 commits into
TraceMachina:mainfrom
erneestoc:ec/pr2243-symlink-chmod-fix
May 23, 2026
Merged

util: make permission walks symlink-safe#2358
MarcusSorealheis merged 3 commits into
TraceMachina:mainfrom
erneestoc:ec/pr2243-symlink-chmod-fix

Conversation

@erneestoc

@erneestoc erneestoc commented May 22, 2026

Copy link
Copy Markdown
Contributor

Problem

The recursive permission walk set_perms_recursive_impl (which drives both
set_readonly_recursive and set_dir_writable_recursive) used fs::metadata
(stat), which follows symlinks. On input trees containing symlinks — e.g.
.venv/bin/python3 produced by rules_python / rules_apple venv tooling —
this has two failure modes:

  • A symlink to a directory reports is_dir() == true, so the walk recurses
    through the link, escaping the materialized tree or descending into an
    unrelated directory.
  • A symlink is passed to set_permissions; chmod follows symlinks, so it
    mutates the link's target. When the target does not exist (a dangling link
    — common when a venv points outside the action's input set), the chmod
    returns ENOENT and fails the entire walk.

That ENOENT failure surfaces as set_readonly_recursive erroring inside
DirectoryCache::get_or_create, which makes prepare_action_inputs log
"Directory cache failed, falling back to traditional download" and take the
slow download_to_directory path.

Fix

set_perms_recursive_impl now uses symlink_metadata (lstat) and returns
early on symlink entries — it never chmods a symlink and never recurses
through one. Regular files keep their existing read-only (0o555) treatment,
so the CAS-hardlinked-inode hermeticity contract (#2347) is unchanged.

hardlink_directory_tree_recursive already recreated symlinks as symlinks; its
symlink branch is reordered ahead of the is_dir() / is_file() branches to
make the symlink-first intent explicit and robust.

Testing

Adds regression tests covering the set-readonly, set-dir-writable, and
hardlink/clone walks over a tree containing a symlink to an in-tree file, a
dangling relative symlink, and a symlink to an in-tree directory — asserting
each walk succeeds and the symlinks are preserved with their targets intact.

bazel test //nativelink-util/... //nativelink-worker/... — 27/27 pass;
clippy + rustfmt aspects clean.

Note

Companion to #2357 (worker: make directory-cache entries already-writable).
Both PRs touch nativelink-util/src/fs_util.rs
(hardlink_directory_tree_recursive); whichever lands second needs a trivial
rebase.

🤖 Generated with Claude Code


This change is Reviewable

The recursive permission walk `set_perms_recursive_impl` (driving both
`set_readonly_recursive` and `set_dir_writable_recursive`) used
`fs::metadata` (stat), which follows symlinks. On input trees containing
symlinks - e.g. `.venv/bin/python3` produced by rules_python /
rules_apple venv tooling - this had two failure modes:

  * A symlink to a directory reported `is_dir() == true`, so the walk
    recursed *through* the link, escaping the materialized tree or
    descending into an unrelated directory.
  * A symlink was passed to `set_permissions`; `chmod` follows symlinks,
    so it mutated the link's target. When the target did not exist (a
    dangling link - common when a venv points outside the action's
    input set) the `chmod` returned ENOENT and failed the entire walk.

That ENOENT failure surfaced as `set_readonly_recursive` erroring inside
`DirectoryCache::get_or_create`, which made `prepare_action_inputs` log
"Directory cache failed, falling back to traditional download" and take
the slow `download_to_directory` path.

Fix: `set_perms_recursive_impl` now uses `symlink_metadata` (lstat) and
returns early on symlink entries - it never chmods a symlink and never
recurses through one. Regular files keep their existing read-only
(0o555) treatment, so the CAS-hardlinked-inode hermeticity contract
(PR TraceMachina#2347) is unchanged.

`hardlink_directory_tree_recursive` already recreated symlinks as
symlinks; its symlink branch is reordered ahead of the `is_dir()` /
`is_file()` branches to make the symlink-first intent explicit and
robust.

Adds regression tests covering set-readonly, set-dir-writable, and
hardlink/clone walks over a tree containing a symlink to an in-tree
file, a dangling relative symlink, and a symlink to an in-tree
directory, asserting each walk succeeds and the symlinks are preserved
with their targets intact.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@MarcusSorealheis MarcusSorealheis left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM by itself

@MarcusSorealheis MarcusSorealheis enabled auto-merge (squash) May 23, 2026 20:57
@MarcusSorealheis MarcusSorealheis merged commit 5699a27 into TraceMachina:main May 23, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants