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

[asder8215]

View all comments

This PR refactors documentations for std::fs::set_permissions_nofollow and fixes BSD-based systems + Android to use fchmodat with AT_SYMLINK_NOFOLLOW flag (instead of no flag set) and refactors all other platforms to defer to OpenOptions with O_NOFOLLOW behavior.

r? @clarfonthey
Since they looked at the original set_permissions_nofollow PR I made

cc @RalfJung

[clarfonthey]

These aren't super necessary but 🤷🏻 doesn't matter too much

View changes since the review

[RalfJung]

Yeah this is grammatically still odd -- the first two bullets are not actually sentences, but the last one is.

[clarfonthey]

@bors r+ rollup

[rust-bors]

📌 Commit d24bee2 has been approved by clarfonthey

It is now in the queue for this repository.

[RalfJung]

That flag is enabled where? Should this say "SetFileInformationByHandle with the flag ..." or would that not be right?

View changes since the review

[asder8215]

The internal code for Windows' set_permissions_nofollow does the following:

pub fn set_perm_nofollow(p: &WCStr, perm: FilePermissions) -> io::Result<()> {
    let mut opts = OpenOptions::new();
    opts.access_mode(c::FILE_WRITE_ATTRIBUTES);
    // `FILE_FLAG_OPEN_REPARSE_POINT` for no_follow behavior
    opts.custom_flags(c::FILE_FLAG_BACKUP_SEMANTICS | c::FILE_FLAG_OPEN_REPARSE_POINT);
    let file = File::open_native(p, &opts)?;
    file.set_permissions(perm)
}

It opens the file first with FILE_FLAG_BACKUP_SEMANTICS and FILE_FLAG_OPEN_REPARSE_POINT and then it sets the permissions on the reparse point itself using SetFileInformationByHandle. Enabled is probably a poor choice of word here; at the time, I wasn't too sure how to discuss the opening a file behavior on Windows because it seems like OpenOptions::open doesn't go in depth about platform-specific behavior and the open_native call seem to be doing a lot here:

rust/library/std/src/sys/fs/windows.rs

Lines 345 to 398 in b5be620

	fn open_native(path: &WCStr, opts: &OpenOptions) -> io::Result<File> {
	let creation = opts.get_creation_mode()?;
	let sa = c::SECURITY_ATTRIBUTES {
	nLength: size_of::<c::SECURITY_ATTRIBUTES>() as u32,
	lpSecurityDescriptor: ptr::null_mut(),
	bInheritHandle: opts.inherit_handle as c::BOOL,
	};
	let handle = unsafe {
	c::CreateFileW(
	path.as_ptr(),
	opts.get_access_mode()?,
	opts.share_mode,
	if opts.inherit_handle { &sa } else { ptr::null() },
	creation,
	opts.get_flags_and_attributes(),
	ptr::null_mut(),
	)
	};
	let handle = unsafe { HandleOrInvalid::from_raw_handle(handle) };
	if let Ok(handle) = OwnedHandle::try_from(handle) {
	if opts.freeze_last_access_time || opts.freeze_last_write_time {
	let file_time =
	c::FILETIME { dwLowDateTime: 0xFFFFFFFF, dwHighDateTime: 0xFFFFFFFF };
	cvt(unsafe {
	c::SetFileTime(
	handle.as_raw_handle(),
	core::ptr::null(),
	if opts.freeze_last_access_time { &file_time } else { core::ptr::null() },
	if opts.freeze_last_write_time { &file_time } else { core::ptr::null() },
	)
	})?;
	}
	// Manual truncation. See #115745.
	if opts.truncate
	&& creation == c::OPEN_ALWAYS
	&& api::get_last_error() == WinError::ALREADY_EXISTS
	{
	// This first tries `FileAllocationInfo` but falls back to
	// `FileEndOfFileInfo` in order to support WINE.
	// If WINE gains support for FileAllocationInfo, we should
	// remove the fallback.
	let alloc = c::FILE_ALLOCATION_INFO { AllocationSize: 0 };
	set_file_information_by_handle(handle.as_raw_handle(), &alloc)
	.or_else(|_| {
	let eof = c::FILE_END_OF_FILE_INFO { EndOfFile: 0 };
	set_file_information_by_handle(handle.as_raw_handle(), &eof)
	})
	.io_result()?;
	}
	Ok(File { handle: Handle::from_inner(handle) })
	} else {
	Err(Error::last_os_error())
	}
	}

[RalfJung]

I would say something like

/// This function currently corresponds to the following underlying operations:
/// * WASI: `open` with `O_NOFOLLOW` followed by `fchmod`.
/// * Unix: `fchmodat` with `AT_SYMLINK_NOFOLLOW`.
/// * Windows: `CreateFileW` with `FILE_FLAG_OPEN_REPARSE_POINT` followed
///   by `SetFileInformationByHandle`.

[asder8215]

I took a look at Unix implementation of set_permissions_nofollow again, and I wanted to correct something I said about MacOS/BSD-based systems. Currently, they call fchmodat with no flag provided (0), and I realized that's a mistake I made since I forgot to include them underneath the fchmodat with AT_SYMLINK_NOFOLLOW.

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

Lines 2038 to 2070 in b5be620

	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);
	let bytes = p.to_bytes();
	let os_str = OsStr::from_bytes(bytes);
	options.open(Path::new(os_str))?.set_permissions(Permissions::from_inner(perm))
	}
	all(target_os = "linux", not(any(target_os = "espidf", target_os = "horizon"))) => {
	cvt_r(|| unsafe {
	libc::fchmodat(libc::AT_FDCWD, p.as_ptr(), perm.mode, libc::AT_SYMLINK_NOFOLLOW)
	})
	.map(|_| ())
	},
	_ => {
	cvt_r(|| unsafe {
	libc::fchmodat(libc::AT_FDCWD, p.as_ptr(), perm.mode, 0)
	})
	.map(|_| ())
	}
	}
	}

I think the reason why I did that was because CI mentioned compiler errors on libc::AT_SYMLINK_NOFOLLOW not existing on certain platforms (though this was on dist-various-1 so wasn't sure what that tested), so I gated it to linux, but forgot to update it to include MacOS/other BSD-based systems that support AT_SYMLINK_NOFOLLOW.

I have to fix this and include the BSD-based systems in that block. I also verified the behavior on my MacBook and it should definitely change the permission bits, which means the test case that sets a symlink with readonly permission should pass.

[RalfJung]

@bors r-

Sorry I have a question :)

[rust-bors]

This pull request was unapproved.

View changes since this unapproval

[clarfonthey]

r? RalfJung since you're mostly reviewing already

[rustbot]

RalfJung is not on the review rotation at the moment.
They may take a while to respond.

[RalfJung]

Android and Linux share the same kernel, so I highly doubt they will behave different here?

View changes since the review

[asder8215]

I'll take it off from this block and we can put up a try job to confirm

[RalfJung]

            // On these systems, the symlink itself is now marked readonly.

This is meant to replace the entire comment, github is just incapable of making that a suggestion...

View changes since the review

[RalfJung]

What is going on here? The not part at the end makes no sense at all...?

View changes since the review

[RalfJung]

We promise that we will not follow the symlink. So why is it correct to not set the flag?

View changes since the review

[asder8215]

To be honest with you, I wasn't sure what Unix platforms didn't support AT_SYMLINK_NOFOLLOW/O_NOFOLLOW because they didn't support symlinks.

For example, I think in my previous PR someone was commenting that ESPIDF/Horizon OS do not have support O_NOFOLLOW or symlinks, so I figured that their behavior should match with just regularly setting permissions on the file (fchmodat with no flag set). I had the not condition from earlier because I wasn't exhaustively sure which Unix-based platform has no symlinks.

I could try separating the not portion of the second branch into its own branch using fchmodat with no flag set and then have the _ branch do the same thing that lolbinarycat did originally:

use crate::fs::OpenOptions;
use crate::os::unix::fs::OpenOptionsExt;

OpenOptions::new().custom_flags(libc::O_NOFOLLOW).open(path)?.set_permissions(perm)

^This does return a different error message than Unsupported on symlinks though.

[RalfJung]

I think in my previous PR someone was commenting that ESPIDF/Horizon OS do not have support O_NOFOLLOW or symlinks, so I figured that their behavior should match with just regularly setting permissions on the file (fchmodat with no flag set)

If we are sure this is correct (ping the target maintainers) then we can skip the flag on those targets.

But your code skips the flag on all unknown targets and that doesn't seem good.

