-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Preserve file permissions on unix during write_atomic
#13898
Conversation
r? @weihanglo rustbot has assigned @weihanglo. Use |
crates/cargo-util/src/paths.rs
Outdated
// On unix platforms, use the same permissions as the original file. Copy only the | ||
// user/group/other read/write/execute permission bits. While we need to also set the | ||
// permissions again later to bypass the umask, we still need to set the permissions here as | ||
// well so that we don't create a more-permissive file than the original. The tempfile lib says |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default seems to be 0o600, which is pretty safe I believe. Do we really need to set permission for tempfiles?
Granted, if the Cargo.toml
was readonly, maybe cargo add
should fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove that part and just use the default 600 permissions to simplify it. tempfile could change their default in the future, but that's probably unlikely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that could simplify the logic a bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It now only sets the permissions once after the file has been created. The file will be created with tempfile's default of 600.
@@ -823,6 +857,30 @@ mod tests { | |||
assert_eq!(contents, original_contents); | |||
} | |||
|
|||
#[test] | |||
#[cfg(unix)] | |||
fn write_atomic_permissions() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have this test in the first commit showing the problematic behavior, and the next commit fixes both the test and the behavior? By doing so it's a bit clearer to reviewers to just read the diff and understand what has been changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reorganized it so that the first commit adds the tests, and the second commit adds the write_atomic
changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in one of the other threads, I fixed this so that the first commit adds the test for the existing behaviour, and the second commit fixes the behaviour and updates the test for the new behaviour.
// permissions above, they were subject to the umask. Now that the file is created, we can use | ||
// fchmod (called by the std lib; subject to change) to set the permissions which ignores the | ||
// umask so that the new file has the same permissions as the old file. | ||
#[cfg(unix)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe on Windows we could try setting read-only if it was read-only?
I don't know whether it would fail or not when replacing a read-only file though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The standard library does not actually change any permissions on Windows. It sets the read-only attribute but from a security pov this is mostly useless as anyone with write permissions could just unset it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, I guess it might make sense to preserve some attributes in any case. Seems not as important as perms though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, I guess it might make sense to preserve some attributes in any case. Seems not as important as perms though.
True. I was thinking from that angle. Just a nice-to-have, not a blocker.
BTW, how could you always notice there is a Windows related issue happening, even when nobody pinged you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha, I do miss things. But often people mention something to me privately or on discord. In this case I just happened to be browsing new PRs and this one looked interesting so I opened it then noticed a Windows thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added code to maintain the readonly property on non-unix platforms, but this doesn't work on Windows since unlike Linux you cannot delete or replace a read-only file on Windows. So when tempfile::persist is called it tries to replace the old read-only file (effectively deleting it), which fails with an "access denied" error.
I think there are workarounds, but we couldn't do it atomically without modifying the tempfile code or changing the readonly property on the original file. So I think it would be better to ignore Windows (and non-unix platforms). What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Go ahead :)
BTW, when I mentioned the commit organization, I meant something like this:
- The first commit asserts the bad behavior. CI is all green with this commit.
- The second commit fixes both bug and test. CI is still green, and the diff bewteen shows the test change so we're more confident it fixes the previously "bad" behavior.
- See fix: emit 1.77 syntax error only when msrv is incompatible #13808 as a reference.
Not really a hard requirement though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to delete read only files. Std has an issue open about renaming using "POSIX semantics" (rust-lang/rust#123985) so that may be changed in the future. Not that it helps in the here and now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah okay. I had borrowed a Windows computer and tried to delete a read-only file (and tried replacing a read-only file) on the command line with del
and move
and wasn't able to. But I have no idea what Windows APIs exist or what was being used by those commands. tempfile seems to use MoveFileExW
.
If you have any suggestions about how to make this work on Windows let me know. My Windows API knowledge is very old and I don't have a good way to test things on Windows other than using the CI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the non-unix code that tries to preserve the readonly property.
BTW, when I mentioned the commit organization, I meant something like this:
Ah sorry I misunderstood. I believe I fixed the commits now.
crates/cargo-util/src/paths.rs
Outdated
// On unix platforms, use the same permissions as the original file. Copy only the | ||
// user/group/other read/write/execute permission bits. While we need to also set the | ||
// permissions again later to bypass the umask, we still need to set the permissions here as | ||
// well so that we don't create a more-permissive file than the original. The tempfile lib says |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that could simplify the logic a bit.
7d35403
to
9103faf
Compare
Preseves u/g/o r/w/x permissions on unix platforms.
9103faf
to
36a63b4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Looks pretty good now. We shouldn't block this from merge.
@bors r+ |
☀️ Test successful - checks-actions |
Update cargo 6 commits in 4de0094ac78743d2c8ff682489e35c8a7cafe8e4..0de7f2ec6c39d68022e6b97a39559d2f4dbf3930 2024-05-09 16:09:22 +0000 to 2024-05-17 16:54:54 +0000 - Add special `check-cfg` lint config for the `unexpected_cfgs` lint (rust-lang/cargo#13913) - refactor: more comments and variable rename (rust-lang/cargo#13924) - test: set safe.directory for git repo in apache container (rust-lang/cargo#13920) - refactor: misc refactors for `ops::resolve` (rust-lang/cargo#13917) - Preserve file permissions on unix during `write_atomic` (rust-lang/cargo#13898) - Update benchmark formatting for new nightly (rust-lang/cargo#13901) r? ghost
What does this PR try to resolve?
Fixes #13896.
I'm not entirely sure how permissions are handled on Windows, but the tempfile lib doesn't seem to support them, so I haven't changed the behaviour on Windows.
Only the user/group/other read/write/execute permission bits are copied.
This PR sets the permissions
twiceonce:1. When creating the file. This has the umask applied, but means that we don't create a file that is more permissive than the original.2. After the file has been created. This doesn't apply the umask, resulting in the file having the same u/g/o r/w/x permissions as the original file.
Since this PR changes a util function, it has a wider scope than just changing the behaviour of
cargo add
andcargo remove
.write_atomic
is called from the following functions:migrate_manifests
update_manifest_with_new_member
LocalManifest::write
gc_workspace
How should we test and review this PR?
Unit test was added (
cargo test -p cargo-util write_atomic_permissions
).