Uh oh!
There was an error while loading. Please reload this page.
Fix check of statx and handle EPERM - #65685
Conversation
lnicola
commented
Oct 22, 2019
I suppose a micro-optimization would be to only do the |
alexcrichton
left a comment
There was a problem hiding this comment.
Looks great to me, thanks again for helping to handle this @oxalica!
| .err() | ||
| .and_then(|e| e.raw_os_error()); | ||
| // `seccomp` will emit `EPERM` on denied syscall. | ||
| // See: https://github.com/rust-lang/rust/issues/65662 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
... 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.
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
| _ => { | ||
| let mut buf: libc::statx = mem::zeroed(); | ||
| if let Err(err) = cvt(statx(fd, path, flags, mask, &mut buf)) { | ||
| return Some(Err(err)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
rust-highfive
commented
Oct 22, 2019
Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
alexcrichton
commented
Oct 22, 2019
@bors: r+ Looks good to me! |
bors
commented
Oct 22, 2019
📌 Commit 10f1bc7 has been approved by |
bors
commented
Oct 22, 2019
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
Fix check of `statx` and handle EPERM Should fixrust-lang#65662rust-lang#65662 (comment) > I think a reasonable solution might be to do something like try to stat AT_CWD initially and if that fails with EPERM or ENOSYS we disable the syscall entirely, otherwise it's cached as always good to use. r? @alexcrichton
Fix check of `statx` and handle EPERM Should fixrust-lang#65662rust-lang#65662 (comment) > I think a reasonable solution might be to do something like try to stat AT_CWD initially and if that fails with EPERM or ENOSYS we disable the syscall entirely, otherwise it's cached as always good to use. r? @alexcrichton
Rollup of 9 pull requests Successful merges: - #64639 (Stabilize `#[non_exhaustive]` (RFC 2008)) - #65074 (Fix the start/end byte positions in the compiler JSON output) - #65315 (Intern place projection) - #65685 (Fix check of `statx` and handle EPERM) - #65731 (Prevent unnecessary allocation in PathBuf::set_extension.) - #65740 (Fix default "disable-shortcuts" feature value) - #65787 (move panictry! to where it is used.) - #65789 (move Attribute::with_desugared_doc to librustdoc) - #65790 (move report_invalid_macro_expansion_item to item.rs) Failed merges: r? @ghost
Should fix#65662
#65662 (comment)
r? @alexcrichton