Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 28, 2019
Merged

Conversation

haraldh
Copy link
Contributor

@haraldh haraldh commented Feb 28, 2019

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
Copy link
Collaborator

r? @withoutboats

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 28, 2019
@haraldh
Copy link
Contributor Author

haraldh commented Feb 28, 2019

r? @alexcrichton

@bors
Copy link
Contributor

bors commented Feb 28, 2019

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

src/libstd/sys/unix/fs.rs Outdated Show resolved Hide resolved
src/libstd/sys/unix/fs.rs Outdated Show resolved Hide resolved
src/libstd/sys/unix/fs.rs Outdated Show resolved Hide resolved
src/libstd/sys/unix/fs.rs Outdated Show resolved Hide resolved
@haraldh haraldh force-pushed the fs_copy_fix branch 2 times, most recently from c7a6814 to d329ec4 Compare March 1, 2019 07:45
@haraldh
Copy link
Contributor Author

haraldh commented Mar 1, 2019

new refined version with better comments and logic

@rust-highfive
Copy link
Collaborator

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
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 1, 2019

📌 Commit fb98ca7 has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 1, 2019
haraldh added a commit to haraldh/rust-1 that referenced this pull request Mar 4, 2019
same fix as commit fb98ca7
PR: rust-lang#58803

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.
Centril added a commit to Centril/rust that referenced this pull request Mar 10, 2019
fs::copy() linux: set file mode early

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: rust-lang#26933
Fixed: rust-lang#37885
Centril added a commit to Centril/rust that referenced this pull request Mar 10, 2019
fs::copy() linux: set file mode early

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: rust-lang#26933
Fixed: rust-lang#37885
Centril added a commit to Centril/rust that referenced this pull request Mar 10, 2019
fs::copy() linux: set file mode early

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: rust-lang#26933
Fixed: rust-lang#37885
Centril added a commit to Centril/rust that referenced this pull request Mar 10, 2019
fs::copy() linux: set file mode early

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: rust-lang#26933
Fixed: rust-lang#37885
@rust-highfive
Copy link
Collaborator

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)

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2019
@haraldh
Copy link
Contributor Author

haraldh commented Mar 23, 2019

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

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.

Use `fcopyfile` on MacOS instead of `copyfile`.

Fixes: rust-lang#26933
Fixed: rust-lang#37885
@haraldh
Copy link
Contributor Author

haraldh commented Mar 23, 2019

added

// ignore-emscripten no files

to test

@haraldh
Copy link
Contributor Author

haraldh commented Mar 23, 2019

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
Copy link
Contributor Author

haraldh commented Mar 23, 2019

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

@haraldh
Copy link
Contributor Author

haraldh commented Mar 23, 2019

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

@alexcrichton
Copy link
Member

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
Copy link
Contributor Author

haraldh commented Mar 25, 2019

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
Copy link
Member

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
Copy link
Contributor Author

haraldh commented Mar 26, 2019

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
Copy link
Member

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
Copy link
Contributor Author

haraldh commented Mar 26, 2019

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
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 26, 2019

📌 Commit cf8347b has been approved by alexcrichton

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 28, 2019
fs::copy() unix: set file mode early

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: rust-lang#26933
Fixed: rust-lang#37885
bors added a commit that referenced this pull request Mar 28, 2019
Rollup of 12 pull requests

Successful merges:

 - #57987 (Fix some AArch64 typos)
 - #58581 (Refactor generic parameter encoder functions)
 - #58803 (fs::copy() unix: set file mode early)
 - #58848 (Prevent cache issues on version updates)
 - #59198 (Do not complain about unmentioned fields in recovered patterns)
 - #59351 (Include llvm-ar with llvm-tools component)
 - #59413 (HirIdify hir::ItemId)
 - #59441 (Remove the block on natvis for lld-link.)
 - #59448 (Use consistent phrasing for all macro summaries)
 - #59456 (Add documentation about `for` used as higher ranked trait bounds)
 - #59472 (Document that `std::io::BufReader` discards contents on drop)
 - #59474 (Fix link capitalization in documentation of std::io::BufWriter.)

Failed merges:

r? @ghost
@bors bors merged commit cf8347b into rust-lang:master Mar 28, 2019
@haraldh haraldh deleted the fs_copy_fix branch March 28, 2019 12:31
@haraldh
Copy link
Contributor Author

haraldh commented Mar 28, 2019

Halleluja 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants