Skip to content

fs::set_permissions_nofollow: Android support, test cleanup #163548

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.

Sign up for GitHub

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

Merged
rust-bors merged 2 commits into from
Oct 4, 2026
Merged

fs::set_permissions_nofollow: Android support, test cleanup #163548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06660e6
simplify and clarify fs::set_permissions_nofollow tests
RalfJung Sep 30, 2026
afc0663
add Android support based on fallback path
RalfJung Sep 30, 2026
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
2 changes: 1 addition & 1 deletion library/std/src/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3645,7 +3645,7 @@ pub fn set_permissions<P: AsRef<Path>>(path: P, perm: Permissions) -> io::Result
/// This function currently corresponds to the following underlying operations:
/// * Android: returns [`Unsupported`] on all files.
/// * Linux, BSD-based platforms, QNX, NTO: `fchmodat` with `AT_SYMLINK_NOFOLLOW`.
/// If that is not supported, we fall back to:
/// If that is not supported, we fall back to:
/// * Unix-based platforms with symlinks: `open` with `O_NOFOLLOW` followed by
/// [`fs::set_permissions`].
/// * Unix-based platforms without symlinks: `open` followed by [`fs::set_permissions`].
Expand Down
40 changes: 17 additions & 23 deletions library/std/src/fs/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,12 +613,13 @@ fn set_get_unix_permissions() {
assert_eq!(mask & metadata1.permissions().mode(), 0o0777);
}

#[cfg(not(target_os = "android"))]
/// Test set_permissions_nofollow on a regular file.
#[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();
Expand Down Expand Up @@ -648,12 +649,8 @@ fn set_get_permissions_nofollows() {
}
}

// Only Windows and Unix support `fs::set_permissions_nofollow`

@RalfJung RalfJung Sep 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't make sense of this comment as the test above runs on everything except for Android.
@asder8215 what did you mean when writing this comment?

View changes since the review

@asder8215 asder8215 Sep 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be a stale comment that I forgot to change.

I think, at the time, I didn't have set_permissions_nofollow supported for wasi (since it didn't have fchmodat support, but pretty sure it works with the openat with no follow flag) or this test specifically didn't run on wasi.

Although technically, does this test may not run on wasi since I thought they use a different symlink create function, i.e. symlink_path. You might want to add an import to symlink_path as a symlink_file gated to wasi.

@RalfJung RalfJung Sep 30, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing else in this file seems to special-case wasi so we probably just don't run these tests there.

/// Test set_permissions_nofollow on a symlink.
#[test]
#[cfg(all(
any(windows, unix),
not(any(target_os = "espidf", target_os = "horizon", target_os = "wasi"))

@clarfonthey clarfonthey Oct 3, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is why you wanted to run test-various as well? I guess it still works here, too?

View changes since the review

@RalfJung RalfJung Oct 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I observed that set_get_permissions_nofollows runs everywhere except android. That means the comment, "Only Windows and Unix support fs::set_permissions_nofollow", just does not make sense.