OpenOptions::new().custom_flags(libc::O_NOFOLLOW).open(path)?.set_permissions(perm) seems like a good fallback impl.

[asder8215]

Regarding ESPIDF/Horizon OS, I found the PR that made the change after lolbinarycat's implementation:
#145746

It's confirmed in this issue that ESPIDF has no libc::O_NOFOLLOW because its supported filesystems don't use symbolic links

[ivmarkov]

The maintainer of the ESP-IDF target here: yes, I confirm ESP-IDF (and likely Horizon) do not support neither fchmodat, nor AT_SYMLINK_NOFOLLOW, nor O_NOFOLLOW so they should fall back to the old code before #158168. The latest code surface of this PR seems to fix that.

[RalfJung]

To be clear, this only makes sense because they don't support symlinks at all. We have to guarantee that these operations never follow symlinks.

[RalfJung]

Do we know any platform where it does nothing at all?

View changes since the review

[asder8215]

I don't have an exhaustive list. All I read from this MacOS post is that it's possible some systems can't change symbolic link permissions at all, which I interpreted to mean two different things: that fchmodat has no effects on symlinks or it throws an error. I did see this issue here that mentions that certain kernel silently ignored AT_SYMLINK_NOFOLLOW, which change permissions on the target, but that does something instead of no effect and it's an incorrect implementation.

Should I assume that AT_SYMLINK_NOFOLLOW should succeed in setting the permission bits/throw an error if it's not supported on symlinks? I'm unsure if no effects on symlinks and not throwing an error is valid behavior.

[asder8215]

Actually, I'll remove the no effects behavior note. I don't think of it being a valid behavior for this function, and since I can't find any results that tell me what platforms does nothing for fchmodat with AT_SYMLINK_NOFOLLOW

[RalfJung]

The user-facing comments are fine but this is turning into a libs discussion about the implementation.

r? libs

[rust-bors]

💔 Test for 01bf5b1 failed: CI. Failed job:

• Calculate job matrix (web logs, enhanced plaintext logs)

[asder8215]

@bors try jobs=x86_64-msvc-1,dist-various-*,test-various,dist-android

(Unsure why aarch64-apple is not working, it worked in the previous PR I made on set_permissions_nofollow).

[clarfonthey]

I generally just check the latest commit on main to see what the full list of jobs is. There are other ways, but, that's one of the easy ones I do it.

[rust-bors]

☀️ Try build successful (CI)
Build commit: 5a55929 (5a5592949ddd5fca3a64ce3f68519dce280db83e)
Base parent: e71c0f1 (e71c0f1e3395b10a8c331317be1a5c107bdf7b2e)

[RalfJung]

Suggested change

	/// * Linux, BSD-based platforms, Android, QNX: `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].
	/// * 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`.
	/// * Linux, BSD-based platforms, Android, QNX: `fchmodat` with `AT_SYMLINK_NOFOLLOW`. If that is not supported, we fall back to
	/// `open` with `O_NOFOLLOW` followed by [`fs::set_permissions`].
	/// * Other Unix-based platforms with symlinks: `open` with `O_NOFOLLOW` followed by [`fs::set_permissions`].
	/// * Other Unix-based platforms without symlinks: `open` followed by [`fs::set_permissions`].
	/// * Windows: `CreateFileW` with `FILE_FLAG_OPEN_REPARSE_POINT` followed
	/// by `SetFileInformationByHandle`.

View changes since the review

[RalfJung]

As an extra twist, the glibc manpages, even for (current) Arch and for Ubuntu 26.04, say that this should not work at all! E.g. the Ubuntu 26.04 manpages say in the "errors" section:

https://manpages.ubuntu.com/manpages/resolute/man2/fchmodat.2.html#errors

ENOTSUP
    (fchmodat()) flags specified AT_SYMLINK_NOFOLLOW, which is not supported.

So I think ideally on Linux we should try the fast path but fall back to open() + fchmod() if it fails, to maintain support for older distros. And potentially send a bug report about the man pages not matching the actual behaviour.

I think the man page just says "if you see that error code, it means the flag is not supported".
It does not say "it will always return that error".
It's confusingly worded, but that makes most sense as interpretation here I think.

[fs-rachel]

As an extra twist, the glibc manpages, even for (current) Arch and for Ubuntu 26.04, say that this should not work at all! E.g. the Ubuntu 26.04 manpages say in the "errors" section:
https://manpages.ubuntu.com/manpages/resolute/man2/fchmodat.2.html#errors

