Refactored docs for std::fs::set_permissions_nofollow + fix BSD-based systems to use fchmodat with AT_SYMLINK_NOFOLLOW flag

library/std/src/fs.rs

@@ -3448,22 +3448,27 @@ pub fn set_permissions<P: AsRef<Path>>(path: P, perm: Permissions) -> io::Result
 ///
 /// # Platform-specific behavior
 ///
-/// 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`].
-///
-/// [`fs::set_permissions`]: crate::fs::set_permissions
+/// This function currently corresponds to the following underlying operations:
+/// * Linux: `fchmodat` with `AT_SYMLINK_NOFOLLOW` with a fallback behavior to use
+/// `open` with `O_NOFOLLOW` followed by behavior denoted in [`fs::set_permissions`] when
+/// the former `fchmodat` call errors with `ENOTSUP`[^1].
+/// * BSD-based platforms, Android: `fchmodat` with `AT_SYMLINK_NOFOLLOW`
+/// * Other Unix-based platforms with symlinks: `open` with `O_NOFOLLOW` followed by behavior
+///   denoted in [`fs::set_permissions`].
+/// * Other Unix-based platforms without symlinks: `open` followed by behavior
+///   denoted in [`fs::set_permissions`].
+/// * Windows: `CreateFileW` with `FILE_FLAG_OPEN_REPARSE_POINT` followed
+///   by `SetFileInformationByHandle`.
 ///
 /// Note that, this [may change in the future][changes].
 ///
+/// [^1]: Ubuntu 20.04, for example, makes `fchmodat` with `AT_SYMLINK_NOFOLLOW` return `ENOTSUP`
+/// on both symlinks and non-symlinks
+///
 /// [changes]: io#platform-specific-behavior
 ///
+/// [`fs::set_permissions`]: crate::fs::set_permissions
+///
 /// # Errors
 ///
 /// This function will return an error in the following situations, but is not
@@ -3472,12 +3477,13 @@ pub fn set_permissions<P: AsRef<Path>>(path: P, perm: Permissions) -> io::Result
 /// * `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
+/// Note: On Linux, this will result in an [`Unsupported`] error
+/// if the final element is a symlink. On other Unix-based platforms
+/// with symlinks (non-BSD-based), this will result in a [`FilesystemLoop`]
+/// error.
 ///
 /// [`Unsupported`]: crate::io::ErrorKind::Unsupported
+/// [`FilesystemLoop`]: crate::io::ErrorKind::FilesystemLoop
 ///
 /// # Examples
 ///
@@ -3488,8 +3494,8 @@ pub fn set_permissions<P: AsRef<Path>>(path: P, perm: Permissions) -> io::Result
 /// 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
+///     // This should result in an error on certain platforms or
+///     // succeed in modifying the permissions of a symlink
 ///     fs::set_permissions_nofollow("foo.txt", perms)?;
 ///     Ok(())
 /// }

library/std/src/fs/tests.rs