My guess is that some of these were disabled because e.g. espidf does not support symlinks. However the comment does not say that and there are other symlink tests in this file and they are not gating espidf. So I'd rather leave it to an espidf maintainer to add the necessary cfg everywhere in this file, with proper comments. Same for horizon. I have not the slightest idea why wasi was added here, that one actually has handling in the implementation so it really should work.

))]
fn set_get_permissions_nofollows_symlink() {
#[cfg(not(windows))]
use crate::os::unix::fs::symlink as symlink_file;
Expand All @@ -667,14 +664,20 @@ fn set_get_permissions_nofollows_symlink() {
check!(symlink_file(&filename, &symlink_name));

let init_symlink_metadata = check!(fs::symlink_metadata(&symlink_name));
let mut init_symlink_permissions = init_symlink_metadata.permissions();

assert!(!init_symlink_metadata.permissions().readonly());
let init_target_metadata = check!(fs::metadata(&symlink_name));
let init_target_permissions = init_target_metadata.permissions();
assert!(!init_target_metadata.permissions().readonly());

// Set symlink permissions to readonly
init_symlink_permissions.set_readonly(true);
let result = fs::set_permissions_nofollow(&symlink_name, init_symlink_permissions);
let result = fs::set_permissions_nofollow(&symlink_name, {
let mut permissions = init_symlink_metadata.permissions();
permissions.set_readonly(true);
permissions
});

// This should not change the permissions of the target!
let after_target_metadata = check!(fs::metadata(&symlink_name));
assert_eq!(after_target_metadata.permissions(), init_target_metadata.permissions());

cfg_select! {
any(
Expand All @@ -687,16 +690,11 @@ fn set_get_permissions_nofollows_symlink() {
target_os = "nto",
target_os = "qnx"
) => {
// On these systems, it's confirmed the symlink itself is marked readonly
// https://superuser.com/questions/1099634/change-permissions-symbolic-link-mac-os
assert_eq!(result.unwrap(), ());

let after_target_metadata = check!(fs::metadata(&symlink_name));
// We should expect the target file to not have its permission bits
// changed
assert_eq!(after_target_metadata.permissions(), init_target_permissions);

let after_symlink_metadata = check!(fs::symlink_metadata(&symlink_name));
// On these systems, it's confirmed the symlink itself is marked readonly
// https://superuser.com/questions/1099634/change-permissions-symbolic-link-mac-os
assert!(after_symlink_metadata.permissions().readonly());

Comment thread
clarfonthey marked this conversation as resolved.
// Reset the read-only bit under Windows 7: avoids the
Expand All @@ -710,11 +708,7 @@ fn set_get_permissions_nofollows_symlink() {
}
}
_ => {
let after_target_metadata = check!(fs::metadata(&symlink_name));
// We should expect the target file to not have its permission bits
// changed
assert_eq!(after_target_metadata.permissions(), init_target_permissions);

// Everywhere else, this just fails.
let error_kind = result.unwrap_err().kind();
assert_eq!(error_kind, crate::io::ErrorKind::Unsupported);
}
Expand Down
21 changes: 6 additions & 15 deletions library/std/src/sys/fs/unix.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1896,22 +1896,11 @@ pub fn set_perm_nofollow(_p: &CStr, _perm: FilePermissions) -> io::Result<()> {
Err(crate::io::ErrorKind::Unsupported.into())
}

#[cfg(target_os = "android")]
pub fn set_perm_nofollow(_p: &CStr, _perm: FilePermissions) -> io::Result<()> {
// Currently Android seems to be having inconsistent behavior with fchmodat
// with `AT_SYMLINK_NOFOLLOW` or openat with `O_NOFOLLOW` + fchmod.
// See this issue here mentioning inconsistent behavior on fchmodat:
// https://github.com/android/ndk/issues/1258
// On the arm-android CI job, using fchmodat with `AT_SYMLINK_NOFOLLOW` +
// fallback behavior on a symlink sets the target file's permissions,
// which is incorrect behavior.
Err(crate::io::ErrorKind::Unsupported.into())
}

#[cfg(not(any(target_os = "android", target_os = "vxworks")))]
#[cfg(not(target_os = "vxworks"))]
pub fn set_perm_nofollow(p: &CStr, perm: FilePermissions) -> io::Result<()> {
/// Helper function for fallback open with `O_NOFOLLOW` + `fchmod` behavior. This will
/// successfully change the permissions of non-symlinks and fail when there's a symlink.
#[inline]
/// Helper function for fallback open with `O_NOFOLLOW` + `fchmod` behavior
fn open_and_set_permissions(p: &CStr, perm: FilePermissions) -> io::Result<()> {
use crate::fs::{OpenOptions, Permissions};

Expand All @@ -1926,6 +1915,7 @@ pub fn set_perm_nofollow(p: &CStr, perm: FilePermissions) -> io::Result<()> {
use crate::os::unix::fs::OpenOptionsExt;
#[cfg(target_os = "wasi")]
use crate::os::wasi::fs::OpenOptionsExt;

options.custom_flags(libc::O_NOFOLLOW);
}

Expand All @@ -1940,7 +1930,8 @@ pub fn set_perm_nofollow(p: &CStr, perm: FilePermissions) -> io::Result<()> {
#[allow(unused)]
let mut res: Result<(), core::io::Error> = Err(crate::io::ErrorKind::Unsupported.into());

// These platforms support `fchmodat`, so utilize this syscall over `open` + `fchmod`
// These platforms support `fchmodat`, so utilize this syscall over `open` + `fchmod`.
// We can *not* use this on Android due to <https://github.com/android/ndk/issues/1258>.
#[cfg(any(
target_os = "linux",
target_os = "macos",
Expand Down
Loading