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

[asder8215]

To be conservative, I just have the fallback use the previous implementation of set_permissions_nofollow here. If there are other platforms that benefits from using fchmodat directly instead of open + fchmod, we can adjust that accordingly in a separate PR.

View changes since the review

[asder8215]

Yea, I guess that's the reason why I had wasi in its own branch, but I'll cfg gate what OpenOptionsExt I'll use between for wasi

[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.

[ivmarkov]

Sorry if I'm bringing useless noise, but the AI keeps alerting me that using fchmodat with AT_SYMLINK_NOFOLLOW on Linux behaves very differently compared to BSDs, and might return an error when called on a symlink.

For brevity, I won't paste its output here, but just mentioning in case you are not aware.

View changes since the review

[asder8215]

I'm aware. I've mentioned in the documentation for set_permissions_nofollow that on Linux it throws an Unsupported error and on BSD it can change the symlink permission bits (both behaviors should be verified in a test I created for symlinks).

[ChrisDenton]

It's probably reading our docs https://doc.rust-lang.org/nightly/std/fs/fn.set_permissions_nofollow.html#errors