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

Do a uname version check before using statx on Android. #312

Merged
merged 3 commits into from
Apr 12, 2023

Conversation

sunfishcode
Copy link
Member

Similar to #197, do a uname version check before using statx on Android, since Android's seccomp configuration kills the process if any unrecognized system calls are used.

Fixes #311.

Similar to #197, do a `uname` version check before using `statx` on
Android, since Android's seccomp configuration kills the process if any
unrecognized system calls are used.

Fixes #311.
Some Android versions disallow these even on Linux kernel versions which
otherwise would support them, and they crash the process rather than
returning `ENOSYS` so there's no way to detect this. So disallow using
`statx` and `openat2` on Android altogether.
@sunfishcode
Copy link
Member Author

It appears there's no reliable way to test for syscall availability on Android, so this PR now disabled the use of openat2 and statx on Android altogether.

@sunfishcode sunfishcode merged commit 8ee1e7f into main Apr 12, 2023
@sunfishcode sunfishcode deleted the sunfishcode/android-statx branch April 12, 2023 15:22
Comment on lines -64 to -79
// On Android, seccomp kills processes that execute unrecognized system
// calls, so we do an explicit version check rather than relying on
// getting an `ENOSYS`.
#[cfg(target_os = "android")]
{
static CHECKED: AtomicBool = AtomicBool::new(false);

if !CHECKED.load(Relaxed) {
if !openat2_supported() {
INVALID.store(true, Relaxed);
return Err(rustix::io::Errno::NOSYS.into());
}

CHECKED.store(true, Relaxed);
}
}
Copy link

Choose a reason for hiding this comment

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

AFACIT removing this is wrong, you need to always return NONSYS instead.

That's because there are other callers of this function, e.g.:

/// Use `openat2` with `O_PATH` and `fstat`. If that's not available, fallback
/// to `manually::stat`.
pub(crate) fn stat_impl(
start: &fs::File,
path: &Path,
follow: FollowSymlinks,
) -> io::Result<Metadata> {
use std::os::unix::fs::OpenOptionsExt;
// Open the path with `O_PATH`. Use `read(true)` even though we don't need
// `read` permissions, because Rust's libstd requires an access mode, and
// Linux ignores `O_RDONLY` with `O_PATH`.
let result = open_beneath(

Copy link

@eddyb eddyb Apr 17, 2023

Choose a reason for hiding this comment

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

Here's what I just tried, locally:

pub(crate) fn open_beneath(
    start: &fs::File,
    path: &Path,
    options: &OpenOptions,
) -> io::Result<fs::File> {
    #[cfg(target_os = "android")]
    {
        return Err(rustix::io::Errno::NOSYS.into());
    }

And that definitely works (tested on an actual Android impl that hit both openat2 and statx issues before now - an Oculus Quest 2, which makes this an extremely side-tracked thing for me so I will very likely not be opening a PR myself).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the report! Confirmed that this what's happening. I've now filed #316 to fix this, and disable openat2 on Android entirely with a cfg.

sunfishcode added a commit that referenced this pull request Apr 20, 2023
Disable the `openat2` code entirely on Android with a `cfg`, and adjust other
code to avoid calling it.

This fixes some `openat2` calls that remained after #312.
sunfishcode added a commit that referenced this pull request Apr 20, 2023
Disable the `openat2` code entirely on Android with a `cfg`, and adjust other
code to avoid calling it.

This fixes some `openat2` calls that remained after #312.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

stat crashes on Android
2 participants