-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Added implementation on set_permissions_nofollow for all primary platforms
#158168
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3432,23 +3432,65 @@ pub fn set_permissions<P: AsRef<Path>>(path: P, perm: Permissions) -> io::Result | |
| fs_imp::set_permissions(path.as_ref(), perm.0) | ||
| } | ||
|
|
||
| /// Set the permissions of a file, unless it is a symlink. | ||
| /// Changes the permissions found on a file or a directory. On certain platforms, if the file | ||
| /// is a symlink, it will change the permissions bits on the symlink itself rather than | ||
| /// the target (e.g. Windows, BSD, MacOS). On other platforms, this results in an error when | ||
| /// attempting to change permissions on a symlink (e.g. Linux). | ||
| /// | ||
| /// Note that the non-final path elements are allowed to be symlinks. | ||
| /// Note that non-final path elements are allowed to be symlinks. | ||
| /// | ||
| /// # Platform-specific behavior | ||
| /// | ||
| /// Currently unimplemented on Windows. | ||
| /// This function currently corresponds to: | ||
| /// * `open` with `O_NOFOLLOW` flag enabled + `fchmod` on WASI | ||
| /// * `fchmodat` function with the flag `AT_SYMLINK_NOFOLLOW` enabled | ||
| /// on Unix platforms | ||
| /// * The flag `FILE_FLAG_OPEN_REPARSE_POINT` is enabled and then the | ||
| /// permissions of the file is set through `SetFileInformationByHandle` | ||
| /// on Windows. | ||
| /// * On all other platforms, the behavior remains the same with | ||
| /// [`fs::set_permissions`]. | ||
| /// | ||
| /// On Unix platforms, this results in a [`FilesystemLoop`] error if the last element is a symlink. | ||
| /// [`fs::set_permissions`]: crate::fs::set_permissions | ||
| /// | ||
| /// This behavior may change in the future. | ||
| /// Note that, this [may change in the future][changes]. | ||
| /// | ||
| /// [`FilesystemLoop`]: crate::io::ErrorKind::FilesystemLoop | ||
| #[doc(alias = "chmod", alias = "SetFileAttributes")] | ||
| /// [changes]: io#platform-specific-behavior | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// This function will return an error in the following situations, but is not | ||
| /// limited to just these cases: | ||
| /// | ||
| /// * `path` does not exist. | ||
| /// * The user lacks the permission to change attributes of the file. | ||
| /// | ||
| /// Note: On Linux, this will result in a [`Unsupported`] error | ||
| /// if the final element is a symlink. On BSD-based systems, the | ||
| /// behavior can vary from symlink permission bits changing or | ||
| /// there being no effects on symlinks | ||
| /// | ||
| /// [`Unsupported`]: crate::io::ErrorKind::Unsupported | ||
| /// | ||
| /// # Examples | ||
| /// | ||
| /// ```no_run | ||
| /// #![feature(set_permissions_nofollow)] | ||
| /// use std::fs; | ||
| /// | ||
| /// fn main() -> std::io::Result<()> { | ||
| /// let mut perms = fs::symlink_metadata("foo.txt")?.permissions(); | ||
| /// perms.set_readonly(true); | ||
| /// // This should result in an error on certain platforms | ||
| /// // or succeed in modifying the permissions of a symlink | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or it might do nothing at all apparently, according to the comment above?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, I forgot to update this once I noticed that it could do nothing at all |
||
| /// fs::set_permissions_nofollow("foo.txt", perms)?; | ||
| /// Ok(()) | ||
| /// } | ||
| /// ``` | ||
| #[doc(alias = "fchmodat", alias = "SetFileInformationByHandle")] | ||
| #[unstable(feature = "set_permissions_nofollow", issue = "141607")] | ||
| pub fn set_permissions_nofollow<P: AsRef<Path>>(path: P, perm: Permissions) -> io::Result<()> { | ||
| fs_imp::set_permissions_nofollow(path.as_ref(), perm) | ||
| fs_imp::set_permissions_nofollow(path.as_ref(), perm.0) | ||
| } | ||
|
|
||
| impl DirBuilder { | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On Windows you can set attributes on symlinks (they're just normal files or directories).
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got you. Does setting an attribute on symlinks do anything meaningful? I was reading up on MacOS and they also can set permission bits on symlinks, but they don't do anything meaningful.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it can be meaningful but only to the extent that setting the readonly attribute is meaningful. See Permissions::set_readonly, which notes that the readonly attribute on a directory is pretty useless nowadays. Tbh the name "Permissions" is a bit of misnomer on Windows as it doesn't set actual permissions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -613,6 +613,70 @@ fn set_get_unix_permissions() { | |
| assert_eq!(mask & metadata1.permissions().mode(), 0o0777); | ||
| } | ||
|
|
||
| #[test] | ||
| fn set_get_permissions_nofollows() { | ||
| let tmpdir = tmpdir(); | ||
| let filename = tmpdir.join("set_get_unix_permissions_file"); | ||
| check!(File::create(&filename)); | ||
| let file_metadata = check!(fs::metadata(&filename)); | ||
| assert!(!file_metadata.permissions().readonly()); | ||
| let mut permission_bits = file_metadata.permissions(); | ||
| permission_bits.set_readonly(true); | ||
| let result = fs::set_permissions_nofollow(&filename, permission_bits); | ||
|
|
||
| cfg_select! { | ||
| any(windows, unix, target_os = "uefi", target_os = "solid_asp3", target_os = "motor") => { | ||
| assert_eq!(result.unwrap(), ()); | ||
| let metadata0 = check!(fs::metadata(&filename)); | ||
| assert!(metadata0.permissions().readonly()); | ||
| }, | ||
| _ => { | ||
| let error_kind = result.unwrap_err().kind(); | ||
| assert_eq!(error_kind, crate::io::ErrorKind::Unsupported); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Only Windows and Unix support `fs::set_permissions_nofollow` | ||
| #[test] | ||
| #[cfg(all(any(windows, unix), not(any(target_os = "espidf", target_os = "horizon"))))] | ||
| fn set_get_permissions_nofollows_symlink() { | ||
| #[cfg(not(windows))] | ||
| use crate::os::unix::fs::symlink as symlink_dir; | ||
| #[cfg(windows)] | ||
| use crate::os::windows::fs::symlink_dir; | ||
|
|
||
| let tmpdir = tmpdir(); | ||
| let filename = tmpdir.join("set_get_unix_permissions_file"); | ||
| let symlink_name = tmpdir.join("set_get_unix_permissions"); | ||
| check!(File::create(&filename)); | ||
| check!(symlink_dir(&filename, &symlink_name)); | ||
|
|
||
| let sym_metadata = check!(fs::symlink_metadata(&symlink_name)); | ||
| let mut permission_bits = sym_metadata.permissions(); | ||
| permission_bits.set_readonly(true); | ||
| let result = fs::set_permissions_nofollow(&symlink_name, permission_bits); | ||
|
|
||
| cfg_select! { | ||
| any(windows, target_os = "android", target_os = "macos", target_os = "freebsd", target_os = "openbsd", target_os = "netbsd", target_os = "dragonfly") => { | ||
| assert_eq!(result.unwrap(), ()); | ||
| let metadata0 = check!(fs::symlink_metadata(&symlink_name)); | ||
| // So seems like BSD-based systems trying to set permissions | ||
| // on symlinks could lead to no effect, so we should expect | ||
| // there being no change to BSD-based systems. | ||
| // https://superuser.com/questions/1099634/change-permissions-symbolic-link-mac-os | ||
| #[cfg(windows)] | ||
| assert!(metadata0.permissions().readonly()); | ||
| #[cfg(not(windows))] | ||
| assert!(!metadata0.permissions().readonly()); | ||
| }, | ||
| _ => { | ||
| let error_kind = result.unwrap_err().kind(); | ||
| assert_eq!(error_kind, crate::io::ErrorKind::Unsupported); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| #[cfg(windows)] | ||
| fn file_test_io_seek_read_write() { | ||
|
|
@@ -1330,6 +1394,29 @@ fn fchmod_works() { | |
| check!(file.set_permissions(p)); | ||
| } | ||
|
|
||
| #[test] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assume you didn't mean to keep this in?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I did mean to keep this in. I templated this test off the two tests before this Which reminds me that I need to update documentation to mention that it uses
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess that the main confusion here is that aren't there platforms where this will just fail, e.g. ones without any permissions, like VexOS?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that you ask that, does CI run these tests on platforms like VexOS/UEFI? I know it checks if the code compiles on those platforms, but I haven't noticed if CI actually runs these tests on those platforms. This should definitely panic on certain lines like 1413. |
||
| fn fchmodat_works() { | ||
| let tmpdir = tmpdir(); | ||
| let file = tmpdir.join("in.txt"); | ||
|
|
||
| check!(File::create(&file)); | ||
| let attr = check!(fs::metadata(&file)); | ||
| assert!(!attr.permissions().readonly()); | ||
| let mut p = attr.permissions(); | ||
| p.set_readonly(true); | ||
| check!(fs::set_permissions_nofollow(&file, p.clone())); | ||
| let attr = check!(fs::metadata(&file)); | ||
| assert!(attr.permissions().readonly()); | ||
|
|
||
| match fs::set_permissions_nofollow(&tmpdir.join("foo"), p.clone()) { | ||
| Ok(..) => panic!("wanted an error"), | ||
| Err(..) => {} | ||
| } | ||
|
|
||
| p.set_readonly(false); | ||
| check!(fs::set_permissions_nofollow(&file, p)); | ||
| } | ||
|
|
||
| #[test] | ||
| fn sync_doesnt_kill_anything() { | ||
| let tmpdir = tmpdir(); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
"can vary from... or ..." is not grammatical.
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.
Note: this has already been merged, so, any suggestions would have to be under a separate PR. Could raise this in the tracking issue 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 know...
@asder8215 any chance you could make a follow-up PR for doc clarifications and grammar fixes?
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.
(apologies for missing this, by the way; after enough CI failures I was just incredibly happy this merged successfully)
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 I get it, it's hard to be on the look for docs nuances after fighting with the actual code for a while.