fs::copy() unix: set file mode early

[haraldh]

A convenience method like fs::copy() should try to prevent pitfalls a
normal user doesn't think about.

In case of an empty umask, setting the file mode early prevents
temporarily world readable or even writeable files,
because the default mode is 0o666.

In case the target is a named pipe or special device node, setting the
file mode can lead to unwanted side effects, like setting permissons on
/dev/stdout or for root setting permissions on /dev/null.

copy_file_range() returns EINVAL, if the destination is a FIFO/pipe or
a device like "/dev/null", so fallback to io::copy, too.

Fixes: #26933
Fixed: #37885

[rust-highfive]

r? @withoutboats

(rust_highfive has picked a reviewer for you, use r? to override)

[haraldh]

r? @alexcrichton

[bors]

☔ The latest upstream changes (presumably #58208) made this pull request unmergeable. Please resolve the merge conflicts.

[haraldh]

new refined version with better comments and logic

[rust-highfive]

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:0a8d1876:start=1551426417322061806,finish=1551426536556350387,duration=119234288581
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout
Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[00:04:51] Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:04:57] error: unused `core::result::Result` that must be used
[00:04:57] --> src/libstd/sys/unix/fs.rs:903:9
[00:04:57] |
[00:04:57] 903 | writer.set_permissions(perm);
[00:04:57] |
[00:04:57] = note: `-D unused-must-use` implied by `-D warnings`
[00:04:57] = note: `-D unused-must-use` implied by `-D warnings`
[00:04:57] = note: this `Result` may be an `Err` variant, which should be handled
[00:04:57] error: aborting due to previous error
[00:04:57] [00:04:57] error: Could not compile `std`.
[00:04:57] 

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[alexcrichton]

@bors: r+

[bors]

📌 Commit fb98ca7 has been approved by alexcrichton

[rust-highfive]

Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[haraldh]

Hmpf, this filesystem test fails on asmjs-unknown-emscripten Does asmjs even handle files?

[haraldh]

added

// ignore-emscripten no files

to test

[haraldh]

removed the ignore and added

// If `dummy_file` does not exist, then we might get another unrelated error
#[cfg(not(target_os = "emscripten"))] let dummy_file = std::env::current_exe().unwrap();
#[cfg(target_os = "emscripten")] let dummy_file = "a";

[haraldh]

Another alternative would be to use TempDir as src/test/run-pass-fulldeps/rename-directory.rs does.

[haraldh]

Or just RUST_TEST_TMPDIR as src/test/run-pass-fulldeps/create-dir-all-bare.rs

[alexcrichton]

Hm I wonder if this test could just be ignored on emscripten? You can just add a #[cfg(not(...))] annotation to the whole test, and it's not too important to bend over backwards to get it running on emscripten.

[haraldh]

Hm I wonder if this test could just be ignored on emscripten? You can just add a #[cfg(not(...))] annotation to the whole test, and it's not too important to bend over backwards to get it running on emscripten.

Basically I did that with the latest version. For emscripten it's the old unchanged test (which should pass then) and for all the others it is hopefully picking up the current executable as the test file.

[alexcrichton]

Oh sure yeah and this might pass, but it's pretty jarring to be reading a test and randomly see a conditional statement for one particular platform. It's pretty common, however, to ignore tests on platforms for various reasons, so I think it's best to just ignore the whole test

[haraldh]

Oh sure yeah and this might pass, but it's pretty jarring to be reading a test and randomly see a conditional statement for one particular platform. It's pretty common, however, to ignore tests on platforms for various reasons, so I think it's best to just ignore the whole test

I didn't want to take away the test for emscripten, because it seems to pass in the past.

[alexcrichton]

Let's please ignore the test for emscripten, because we don't really need to keep tests running that just happened to pass before.

[haraldh]

Let's please ignore the test for emscripten, because we don't really need to keep tests running that just happened to pass before.

here we go, although that contradicts my understanding of a CI

[alexcrichton]

@bors: r+

[bors]

📌 Commit cf8347b has been approved by alexcrichton

[haraldh]

Halleluja 😀