Skip to content

cp: don't chmod through the destination symlink#13443

Open
Angadi56 wants to merge 3 commits into
uutils:mainfrom
Angadi56:cp-no-chmod-through-symlink
Open

cp: don't chmod through the destination symlink#13443
Angadi56 wants to merge 3 commits into
uutils:mainfrom
Angadi56:cp-no-chmod-through-symlink

Conversation

@Angadi56

Copy link
Copy Markdown

copy_file recreates a source symlink at the destination, but the following set_permissions used the stale pre-copy symlink check, so chmod followed the new link and changed its target's mode outside the copied tree; it now re-checks dest.is_symlink() before setting the mode, matching GNU cp. Fixes #12191.

@github-actions

github-actions Bot commented Jul 18, 2026

Copy link
Copy Markdown

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/io-errors (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/pr/bounded-memory (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/expand/bounded-memory is now passing!
Congrats! The gnu test tests/rm/many-dir-entries-vs-OOM is now passing!

@codspeed-hq

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 339 untouched benchmarks
⏩ 46 skipped benchmarks1


Comparing Angadi56:cp-no-chmod-through-symlink (dc85181) with main (38f696d)

Open in CodSpeed

Footnotes

  1. 46 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

Copy link
Copy Markdown
Contributor

Since dest_is_symlink is stale by this point anyway, just use if !dest.is_symlink() - that's what this line was before #6190, and it also fixes cp --remove-destination onto a symlink (we give 600, GNU gives 664).
Also please add a test for it: ln -s target dst; cp --remove-destination src dst should leave dst at 664, not 600.

Drop the stale pre-copy dest_is_symlink from the guard and check
dest.is_symlink() alone, as this line did before uutils#6190. Besides not
chmodding through a freshly copied symlink, this also applies the
mode when --remove-destination replaces a symlink with a regular
file, matching GNU cp.
@Angadi56

Copy link
Copy Markdown
Author

Done, switched the guard to a plain !dest.is_symlink() and added the --remove-destination test. One note on the numbers: on macOS the clonefile path clones the source mode, so a 664 source ends up at 664 even without the final chmod. I set the test up as source 664 with umask 022, where the correct result is 644, so both wrong outcomes (600 from the restrictive create on Linux, 664 from the clone on macOS) fail it. Verified it fails with the old check on this machine, and the full cp suite is green.

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.

cp -r mutates symlink targets' permissions on macOS (chmod follows symlink to source)

2 participants