Skip to content

Conversation

@max-amb
Copy link
Contributor

@max-amb max-amb commented Jan 12, 2026

Fixes #10011.

@github-actions
Copy link

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 max-amb marked this pull request as draft January 12, 2026 16:55
@max-amb max-amb marked this pull request as ready for review January 14, 2026 18:43
@max-amb
Copy link
Contributor Author

max-amb commented Jan 18, 2026

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

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 permission race

1 participant