Repository: rust-lang/cargo (rust)

Issue:
`cargo add` changes file permissions of `Cargo.toml` to 600
### Problem

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.

### Steps

```bash
$ mkdir cargo-add-test && cd cargo-add-test
$ cargo init
$ ls -l
-rw-rw-r-- 1 user user   85 May  9 23:09 Cargo.toml
drwxrwxr-x 2 user user 4096 May  9 23:09 src
$ cargo add libc
$ ls -l
-rw-rw-r-- 1 user user  376 May  9 23:09 Cargo.lock
-rw------- 1 user user  102 May  9 23:09 Cargo.toml
drwxrwxr-x 2 user user 4096 May  9 23:09 src
$ umask
0002
```

### Possible Solution(s)

I'm guessing that Cargo replaces the existing file with a new file, and this is why the permissions change. In this case, I think it would be better to create the file with the original user/group/world permissions (ignoring things like suid permissions), ideally bypassing the umask as well.

**Edit:** It appears to use the tempfile crate, which [uses 600 by default](https://docs.rs/tempfile/3.10.1/tempfile/struct.Builder.html#method.permissions) on Linux.

https://github.com/rust-lang/cargo/blob/4de0094ac78743d2c8ff682489e35c8a7cafe8e4/crates/cargo-util/src/paths.rs#L186-L194

### Notes

_No response_

### Version

```text
cargo 1.80.0-nightly (0ca60e940 2024-05-08)
release: 1.80.0-nightly
commit-hash: 0ca60e940821c311c9b25a6423b59ccdbcea218f
commit-date: 2024-05-08
host: x86_64-unknown-linux-gnu
libgit2: 1.7.2 (sys:0.18.3 vendored)
libcurl: 8.6.0-DEV (sys:0.4.72+curl-8.6.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Ubuntu 20.4.0 (focal) [64-bit]
```


Pull request description:
Preserve file permissions on unix during `write_atomic`
### What does this PR try to resolve?

Fixes #13896.

> 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](https://docs.rs/tempfile/3.10.1/tempfile/struct.Builder.html#windows-and-others), 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:

- [`migrate_manifests`](https://github.com/rust-lang/cargo/blob/4de0094ac78743d2c8ff682489e35c8a7cafe8e4/src/cargo/ops/fix.rs#L340)
- [`update_manifest_with_new_member`](https://github.com/rust-lang/cargo/blob/4de0094ac78743d2c8ff682489e35c8a7cafe8e4/src/cargo/ops/cargo_new.rs#L1008)
- [`LocalManifest::write`](https://github.com/rust-lang/cargo/blob/4de0094ac78743d2c8ff682489e35c8a7cafe8e4/src/cargo/util/toml_mut/manifest.rs#L299)
- [`gc_workspace`](https://github.com/rust-lang/cargo/blob/4de0094ac78743d2c8ff682489e35c8a7cafe8e4/src/bin/cargo/commands/remove.rs#L274)

### How should we test and review this PR?

Unit test was added (`cargo test -p cargo-util write_atomic_permissions`).

Patch:
diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs
index 9cfb73cdd..7a19479ad 100644
--- a/crates/cargo-util/src/paths.rs
+++ b/crates/cargo-util/src/paths.rs
@@ -4,7 +4,7 @@ use anyhow::{Context, Result};
 use filetime::FileTime;
 use std::env;
 use std::ffi::{OsStr, OsString};
-use std::fs::{self, File, Metadata, OpenOptions};
+use std::fs::{self, File, Metadata, OpenOptions, Permissions};
 use std::io;
 use std::io::prelude::*;
 use std::iter;
@@ -185,10 +185,34 @@ pub fn write<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()>
 /// write_atomic uses tempfile::persist to accomplish atomic writes.
 pub fn write_atomic<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> {
     let path = path.as_ref();
+
+    // On unix platforms, get the permissions of the original file. Copy only the user/group/other
+    // read/write/execute permission bits. The tempfile lib defaults to an initial mode of 0o600,
+    // and we'll set the proper permissions after creating the file.
+    #[cfg(unix)]
+    let perms = path.metadata().ok().map(|meta| {
+        use std::os::unix::fs::PermissionsExt;
+
+        // these constants are u16 on macOS
+        let mask = u32::from(libc::S_IRWXU | libc::S_IRWXG | libc::S_IRWXO);
+        let mode = meta.permissions().mode() & mask;
+
+        Permissions::from_mode(mode)
+    });
+
     let mut tmp = TempFileBuilder::new()
         .prefix(path.file_name().unwrap())
         .tempfile_in(path.parent().unwrap())?;
     tmp.write_all(contents.as_ref())?;
+
+    // On unix platforms, set the permissions on the newly created file. We can use fchmod (called
+    // by the std lib; subject to change) which ignores the umask so that the new file has the same
+    // permissions as the old file.
+    #[cfg(unix)]
+    if let Some(perms) = perms {
+        tmp.as_file().set_permissions(perms)?;
+    }
+
     tmp.persist(path)?;
     Ok(())
 }
@@ -823,6 +847,32 @@ mod tests {
         assert_eq!(contents, original_contents);
     }
 
+    #[test]
+    #[cfg(unix)]
+    fn write_atomic_permissions() {
+        use std::os::unix::fs::PermissionsExt;
+
+        let original_perms = std::fs::Permissions::from_mode(u32::from(
+            libc::S_IRWXU | libc::S_IRGRP | libc::S_IWGRP | libc::S_IROTH,
+        ));
+
+        let tmp = tempfile::Builder::new().tempfile().unwrap();
+
+        // need to set the permissions after creating the file to avoid umask
+        tmp.as_file()
+            .set_permissions(original_perms.clone())
+            .unwrap();
+
+        // after this call, the file at `tmp.path()` will not be the same as the file held by `tmp`
+        write_atomic(tmp.path(), "new").unwrap();
+        assert_eq!(std::fs::read_to_string(tmp.path()).unwrap(), "new");
+
+        let new_perms = std::fs::metadata(tmp.path()).unwrap().permissions();
+
+        let mask = u32::from(libc::S_IRWXU | libc::S_IRWXG | libc::S_IRWXO);
+        assert_eq!(original_perms.mode(), new_perms.mode() & mask);
+    }
+
     #[test]
     fn join_paths_lists_paths_on_error() {
         let valid_paths = vec!["/testing/one", "/testing/two"];


Test patch:
diff --git a/tests/testsuite/bench.rs b/tests/testsuite/bench.rs
index 85efeaba5..eb9c92c1e 100644
--- a/tests/testsuite/bench.rs
+++ b/tests/testsuite/bench.rs
@@ -267,9 +267,9 @@ fn many_similar_names() {
         .build();
 
     p.cargo("bench")
-        .with_stdout_contains("test bin_bench ... bench:[..]")
-        .with_stdout_contains("test lib_bench ... bench:[..]")
-        .with_stdout_contains("test bench_bench ... bench:[..]")
+        .with_stdout_contains("test bin_bench ... bench:           0 ns/iter (+/- 0)")
+        .with_stdout_contains("test lib_bench ... bench:           0 ns/iter (+/- 0)")
+        .with_stdout_contains("test bench_bench ... bench:           0 ns/iter (+/- 0)")
         .run();
 }