Skip to content

Commit 0ea330d

Browse files
committed
Auto merge of #13898 - stevenengler:cargo-add-perms, r=weihanglo
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`).
2 parents 2f17770 + 36a63b4 commit 0ea330d

File tree

1 file changed

+51
-1
lines changed

1 file changed

+51
-1
lines changed

crates/cargo-util/src/paths.rs

+51-1
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use anyhow::{Context, Result};
44
use filetime::FileTime;
55
use std::env;
66
use std::ffi::{OsStr, OsString};
7-
use std::fs::{self, File, Metadata, OpenOptions};
7+
use std::fs::{self, File, Metadata, OpenOptions, Permissions};
88
use std::io;
99
use std::io::prelude::*;
1010
use std::iter;
@@ -185,10 +185,34 @@ pub fn write<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()>
185185
/// write_atomic uses tempfile::persist to accomplish atomic writes.
186186
pub fn write_atomic<P: AsRef<Path>, C: AsRef<[u8]>>(path: P, contents: C) -> Result<()> {
187187
let path = path.as_ref();
188+
189+
// On unix platforms, get the permissions of the original file. Copy only the user/group/other
190+
// read/write/execute permission bits. The tempfile lib defaults to an initial mode of 0o600,
191+
// and we'll set the proper permissions after creating the file.
192+
#[cfg(unix)]
193+
let perms = path.metadata().ok().map(|meta| {
194+
use std::os::unix::fs::PermissionsExt;
195+
196+
// these constants are u16 on macOS
197+
let mask = u32::from(libc::S_IRWXU | libc::S_IRWXG | libc::S_IRWXO);
198+
let mode = meta.permissions().mode() & mask;
199+
200+
Permissions::from_mode(mode)
201+
});
202+
188203
let mut tmp = TempFileBuilder::new()
189204
.prefix(path.file_name().unwrap())
190205
.tempfile_in(path.parent().unwrap())?;
191206
tmp.write_all(contents.as_ref())?;
207+
208+
// On unix platforms, set the permissions on the newly created file. We can use fchmod (called
209+
// by the std lib; subject to change) which ignores the umask so that the new file has the same
210+
// permissions as the old file.
211+
#[cfg(unix)]
212+
if let Some(perms) = perms {
213+
tmp.as_file().set_permissions(perms)?;
214+
}
215+
192216
tmp.persist(path)?;
193217
Ok(())
194218
}
@@ -823,6 +847,32 @@ mod tests {
823847
assert_eq!(contents, original_contents);
824848
}
825849

850+
#[test]
851+
#[cfg(unix)]
852+
fn write_atomic_permissions() {
853+
use std::os::unix::fs::PermissionsExt;
854+
855+
let original_perms = std::fs::Permissions::from_mode(u32::from(
856+
libc::S_IRWXU | libc::S_IRGRP | libc::S_IWGRP | libc::S_IROTH,
857+
));
858+
859+
let tmp = tempfile::Builder::new().tempfile().unwrap();
860+
861+
// need to set the permissions after creating the file to avoid umask
862+
tmp.as_file()
863+
.set_permissions(original_perms.clone())
864+
.unwrap();
865+
866+
// after this call, the file at `tmp.path()` will not be the same as the file held by `tmp`
867+
write_atomic(tmp.path(), "new").unwrap();
868+
assert_eq!(std::fs::read_to_string(tmp.path()).unwrap(), "new");
869+
870+
let new_perms = std::fs::metadata(tmp.path()).unwrap().permissions();
871+
872+
let mask = u32::from(libc::S_IRWXU | libc::S_IRWXG | libc::S_IRWXO);
873+
assert_eq!(original_perms.mode(), new_perms.mode() & mask);
874+
}
875+
826876
#[test]
827877
fn join_paths_lists_paths_on_error() {
828878
let valid_paths = vec!["/testing/one", "/testing/two"];

0 commit comments

Comments
 (0)