cp: FIxed permission race when creating files #10204

max-amb · 2026-01-12T16:31:43Z

github-actions · 2026-01-12T16:49:47Z

GNU testsuite comparison:

GNU test failed: tests/cp/thru-dangling. tests/cp/thru-dangling is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/shuf/shuf-reservoir (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/sort/sort-stale-thread-mem (fails in this run but passes in the 'main' branch)

max-amb · 2026-01-18T00:51:26Z

I realise this is rather untenable right now, so I have dug up some logs to show the change in action, here is gnu cp:

openat(AT_FDCWD, "/tmp/b.txt", O_RDONLY|O_PATH|O_DIRECTORY) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY) = 3
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_EXCL, 0644) = 8
+++ exited with 0 +++

and when b.txt already exists:

openat(AT_FDCWD, "/tmp/b.txt", O_RDONLY|O_PATH|O_DIRECTORY) = -1 ENOTDIR (Not a directory)
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY) = 3
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_TRUNC) = 8
+++ exited with 0 +++

Here is the implementations strace

penat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0600) = 8
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 9
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0100644) = 12
fchmod(12, 0100644)                     = 0
chmod("/tmp/b.txt", 0644)               = 0
+++ exited with 0 +++

Note that the first 3 reads of a.txt are for other checks and opening the file and haven't been changed in this PR. The only difference between the current implementation and this implementation is in the initial opening of b.txt which is now

openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0600) = 8

instead of

openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 8

on main.

Now when we already have b.txt, in main right now we have

openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0666) = 8
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 9
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0100644) = 12
fchmod(12, 0100644)                     = 0
chmod("/tmp/b.txt", 0100644)            = 0
+++ exited with 0 +++

which opens b.txt with 666 and tries to ioctl clone it over, this is insecure as it would allow other users to read the file before the data is fully copied over which may not be intended. The later open is done by fs::copy as a fallback. In contrast here is the new implementation.

openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 3
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0600) = -1 EEXIST (File exists)
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_TRUNC|O_CLOEXEC) = 8
fchmod(8, 0600)                         = 0
openat(AT_FDCWD, "/tmp/a.txt", O_RDONLY|O_CLOEXEC) = 9
openat(AT_FDCWD, "/tmp/b.txt", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0100644) = 12
fchmod(12, 0100644)                     = 0
chmod("/tmp/b.txt", 0100644)            = 0
+++ exited with 0 +++

By changing the files permissions we ensure that the file cannot be read while data may be being copied in. This permission is later loosened to the correct permission.

@sylvestre I hope that this makes it less hand-wavy :)

src/uu/cp/src/platform/linux.rs

sylvestre · 2026-01-18T16:39:08Z

src/uu/cp/src/platform/linux.rs

+            } else {
+                // If a symlink exists in the position we want to write the file to, and it symlinks to
+                // a nonexistent file, we should just overwrite the symlink
+                let nonexistent_file = std::fs::read_link(&dest)?;


Dangling symlink handling is incorrect. This creates a file at the symlink target instead of replacing the symlink itself, causing test failures. Should remove the symlink first with std::fs::remove_file(&dest)? before creating new file at &dest

I disagree, if we look at test_copy_through_dangling_symlink_posixly_correct:

#[test] fn test_copy_through_dangling_symlink_posixly_correct() { let (at, mut ucmd) = at_and_ucmd!(); at.touch("file"); at.write("file", "content"); at.symlink_file("nonexistent", "target"); ucmd.arg("file") .arg("target") .env("POSIXLY_CORRECT", "1") .succeeds(); assert!(at.file_exists("nonexistent")); let contents = at.read("nonexistent"); assert_eq!(contents, "content"); }

we see that the file should be put at the symlink.

src/uu/cp/src/platform/linux.rs

sylvestre · 2026-01-18T16:40:12Z

and it needs tests :)

max-amb · 2026-01-19T12:41:55Z

and it needs tests :)

Specific tests are at test_cp_to_existing_file_permissions and test_copy_through_dangling_symlink_posixly_correct. The only intended change of the PR is during the process of opening files, the end result is the same.

codspeed-hq · 2026-01-19T13:01:34Z

CodSpeed Performance Report

Merging this PR will degrade performance by 16.32%

_{Comparing max-amb:cp_perm_race (c15ae3d) with main (425b232)}

Summary

⚡ 6 improved benchmarks
❌ 3 regressed benchmarks
✅ 273 untouched benchmarks
⏩ 38 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Memory	`dd_copy_partial`	127.7 KB	132.1 KB	-3.29%
⚡	Memory	`sort_unique_locale[500000]`	36.9 MB	33 MB	+11.83%
❌	Memory	`sort_long_line[160000]`	716.7 KB	856.4 KB	-16.32%
⚡	Memory	`sort_accented_data[500000]`	25.5 MB	21.6 MB	+18.11%
⚡	Memory	`sort_key_field[500000]`	32.8 MB	29.2 MB	+12.53%
⚡	Memory	`sort_numeric[500000]`	48.7 MB	46 MB	+5.8%
⚡	Memory	`sort_ascii_only[500000]`	25.5 MB	21.9 MB	+16.76%
⚡	Memory	`sort_mixed_data[500000]`	25.8 MB	22.4 MB	+15.2%
❌	Memory	`cp_large_file[16]`	115.4 KB	119.9 KB	-3.75%

38 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. ↩

github-actions · 2026-01-19T13:08:12Z

GNU testsuite comparison:

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

github-actions · 2026-01-19T14:17:51Z

GNU testsuite comparison:

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

max-amb added 2 commits January 12, 2026 16:24

cp: Fixed permission race for creating files

2fa1e38

Merge branch 'uutils:main' into cp_perm_race

905d821

max-amb marked this pull request as draft January 12, 2026 16:55

cp: Fixed handling of dangling symlinks

be073dc

max-amb marked this pull request as ready for review January 14, 2026 18:43