Could you please add a test to make sure we don't regress? Thanks

Could you please add a test to make sure we don't regress? Thanks

There was indeed a regression: when the fs::copy call failed, the created destination file was not removed. I just pushed a new patch that fixes this.

I added a test to make sure that no file will be created when fs::copy fails (e.g., the source file cannot be read), and if it succeed, the destination file's mode is set correctly. This makes sure that it works as before no matter the copy succeeds or not.

I'm not sure how to use a test case to check whether the new file is created in mode 0600, since the creation is in the middle of an internal function, but the strace output confirms so If you have any suggestions please let me know. Thanks!

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

GNU testsuite comparison:

Skipping an intermittent issue tests/rm/rm1 (passes in this run but fails in the 'main' branch)

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?
Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

thanks and sorry for the latency

This did not fix the problem -- the magic fchmod to 644 is still there making the file temporarily accessible.

Moreover this introduces a regression where the target file gets unlinked even if the source does not exist. I verified install from gnu coreutils just exits in such a case without messing with the target.

below I'm annotating strace output from a setup as in the bug report, installing from /tmp/srcdir/foo to /tmp/dstdir/foo. there is tons of noise there which I elided for clarity.

unlink("/tmp/dstdir/foo")               = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/tmp/dstdir/foo", O_WRONLY|O_CREAT|O_EXCL|O_CLOEXEC, 0600) = 3
close(3)                                = 0

This executes regardless if /tmp/srcdir/foo exists or can be copied, meaning by this point the content of the target is whacked. The new file does however have 600 perms. The file should remain unaltered if src can't be copied.

openat(AT_FDCWD, "/tmp/srcdir/foo", O_RDONLY|O_CLOEXEC) = 3
statx(3, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0644, stx_size=0, ...}) = 0
openat(AT_FDCWD, "/tmp/dstdir/foo", O_WRONLY|O_CREAT|O_TRUNC|O_CLOEXEC, 0100644) = 4
statx(4, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0600, stx_size=0, ...}) = 0
fchmod(4, 0100644)                      = 0

Note the target is accessible through fd 4. Rust code just chmoded it to 644, making it again readable by everyone who can access the directory and defeating the original open.

copy_file_range(3, NULL, 4, NULL, 1073741824, 0) = 0
[..]
chmod("/tmp/dstdir/foo", 0700)          = 0

Only now the file goes back to the expected state.

All in all the fs::copy API is probably fine for some cases, but is a total non-starter for serious programs. I failed to find a variant which accepts file handles instead, Rust needs to grow one.

ok, thanks for the very quick reaction. I will revert it then.

Ye makes sense.

I'm going to prod the rust people about better fs::copy so that this can be properly fixed.

@mjguzik Thanks a lot for the feedback.

I found that rust has a variant accepting file handles: https://doc.rust-lang.org/beta/std/io/fn.copy.html

this will do it