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

Preserve file permissions on unix during write_atomic #13898

Merged
merged 2 commits into from
May 15, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 51 additions & 1 deletion crates/cargo-util/src/paths.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)]
Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

@stevenengler stevenengler May 10, 2024

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?

Copy link
Member

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.

Copy link
Member

@ChrisDenton ChrisDenton May 11, 2024

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

@stevenengler stevenengler May 15, 2024

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.

if let Some(perms) = perms {
tmp.as_file().set_permissions(perms)?;
}

tmp.persist(path)?;
Ok(())
}
Expand Down Expand Up @@ -823,6 +847,32 @@ mod tests {
assert_eq!(contents, original_contents);
}

#[test]
#[cfg(unix)]
fn write_atomic_permissions() {
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

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"];
Expand Down