cp: apply the finalize chmod without following a symlink

[sylvestre]

copy_file and copy_attributes both decide whether to chmod the destination by testing it for being a symlink, but the test happens long before the chmod. In copy_file the lstat is taken at function entry, before the copy starts, so the gap spans the entire file copy - proportional to file size, not a few instructions. A symlink swapped into dest during that gap redirects the mode change onto the link's target, outside the destination tree.

[github-actions]

GNU testsuite comparison:

Skip an intermittent issue tests/date/date-locale-hour (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/misc/tty-eof (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/symlink (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/cut/bounded-memory (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/tail/tail-n0f is now being skipped but was previously passing.
Congrats! The gnu test tests/seq/seq-epipe is now passing!

[codspeed]

Merging this PR will not alter performance

✅ 367 untouched benchmarks
⏩ 50 skipped benchmarks¹

---

_{Comparing sylvestre:cp-finalize-chmod-nofollow (03d438e) with main (c9077c7)}

Footnotes

1. 50 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

[sylvestre]

@codspeedbot fix this regression

[codspeed]

@codspeedbot fix this regression

Fixed and pushed to this branch.

Cause. The new no-follow chmod opened the destination's parent directory to obtain a dirfd, then chmod'ed the final component through it. That is one extra openat(2) and one extra close(2) for every copied file and directory, on top of the chmod itself — which is why only the mode-preserving benchmarks (-a, --preserve=mode) moved. Measured with strace -c on a 341-directory / 3410-file tree:

cp -a	openat	close	total syscalls
main	4098	24558	175055
this branch (before)	11259	31719	182217
this branch (after)	4098	24558	167894

Fix. The chmod is now issued against AT_FDCWD with the whole path: a single fchmodat2/fchmodat with AT_SYMLINK_NOFOLLOW, costing exactly what the chmod(2) it replaces did. The safety property is unchanged — the final component is still not followed — and dropping the dirfd loses nothing, since it was opened immediately before the chmod and therefore pinned nothing across the copy gap. The fchmodat2 fast path and the musl O_PATH fallback now live in one shared helper in uucore::safe_traversal, used by both DirFd::chmod_at and the new path-based chmod_nofollow.

Verification. Reproduced the report locally in CPU simulation mode against main, then re-measured with the fix:

Benchmark	before	after
cp_archive_balanced_tree[(5, 4, 10)]	-3.8%	unchanged
cp_preserve_metadata[(5, 4, 10)]	-4.0%	unchanged

The uucore safe_traversal suite passes, with a new test_chmod_nofollow_preserves_target_mode covering the path-based helper (the symlink target's mode stays untouched), and the cp test suite passes — the permission-denied tests fail in my sandbox only because it runs as root, identically on main.

One drive-by: the new function had been inserted between copy_file's doc comment and copy_file itself, so that doc had become the helper's. It is back on copy_file.

Summary

• Warming up the performance wizard...
• Investigating the cp regression — Root cause: the new nofollow chmod opened the parent directory for every copied entry
• Validating the fix — uucore safe_traversal tests and cp tests pass (pre-existing root-only permission test failures aside)
• Re-running the cp benchmarks — Regression is gone: cp_archive_balanced_tree and cp_preserve_metadata back to main's level
• Pushing the fix — Committed and pushed the single-syscall chmod fix to the PR branch
• Handling comment — Comment handling finished successfully

_{Was this response helpful? React with 👍 or 👎 to help us improve CodSpeedBot.}

[SuperMuel]

Hello @sylvestre, sorry about the failed request. I fixed the bug that prevented the agent from replying here and restarted the latest request. It opened a PR at #13683

[Copilot]

🟡 Changes recommended

Address the Redox build issue, ENOTSUP handling, and remaining xattr TOCTOU race.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens cp permission finalization against destination symlink replacement races.

Changes:

• Adds shared no-follow chmod support with platform fallbacks.
• Uses it during file and attribute copying.
• Adds unit and integration tests for mode preservation and symlink safety.

File summaries

File	Summary	Review findings
tests/by-util/test_cp.rs	Adds cp permission and symlink-safety tests.	—
src/uucore/src/lib/features/safe_traversal.rs	Provides shared no-follow chmod functionality and tests.	—
src/uu/cp/src/cp.rs	Applies no-follow permission updates during copying.	Critical (3 votes): Redox compilation gating is missing. Moderate (1 vote): Handle ENOTSUP as well as EOPNOTSUPP. Critical (2 votes): Extended-attribute permission transitions remain vulnerable to symlink races.

Review details

Suppressed comments (2)

src/uu/cp/src/cp.rs:2515

• The shared helper treats both EOPNOTSUPP and ENOTSUP as the unsupported no-follow result (lines 464-477), and the repository notes that these errno values are distinct on BSDs (src/uucore/src/lib/features/fsxattr.rs:16-23). Filtering only EOPNOTSUPP means a final symlink encountered during the race can make copy_attributes fail on those platforms instead of taking the intended no-op path; handle ENOTSUP here too.

        Err(e) if matches!(e.raw_os_error(), Some(libc::EOPNOTSUPP) | Some(libc::ELOOP)) => Ok(()),

src/uu/cp/src/cp.rs:2515

• This converts every EOPNOTSUPP into success, even when the final component is a regular file and the platform simply does not support AT_SYMLINK_NOFOLLOW; chmod_at_fd explicitly returns that error without a fallback on non-Linux targets. In that case cp -p reports success while leaving the destination mode unchanged. The no-op should be limited to a confirmed symlink case, or the regular-file path needs a safe fd-based fallback.

        Err(e) if matches!(e.raw_os_error(), Some(libc::EOPNOTSUPP) | Some(libc::ELOOP)) => Ok(()),

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

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Critical portability and test issues, plus unresolved safety and error-handling problems, remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

src/uu/cp/src/cp.rs:1943

• This mode branch does not cover the read-only setup in copy_extended_attrs: it still snapshots symlink_metadata(dest) and then calls path-based fs::set_permissions to make and restore the destination writable. If a regular destination is swapped for a symlink after that snapshot, those calls still chmod the link target, so the copy_attributes race remains for xattr-preserving copies. Route those temporary changes through the same no-follow/inode-pinned operation.

            chmod_nofollow(dest, &source_perms)

src/uu/cp/src/cp.rs:2580

• The shared helper treats both EOPNOTSUPP and ENOTSUP as unsupported no-follow operations, and this repository notes that those errno values are distinct on BSDs. If a destination is swapped to a symlink and the platform returns ENOTSUP, this guard propagates the error from copy_attributes instead of taking the intended no-op path, so cp -p can fail; include libc::ENOTSUP in this check.

        Err(e) if matches!(e.raw_os_error(), Some(libc::EOPNOTSUPP) | Some(libc::ELOOP)) => Ok(()),

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

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect symlink safety, portability, and test correctness.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/uu/cp/src/cp.rs:2592

• On non-Linux targets, chmod_at_fd deliberately returns ENOSYS when no-follow fchmodat is unsupported, but this filter omits it. If the destination is swapped to a symlink in that case, copy_attributes propagates the error instead of taking the documented no-op path; classify ENOSYS with the other refusal codes.

    let refused_nofollow = matches!(
        e.raw_os_error(),
        Some(code) if code == libc::EOPNOTSUPP || code == libc::ENOTSUP || code == libc::ELOOP
    );

src/uu/cp/src/cp.rs:2838

• This only protects the normal final chmod. The SELinux-error recovery below still does a path-based symlink_metadata followed by File::create and set_permissions (lines 2885-2889); a symlink swap there can truncate and chmod its target, so this copy_file path retains the TOCTOU escape this change is meant to close.

        chmod_nofollow(dest, &dest_permissions).ok();

src/uucore/src/lib/features/safe_traversal.rs:531

• On Linux without fchmodat2, this is the only fallback and it assumes /proc/self/fd is mounted. In a chroot or container without procfs, cp -p now fails its attribute step for every destination, while ordinary copies silently keep the initial 0600 mode because copy_file ignores this error. Avoid making the cp path depend on procfs or handle that environment explicitly.

    // set_permissions goes through the libc chmod() symbol.
    fs::set_permissions(
        format!("/proc/self/fd/{}", fd.as_raw_fd()),
        fs::Permissions::from_mode(mode),
    )

tests/by-util/test_cp.rs:7653

• This cp -p test runs under cfg(unix) but lacks the cfg_attr(wasi_runner, ignore = ...) used by the surrounding preserve-mode tests; WASI has no chmod syscall, so it will fail instead of being skipped. Add the same WASI ignore attribute.

#[test]
#[cfg(unix)]
fn test_cp_preserve_mode_via_nofollow_chmod() {
    for mode in [0o644, 0o600, 0o755, 0o444, 0o4755] {

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

[Copilot]

🟡 Changes recommended

Unresolved platform, extended-attribute, file-descriptor, and WASI test issues block approval.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/uu/cp/src/cp.rs:2599

• fs::set_permissions follows the final symlink on Unix, so this fallback leaves the original check-then-chmod race on AIX, Hurd, and Redox—the exact platforms excluded from safe_traversal. Implement a platform-specific no-follow operation or explicitly mark those targets unsupported; otherwise this wrapper does not provide the guarantee its name and callers claim.

fn chmod_nofollow(dest: &Path, permissions: &Permissions) -> io::Result<()> {
    fs::set_permissions(dest, permissions.clone())

src/uu/cp/src/cp.rs:1943

• This hardens the mode-preservation call, but copy_attributes then reaches copy_extended_attrs for -a; that helper still changes permissions with path-based set_permissions after a separate metadata check. A destination symlink swap in that block can still chmod the link target, so the reported TOCTOU fix is incomplete.

            chmod_nofollow(dest, &source_perms)

src/uucore/src/lib/features/safe_traversal.rs:525

• AT_FDCWD is -100, not a live file descriptor; wrapping it with BorrowedFd::borrow_raw violates that API's safety precondition even though the sentinel is accepted by *at syscalls. Keep this path on a raw-dirfd API or pass an actual cwd directory fd instead of constructing an invalid BorrowedFd.

    // SAFETY: `AT_FDCWD` is the sentinel the `*at` calls accept to resolve a
    // relative path against the current working directory. It is not a real
    // descriptor, so it is always valid and is never closed.
    let cwd = unsafe { BorrowedFd::borrow_raw(libc::AT_FDCWD) };

tests/by-util/test_cp.rs:7652

• AtPath::set_mode and the cp -p mode assertion require chmod, but this cfg(unix) test is not ignored under wasi_runner; it will panic or fail on WASI. Add the same guard used by test_preserve_mode at lines 3616-3619.

#[cfg(unix)]
fn test_cp_preserve_mode_via_nofollow_chmod() {

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

[Copilot]

🟡 Changes recommended

Unresolved platform, procfs, file-descriptor safety, and WASI test issues remain.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (4)

src/uu/cp/src/cp.rs:2599

• This fallback reintroduces the original symlink TOCTOU on AIX, Hurd, and Redox: fs::set_permissions follows the final symlink after the caller's separate is_symlink check. The PR therefore does not close the reported escape on these supported Unix targets; provide a platform-specific no-follow operation (or an explicit safe failure) instead of this path-based fallback.

    fs::set_permissions(dest, permissions.clone())

src/uucore/src/lib/features/safe_traversal.rs:517

• This documentation is not true on the Linux kernels that take the fallback below: chmod_at_fd then performs openat plus a /proc/self/fd chmod rather than one fchmodat call, so the claimed syscall cost and resolution behavior are unconditional when they are not. Qualify the description to cover the fallback path.

/// The `AT_FDCWD` counterpart of [`DirFd::chmod_at`]: the whole path goes to one
/// `fchmodat` call, so it costs the same as the `chmod(2)` it replaces rather than
/// also opening the parent directory. Parent components are resolved by the kernel
/// inside that single call, so there is no check-then-use window of our own.

src/uucore/src/lib/features/safe_traversal.rs:525

• BorrowedFd::borrow_raw is for a borrowed real file descriptor, but AT_FDCWD is the negative sentinel -100, not an open fd. Passing it through this type violates its safety contract; pass the sentinel through an API that accepts a raw dirfd, or use a real cwd directory fd instead of constructing BorrowedFd from it.

    // SAFETY: `AT_FDCWD` is the sentinel the `*at` calls accept to resolve a
    // relative path against the current working directory. It is not a real
    // descriptor, so it is always valid and is never closed.
    let cwd = unsafe { BorrowedFd::borrow_raw(libc::AT_FDCWD) };

tests/by-util/test_cp.rs:7673

• This test also calls set_mode on the fixture, but has no wasi_runner guard. The existing cp mode tests skip under WASI because chmod is unavailable; add the same guard here so the test does not fail before it can exercise the overwrite behavior.

#[test]
#[cfg(unix)]
fn test_cp_existing_dest_keeps_its_mode() {

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

[Copilot]

🔵 Needs a closer look

Unresolved portability, safety, mode-preservation, and WASI test issues remain.

Review details

Suppressed comments (5)

src/uu/cp/src/cp.rs:2599

• This fallback still calls path-based set_permissions, which follows a final symlink. On AIX/Hurd/Redox (and non-Unix targets), the callers' pre-check remains racy, so a symlink swapped in during the copy can still chmod its target—the security fix is not applied on those targets. Provide a target-appropriate no-follow implementation or do not expose this as chmod_nofollow.

    fs::set_permissions(dest, permissions.clone())

src/uu/cp/src/cp.rs:2827

• If the no-follow chmod is unsupported on a Unix target, this silently drops EOPNOTSUPP for a regular destination. Fresh destinations are created restrictively at 0600, so plain cp then leaves them at 0600 instead of applying dest_permissions; add a platform-safe chmod fallback or otherwise preserve normal-copy mode semantics without following symlinks.

        chmod_nofollow(dest, &dest_permissions).ok();

src/uucore/src/lib/features/safe_traversal.rs:527

• This new path is used by cp -p on every non-excluded Unix target, but the shared implementation has no non-Linux fd fallback when fchmodat(..., AT_SYMLINK_NOFOLLOW) is unsupported. On OpenBSD, where the existing permission-preservation tests are explicitly excluded, that leaves regular-file mode preservation returning the unsupported error and makes the new chmod_nofollow_changes_a_regular_file assumption invalid. Add a supported non-Linux implementation or guard unsupported targets before enabling this API.

pub fn chmod_nofollow(path: &Path, mode: u32) -> io::Result<()> {
    let path_cstr = CString::new(path.as_os_str().as_bytes())
        .map_err(|_| SafeTraversalError::PathContainsNull)?;

    // SAFETY: `AT_FDCWD` is the sentinel the `*at` calls accept to resolve a
    // relative path against the current working directory. It is not a real
    // descriptor, so it is always valid and is never closed.
    let cwd = unsafe { BorrowedFd::borrow_raw(libc::AT_FDCWD) };

    DirFd::chmod_at_fd(cwd, path_cstr.as_c_str(), mode, SymlinkBehavior::NoFollow)

src/uucore/src/lib/features/safe_traversal.rs:525

• BorrowedFd::borrow_raw requires an actual open descriptor, but AT_FDCWD is the negative sentinel accepted by *at syscalls, not a valid descriptor. Passing it as BorrowedFd violates the type's safety contract; keep the cwd case separate or use an API that models the sentinel instead of manufacturing this value.

    let cwd = unsafe { BorrowedFd::borrow_raw(libc::AT_FDCWD) };

tests/by-util/test_cp.rs:7677

• This test changes and asserts POSIX mode bits but has no wasi_runner guard. WASI has no chmod syscall, so it will fail in the WASI test job; add the same ignore used by the preceding mode-preservation test.

fn test_cp_existing_dest_keeps_its_mode() {

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