Skip to content
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

Fix check of statx and handle EPERM #65685

Merged
merged 2 commits into from
Oct 25, 2019
Merged
Changes from 1 commit
Commits
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
50 changes: 34 additions & 16 deletions src/libstd/sys/unix/fs.rs
Original file line number Diff line number Diff line change
@@ -105,11 +105,14 @@ cfg_has_statx! {{
flags: i32,
mask: u32,
) -> Option<io::Result<FileAttr>> {
use crate::sync::atomic::{AtomicBool, Ordering};
use crate::sync::atomic::{AtomicU8, Ordering};

// Linux kernel prior to 4.11 or glibc prior to glibc 2.28 don't support `statx`
// We store the availability in a global to avoid unnecessary syscalls
static HAS_STATX: AtomicBool = AtomicBool::new(true);
// We store the availability in global to avoid unnecessary syscalls.
// 0: Unknown
// 1: Not available
// 2: Available
static STATX_STATE: AtomicU8 = AtomicU8::new(0);
syscall! {
fn statx(
fd: c_int,
@@ -120,21 +123,36 @@ cfg_has_statx! {{
) -> c_int
}

if !HAS_STATX.load(Ordering::Relaxed) {
return None;
}

let mut buf: libc::statx = mem::zeroed();
let ret = cvt(statx(fd, path, flags, mask, &mut buf));
match ret {
Err(err) => match err.raw_os_error() {
Some(libc::ENOSYS) => {
HAS_STATX.store(false, Ordering::Relaxed);
return None;
match STATX_STATE.load(Ordering::Relaxed) {
// For the first time, we try to call on current working directory
// to check if it is available.
0 => {
let mut buf: libc::statx = mem::zeroed();
let err = cvt(statx(
libc::AT_FDCWD,
b".\0".as_ptr().cast(),
0,
libc::STATX_ALL,
&mut buf,
))
.err()
.and_then(|e| e.raw_os_error());
// `seccomp` will emit `EPERM` on denied syscall.
// See: https://github.com/rust-lang/rust/issues/65662
Copy link
Member

Choose a reason for hiding this comment

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

Could this expand the comment inline to explain why EPERM is handled specially and why we attempt to stat a "known good" location, along with the pitfalls of this could still return a false negative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... along with the pitfalls of this could still return a false negative?

Will it? The manual said it never returns EPERM. So it should be the only case.

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps! I've come to not trust man pages on errors though, they basically never describe the exhaustive set of errors, only some possible ones.

if err == Some(libc::ENOSYS) || err == Some(libc::EPERM) {
STATX_STATE.store(1, Ordering::Relaxed);
} else {
STATX_STATE.store(2, Ordering::Relaxed);
}
_ => return Some(Err(err)),
try_statx(fd, path, flags, mask)
}
Ok(_) => {
1 => None,
_ => {
let mut buf: libc::statx = mem::zeroed();
if let Err(err) = cvt(statx(fd, path, flags, mask, &mut buf)) {
return Some(Err(err));
Copy link
Member

Choose a reason for hiding this comment

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

Although not part of this PR, it might be nice to change this to return io::Result<Option<FileAttr>> to use ? here instead of if let

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest, Ok(None) for "unavailable" seems weird to me...
Option<Result<T>> is for the syscall may be unavailable, or available with a result.

Changing the type does not simplify much more code. Instead, we need an extra ? after every calls below.

}

// We cannot fill `stat64` exhaustively because of private padding fields.
let mut stat: stat64 = mem::zeroed();
// `c_ulong` on gnu-mips, `dev_t` otherwise