cp: FIxed permission race when creating files by max-amb · Pull Request #10204 · uutils/coreutils

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: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

Merging this PR will not alter performance

✅ 288 untouched benchmarks
⏩ 38 skipped benchmarks¹

_{Comparing max-amb:cp_perm_race (21eaa42) with main (8eb77a0)²}

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. ↩
No successful run was found on main (7acacd0) during the generation of this report, so 8eb77a0 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

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?

github-actions · 2026-01-22T13:00:12Z

GNU testsuite comparison:

Skipping an intermittent issue tests/shuf/shuf-reservoir (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/sort/sort-stale-thread-mem (passes in this run but fails in the 'main' branch)

max-amb · 2026-01-22T13:03:23Z

ok so how I was handling truncation is the issue.

github-actions · 2026-01-28T11:05:16Z

GNU testsuite comparison:

GNU test failed: tests/cp/sparse. tests/cp/sparse is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/cp/sparse-2. tests/cp/sparse-2 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/cp/sparse-extents-2. tests/cp/sparse-extents-2 is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/cp/sparse-to-pipe. tests/cp/sparse-to-pipe 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)

sylvestre · 2026-01-28T11:14:32Z

it regressed a bunch of tests, sorry!

max-amb · 2026-01-28T16:06:29Z

Ah yeah I think I see why now, the rewind as a replacement for truncate would mean sparse files are just layered on with any 0 spaces being filled with the old file.

github-actions · 2026-01-28T16:21:44Z

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-28T18:05:22Z

GNU testsuite comparison:

Skipping an intermittent issue tests/shuf/shuf-reservoir (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/sort/sort-stale-thread-mem (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

github-actions · 2026-01-29T09:39:18Z

GNU testsuite comparison:

Skipping an intermittent issue tests/shuf/shuf-reservoir (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/sort/sort-stale-thread-mem (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/basenc/bounded-memory is now passing!

github-actions · 2026-01-30T11:49:17Z

GNU testsuite comparison:

Skipping an intermittent issue tests/shuf/shuf-reservoir (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/sort/sort-stale-thread-mem (passes in this run but fails in the 'main' branch)
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/basenc/bounded-memory is now passing!

sylvestre · 2026-02-04T21:36:08Z

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

+                let mut desired_permissions = dest_metadata.permissions();
+                // This will be reset to the correct permissions later, this is defensive as it is
+                // the most restrictive
+                let mut dst = OpenOptions::new().write(true).open(&dest)?;


The file is opened with existing permissions before they're tightened to 0o600. The permissions should be set on the file path using std::fs::set_permissions() BEFORE opening the file handle. no ?

For this, std::fs::set_permissions pre open sets the files permissions to writeable when the file is readonly and then it opens it without issue (because now it is writeable). This isn't the desired behaviour

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

…erm_race

github-actions · 2026-02-11T12:15:07Z

GNU testsuite comparison:

Congrats! The gnu test tests/pr/bounded-memory is no longer failing!

github-actions · 2026-02-16T10:21:08Z

GNU testsuite comparison:

GNU test failed: tests/cut/bounded-memory. tests/cut/bounded-memory is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/seq/seq-io-errors. tests/seq/seq-io-errors is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/tail/tail-n0f is no longer failing!

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

sylvestre reviewed Jan 18, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

sylvestre reviewed Jan 18, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

sylvestre reviewed Jan 18, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

sylvestre reviewed Jan 18, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

sylvestre reviewed Jan 18, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

sylvestre reviewed Jan 18, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

sylvestre reviewed Jan 18, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

max-amb added 3 commits January 19, 2026 11:08

Cleaned up error handling

6755e71

Explicit handling for NotFound

66bdff2

fixed permission race due to truncation pre modification of perms

d47bde7

Merge branch 'uutils:main' into cp_perm_race

c15ae3d

Checking if truncation is reason for failing GNU test

9be9306

max-amb added 2 commits January 22, 2026 13:24

Implemented alternative truncation method

e8bf148

Merge branch 'uutils:main' into cp_perm_race

ebb361f

max-amb requested a review from sylvestre January 22, 2026 14:50

Merge branch 'uutils:main' into cp_perm_race

4c97cbe

Reset file to 0's for sparse

7d80334

Added file check

96befc2

Merge branch 'uutils:main' into cp_perm_race

ab21714

Merge branch 'uutils:main' into cp_perm_race

a34b86e

sylvestre reviewed Feb 4, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Show resolved Hide resolved

sylvestre reviewed Feb 4, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

sylvestre reviewed Feb 4, 2026

View reviewed changes

src/uu/cp/src/platform/linux.rs Outdated Show resolved Hide resolved

max-amb added 3 commits February 4, 2026 23:49

setting permissions pre-opening

428ac3a

Merge branch 'cp_perm_race' of github.com:max-amb/coreutils into cp_p…

1c26ffe

…erm_race

Moved create_new_file logic into helper function

ba2eee7

max-amb marked this pull request as draft February 5, 2026 09:04

Fixed permission logic

6dbab83

max-amb marked this pull request as ready for review February 5, 2026 11:06

max-amb requested a review from sylvestre February 5, 2026 15:05

Merge branch 'main' into cp_perm_race

0be55f1

Merge branch 'uutils:main' into cp_perm_race

21eaa42

Uh oh!

Conversation

max-amb commented Jan 12, 2026

Uh oh!

github-actions bot commented Jan 12, 2026

Uh oh!

max-amb commented Jan 18, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sylvestre commented Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

max-amb commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codspeed-hq bot commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

github-actions bot commented Jan 19, 2026

Uh oh!

github-actions bot commented Jan 19, 2026

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

max-amb commented Jan 22, 2026

Uh oh!

github-actions bot commented Jan 28, 2026

Uh oh!

sylvestre commented Jan 28, 2026

Uh oh!

max-amb commented Jan 28, 2026

Uh oh!

github-actions bot commented Jan 28, 2026

Uh oh!

github-actions bot commented Jan 28, 2026

Uh oh!

github-actions bot commented Jan 29, 2026

Uh oh!

github-actions bot commented Jan 30, 2026

Uh oh!

sylvestre Feb 4, 2026

Choose a reason for hiding this comment

Uh oh!

max-amb Feb 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 11, 2026

Uh oh!

github-actions bot commented Feb 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Comments

sylvestre commented Jan 18, 2026 •

edited

Loading

max-amb commented Jan 19, 2026 •

edited

Loading

codspeed-hq bot commented Jan 19, 2026 •

edited

Loading