@@ -649,7 +649,10 @@ fn set_get_permissions_nofollows() {
 
 // Only Windows and Unix support `fs::set_permissions_nofollow`
 #[test]
-#[cfg(all(any(windows, unix), not(any(target_os = "espidf", target_os = "horizon"))))]
+#[cfg(all(
+    any(windows, unix),
+    not(any(target_os = "espidf", target_os = "horizon", target_os = "wasi"))
+))]
 fn set_get_permissions_nofollows_symlink() {
     #[cfg(not(windows))]
     use crate::os::unix::fs::symlink as symlink_dir;
@@ -668,17 +671,12 @@ fn set_get_permissions_nofollows_symlink() {
     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") => {
+        any(windows, 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.
+            // On these systems, it's confirmed the symlink itself is marked readonly
             // https://superuser.com/questions/1099634/change-permissions-symbolic-link-mac-os
-            #[cfg(windows)]
             assert!(metadata0.permissions().readonly());
-            #[cfg(not(windows))]
-            assert!(!metadata0.permissions().readonly());
 
             // Reset the read-only bit under Windows 7: avoids the
             // `TempDir::drop` from crashing on a permission denial when

library/std/src/path.rs

@@ -2377,7 +2377,7 @@ pub struct NormalizeError;
 impl Path {
     // The following (private!) function allows construction of a path from a u8
     // slice, which is only safe when it is known to follow the OsStr encoding.
-    unsafe fn from_u8_slice(s: &[u8]) -> &Path {
+    pub(crate) unsafe fn from_u8_slice(s: &[u8]) -> &Path {
         unsafe { Path::new(OsStr::from_encoded_bytes_unchecked(s)) }
     }
     // The following (private!) function reveals the byte encoding used for OsStr.

library/std/src/sys/fs/unix.rs

@@ -2036,35 +2036,77 @@ pub fn set_perm(p: &CStr, perm: FilePermissions) -> io::Result<()> {
 }
 
 pub fn set_perm_nofollow(p: &CStr, perm: FilePermissions) -> io::Result<()> {
-    // ESP-IDF and Horizon do not support O_NOFOLLOW, so we skip setting it.
-    // Their filesystems do not have symbolic links, so no special handling is required.
     cfg_select! {
-        // wasm32-wasip1 targets do not support fchmodat, so we fall down to
-        // open + fchmod
-        target_os = "wasi" => {
-            use crate::fs::OpenOptions;
-            use crate::fs::Permissions;
-            use crate::os::wasi::ffi::OsStrExt;
-            use crate::os::wasi::fs::OpenOptionsExt;
-
-            let mut options = OpenOptions::new();
-            options.custom_flags(libc::O_NOFOLLOW);
+        target_os = "linux" => {
+            let res = cvt_r(|| unsafe {
+                libc::fchmodat(libc::AT_FDCWD, p.as_ptr(), perm.mode, libc::AT_SYMLINK_NOFOLLOW)
+            })
+            .map(|_| ());
+
+            match res {
+                Ok(_) => return Ok(()),
+                Err(err) => {
+                    if err.kind() == crate::io::ErrorKind::Unsupported {
+                        use crate::fs::OpenOptions;
+                        use crate::fs::Permissions;
+                        use crate::os::unix::ffi::OsStrExt;
+                        use crate::os::unix::fs::OpenOptionsExt;
+
+                        let mut options = OpenOptions::new();
+                        options.read(true).custom_flags(libc::O_NOFOLLOW);
+
+                        let os_str = OsStr::from_bytes(p.to_bytes());
+                        let path = Path::new(os_str);
+                        match options.open(path) {
+                            Ok(file) => {
+                                return file.set_permissions(Permissions::from_inner(perm));
+                            },
+                            Err(e) => {
+                                if e.kind() == crate::io::ErrorKind::FilesystemLoop {
+                                    // When O_NOFOLLOW flag is enabled, if the trailing component of
+                                    // a path is a symbolic link, open should fail with ELOOP error
+                                    // For consistency with other Linux distributions, we return
+                                    // `ErrorKind::Unsupported`.
+                                    return Err(err);
+                                }
+                                return Err(e);
+                            }
+                        }
+                    }
 
-            let bytes = p.to_bytes();
-            let os_str = OsStr::from_bytes(bytes);
-            options.open(Path::new(os_str))?.set_permissions(Permissions::from_inner(perm))
+                    return Err(err);
+                }
+            }
         }
-        all(target_os = "linux", not(any(target_os = "espidf", target_os = "horizon"))) => {
+        any(target_os = "macos", target_os = "freebsd", target_os = "openbsd", target_os = "netbsd", target_os = "dragonfly", target_os = "android") => {
             cvt_r(|| unsafe {
                 libc::fchmodat(libc::AT_FDCWD, p.as_ptr(), perm.mode, libc::AT_SYMLINK_NOFOLLOW)
             })
             .map(|_| ())
         },
+        // Not all targets support fchmodat, so we fall back to
+        // open + fchmod.
         _ => {
-            cvt_r(|| unsafe {
-                libc::fchmodat(libc::AT_FDCWD, p.as_ptr(), perm.mode, 0)
-            })
-            .map(|_| ())
+            use crate::fs::OpenOptions;
+            use crate::fs::Permissions;
+            let mut options = OpenOptions::new();
+            // ESP-IDF and Horizon do not support O_NOFOLLOW, so we skip setting it.
+            // Their filesystems do not have symbolic links, so no special handling is required.
+            #[cfg(not(any(target_os = "espidf", target_os = "horizon")))]
+            {
+                #[cfg(target_os = "wasi")]
+                use crate::os::wasi::fs::OpenOptionsExt;
+                #[cfg(not(target_os = "wasi"))]
+                use crate::os::unix::fs::OpenOptionsExt;
+                options.read(true).custom_flags(libc::O_NOFOLLOW);
+            }
+
+            // SAFETY: Since this function is called with `with_native_path`
+            // and that successfully converted the `&Path` to a `CString`, it
+            // should be safe to slice away the nul byte from `&CStr` and convert
+            // it back to a `&Path`.
+            let path = unsafe { Path::from_u8_slice(p.to_bytes()) };
+            options.open(path)?.set_permissions(Permissions::from_inner(perm))
         }
     }
 }