Preserve file permissions on unix during `write_atomic` #13898

stevenengler · 2024-05-10T06:13:53Z

What does this PR try to resolve?

When you run cargo add, it changes the file permissions of Cargo.toml to 600 (user read+write only). This is a little bit painful when you're building the code as a different user than the user writing the code, for example if you're running the code in a container. This applies to cargo remove as well. I tested this behaviour on Cargo 1.78.0 and nightly.

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 ~~twice~~ once:
~~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 and cargo remove. write_atomic is called from the following functions:

How should we test and review this PR?

Unit test was added (cargo test -p cargo-util write_atomic_permissions).

rustbot · 2024-05-10T06:13:58Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

weihanglo · 2024-05-10T17:10:06Z

crates/cargo-util/src/paths.rs

+    // 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


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.

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.

Yeah that could simplify the logic a bit.

It now only sets the permissions once after the file has been created. The file will be created with tempfile's default of 600.

weihanglo · 2024-05-10T17:24:00Z

crates/cargo-util/src/paths.rs

@@ -823,6 +857,30 @@ mod tests {
        assert_eq!(contents, original_contents);
    }

+    #[test]
+    #[cfg(unix)]
+    fn write_atomic_permissions() {


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.

Reorganized it so that the first commit adds the tests, and the second commit adds the write_atomic changes.

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.

weihanglo · 2024-05-10T18:17:45Z

crates/cargo-util/src/paths.rs

+    // 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)]


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.

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.

I mean, I guess it might make sense to preserve some attributes in any case. Seems not as important as perms though.

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?

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.

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?

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.

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.

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.

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.

weihanglo · 2024-05-10T18:20:27Z

crates/cargo-util/src/paths.rs

+    // 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


Yeah that could simplify the logic a bit.

Preseves u/g/o r/w/x permissions on unix platforms.

weihanglo

Thanks! Looks pretty good now. We shouldn't block this from merge.

weihanglo · 2024-05-15T12:42:25Z

@bors r+

bors · 2024-05-15T12:42:27Z

📌 Commit 36a63b4 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2024-05-15T12:44:42Z

⌛ Testing commit 36a63b4 with merge 0ea330d...

bors · 2024-05-15T13:13:00Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 0ea330d to master...

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

rustbot assigned weihanglo May 10, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 10, 2024

stevenengler mentioned this pull request May 10, 2024

cargo add changes file permissions of Cargo.toml to 600 #13896

Closed

weihanglo reviewed May 10, 2024

View reviewed changes

stevenengler force-pushed the cargo-add-perms branch from 7d35403 to 9103faf Compare May 10, 2024 22:51

stevenengler added 2 commits May 14, 2024 22:48

test: write_atomic changes file permissions to 0o600 on unix

038ccd2

fix: preserve file permissions on unix during write_atomic

36a63b4

Preseves u/g/o r/w/x permissions on unix platforms.

stevenengler force-pushed the cargo-add-perms branch from 9103faf to 36a63b4 Compare May 15, 2024 02:51

weihanglo approved these changes May 15, 2024

View reviewed changes

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 May 15, 2024

bors merged commit 0ea330d into rust-lang:master May 15, 2024
21 checks passed

stevenengler deleted the cargo-add-perms branch May 15, 2024 20:40

weihanglo mentioned this pull request May 18, 2024

Update cargo rust-lang/rust#125237

Merged

rustbot added this to the 1.80.0 milestone May 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve file permissions on unix during `write_atomic` #13898

Preserve file permissions on unix during `write_atomic` #13898

stevenengler commented May 10, 2024 •

edited

rustbot commented May 10, 2024

weihanglo May 10, 2024

stevenengler May 10, 2024

weihanglo May 10, 2024

stevenengler May 10, 2024

weihanglo May 10, 2024

stevenengler May 10, 2024

stevenengler May 15, 2024

weihanglo May 10, 2024

ChrisDenton May 10, 2024

ChrisDenton May 10, 2024

weihanglo May 10, 2024

ChrisDenton May 10, 2024

stevenengler May 10, 2024 •

edited

weihanglo May 11, 2024

ChrisDenton May 11, 2024 •

edited

stevenengler May 11, 2024

stevenengler May 15, 2024 •

edited

weihanglo May 10, 2024

weihanglo left a comment

weihanglo commented May 15, 2024

bors commented May 15, 2024

bors commented May 15, 2024

bors commented May 15, 2024

Preserve file permissions on unix during write_atomic #13898

Preserve file permissions on unix during write_atomic #13898

Conversation

stevenengler commented May 10, 2024 • edited

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented May 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenengler May 10, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChrisDenton May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevenengler May 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weihanglo left a comment

Choose a reason for hiding this comment

weihanglo commented May 15, 2024

bors commented May 15, 2024

bors commented May 15, 2024

bors commented May 15, 2024

Preserve file permissions on unix during `write_atomic` #13898

Preserve file permissions on unix during `write_atomic` #13898

stevenengler commented May 10, 2024 •

edited

stevenengler May 10, 2024 •

edited

ChrisDenton May 11, 2024 •

edited

stevenengler May 15, 2024 •

edited