ENOTSUP
    (fchmodat()) flags specified AT_SYMLINK_NOFOLLOW, which is not supported.

So I think ideally on Linux we should try the fast path but fall back to open() + fchmod() if it fails, to maintain support for older distros. And potentially send a bug report about the man pages not matching the actual behaviour.

I think the man page just says "if you see that error code, it means the flag is not supported". It does not say "it will always return that error". It's confusingly worded, but that makes most sense as interpretation here I think.

I agree, that's the only reading that's consistent with the actual behaviour

[fs-rachel]

@fs-rachel Added the fallback behavior in the latest commit (and updated the docs to mention this for Linux platforms). I'll let the reviewers go through it to see if the current behavior is okay.
Change for fallback behavior I took reference from your patch, so listed you as co-author for it.

Thank you :)

I've also noticed that the set_get_permissions_nofollows_symlink fails on QNX. Turns out fchmodat on a symlink succeeds there, but the test expects it to fail.

Rather than adding QNX (and NTO) to the giant list of platforms where we expect fchmodat to succeed, maybe it would be better to invert the condition so that we have a list of platforms where we expect it to fail. That would be shorter and IMHO a lot clearer.

Following up on this, because I realized I wasn't very clear:

The code currently in the main branch, and this patch, both fail on QNX. The tests treat it as a platform where fchmodat on a symlink should fail (like Linux), when actually it succeeds (like Windows and BSD).

I've written a fix and tested that it works on our CI here: ferrocene/ferrocene@a450413 . I don't love how long the list of platforms in that any() is, but I'm not sure that there's a better option with the way the code is currently written.

@asder8215 Please can you cherry-pick that commit into this PR? (minus the "Test" label because I've confirmed that it works now)

[ivmarkov]

☔ The latest upstream changes (presumably #161396) made this pull request unmergeable. Please resolve the merge conflicts by rebasing.

Folks, is there a reasonable chance to get this approved and merged - if not in the next days then till end of month? ^^^ is a bit discouraging.

The problem is, the already-merged precursor to this PR broke the ESP-IDF Tier 3 target in a way where STD does not even compile anymore for that target - which this PR addresses.

And it's been broken since a month... given that the target is Tier 3, having a usable nightly (because -Z build-std is a nightly-only exercise after all these years) is important.

Thanks for the understanding and sorry if I pushed a bit too much. I get it it is good to get this right after the first PR failed, but maybe we should've rolled back the first PR in the meantime. Which is also an option if this takes too long I guess?

EDIT: #158168 is the problematic one.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[asder8215]

I'm with @ivmarkov that something needs to be done about the mistake I made in #158168. I'm concerned about set_permissions_nofollow being used on bsd-based platforms because it falls down to libc::fchmodat(libc::AT_FDCWD, p.as_ptr(), perm.mode, 0) which doesn't use AT_SYMLINK_NOFOLLOW (pretty big mistake on my part).

I think either the commit b319483 should be reverted/removed or that this PR should be approved soon. I'm unfamiliar with how libs team handle reversion on nightly features though (is it safe to just pop a commit out of history, and then just push that?).

As for this PR, I think all it needs, if no other feedback or review is given, is a try job run on Apple (rebased code should not have changed from the last try job other than tidy formatting). I'll give a try job on Apple after CI turns green.

[RalfJung]

The feature not working properly is not great but, well, it's a nightly feature so 🤷 .

The standard library not building on a tier 3 target is a problem. It sometimes happens but it shouldn't take a month to fix. If you submit a minimal PR that fixes just that we can get that approved fairly quickly I think.

[ivmarkov]

The feature not working properly is not great but, well, it's a nightly feature so 🤷 .

The standard library not building on a tier 3 target is a problem. It sometimes happens but it shouldn't take a month to fix. If you submit a minimal PR that fixes just that we can get that approved fairly quickly I think.

Thanks.
I will then, if we can't merge this PR till - say - middle of next week.

[asder8215]

@bors try jobs=aarch64-apple-*

[rust-bors]

☀️ Try build successful (CI)
Build commit: 4aa7588 (4aa7588b335d8f07ab05692ac71a3f44aa8fc914)
Base parent: c656540 (c656540d6467dee1381f0cbd882412d6bd1cd5ae)