Skip to content

cp: keep permissions - #14176

Open
mtvb wants to merge 5 commits into
uutils:mainfrom
mtvb:cp_permissions_fix
Open

cp: keep permissions#14176
mtvb wants to merge 5 commits into
uutils:mainfrom
mtvb:cp_permissions_fix

Conversation

@mtvb

@mtvb mtvb commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

delete_dest_if_needed_and_allowed in cp.rs decided whether "cp -f" needs to recreate an existing destination by checking fs::metadata(dest)?.permissions().readonly().

Its only a mode-bit check, and ignores that a privileged process (root) can write to a file regardless of its mode bits, so it always treated a chmod 000 destination as needs recreating.

Fixes #14175

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skipping an intermittent issue tests/tail/tail-n0f (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/csplit/csplit-heap is now passing!
Congrats! The gnu test tests/seq/seq-epipe is now passing!

@sylvestre

Copy link
Copy Markdown
Contributor

GNU test failed: tests/cp/special-f. tests/cp/special-f is passing on 'main'. Maybe you have to rebase?

you probably regressed this test

@mtvb mtvb changed the title cp: keep permissions #14175 cp: keep permissions Aug 27, 2026
@mtvb

mtvb commented Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Handling for non-regular destinations was missing.
Added them, see new commit.

@codspeed-hq

codspeed-hq Bot commented Aug 27, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 11 untouched benchmarks
⏩ 400 skipped benchmarks1


Comparing mtvb:cp_permissions_fix (4d6e997) with main (0d8310c)

Open in CodSpeed

Footnotes

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

Comment thread tests/by-util/test_cp.rs Outdated
#[test]
#[cfg(unix)]
fn test_cp_f_i_verbose_non_writeable_destination_y() {
use uucore::process::geteuid;

@oech3 oech3 Aug 27, 2026

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.

Please use rustix directly. We will remove it soon.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved over to rustix.

test_cp_f_i_verbose_non_writeable_destination_y has been changed to check for root first, as a privileged process can write to a 000-mode file regardless.

Moved this testcase to rustix also.

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: derivation from gnu cp behavior

3 participants