std.posix.faccessat: fallback to older faccessat syscall if possible
#24900
Conversation
rohlem
force-pushed
the
PR-fallback-to-older-faccessat
branch
from
8167c92 to
1ef603e
Compare
lib/std/posix.zig
Outdated
| const linux_is_guaranteed_at_least_5_8 = builtin.target.os.isAtLeast(.linux, .{ .major = 5, .minor = 8, .patch = 0 }); | ||
| const try_with_flags_first = try_with_flags_first: switch (system) { | ||
| linux => { //regular .faccessat syscall doesn't support flags argument | ||
| if (builtin.abi.isAndroid()) break :try_with_flags_first false; //standard android programs run in a seccomp sandbox that seems to trigger signal 31 (SIGSYS) for faccessat2 | ||
| if (linux_is_guaranteed_at_least_5_8 != true) break :try_with_flags_first false; //faccessat2 was introduced in Linux 5.8 | ||
| break :try_with_flags_first (flags != 0); //the older, simpler syscall should stay supported, so we only need to use faccessat2 if we need flags | ||
| }, | ||
| else => true, | ||
| }; | ||
| try_with_flags_now: switch (try_with_flags_first) { | ||
| else => |try_with_flags_now| { | ||
| if (!try_with_flags_now) { | ||
| if (flags != 0) return error.UnsupportedFlags; | ||
| } | ||
| const faccessat_result = switch (system) { | ||
| linux => if (try_with_flags_now) | ||
| linux.faccessat2(dirfd, path, mode, flags) | ||
| else | ||
| linux.faccessat(dirfd, path, mode), | ||
| else => system.faccessat(dirfd, path, mode, flags), | ||
| }; | ||
| switch (errno(faccessat_result)) { | ||
| .SUCCESS => return, | ||
| .ACCES => return error.AccessDenied, | ||
| .PERM => return error.PermissionDenied, | ||
| .ROFS => return error.ReadOnlyFileSystem, | ||
| .LOOP => return error.SymLinkLoop, | ||
| .TXTBSY => return error.FileBusy, | ||
| .NOTDIR => return error.FileNotFound, | ||
| .NOENT => return error.FileNotFound, | ||
| .NAMETOOLONG => return error.NameTooLong, | ||
| .INVAL => unreachable, | ||
| .FAULT => unreachable, | ||
| .IO => return error.InputOutput, | ||
| .NOMEM => return error.SystemResources, | ||
| .ILSEQ => |err| if (native_os == .wasi) | ||
| return error.InvalidUtf8 | ||
| else | ||
| return unexpectedErrno(err), | ||
| .NOSYS => |err| if (system == linux and try_with_flags_now) | ||
| continue :try_with_flags_now false //retry with older .faccessat (without support for flags) | ||
| else | ||
| return unexpectedErrno(err), | ||
| else => |err| return unexpectedErrno(err), | ||
| } | ||
| }, |
There was a problem hiding this comment.
This code would be much easier to reason about if it just did faccessat on Android (returning an error if flags is non-zero, before even doing the syscall). Then it can just unconditionally do faccessat2 on regular Linux. I don't think the complexity added by the version checking and fallback logic is worth it.
Note that Linux 5.8.0 is 5 years old. For perspective, even Debian bullseye is on 5.10.0 currently.
There was a problem hiding this comment.
pushed an alternative without the retrying/looping
EDIT: re-read your comment, I've found another way to reduce line count to be pretty minimal, pushing in a minute
There was a problem hiding this comment.
Okay, sorry for the wait - the linux version check is quite short now, and I've split it into a 3rd commit.
Take another look and decide whether that's ok to keep, or should still be dropped.
rohlem
force-pushed
the
PR-fallback-to-older-faccessat
branch
from
1ef603e to
f558c49
Compare
…essat2 when required for flags
…mp sandbox would SIGSYS)
rohlem
force-pushed
the
PR-fallback-to-older-faccessat
branch
from
f558c49 to
6e3fdb6
Compare
lib/std/posix.zig
Outdated
| @@ -5055,7 +5055,12 @@ pub fn faccessatZ(dirfd: fd_t, path: [*:0]const u8, mode: u32, flags: u32) Acces | |||
| return faccessat(dirfd, mem.sliceTo(path, 0), mode, flags); | |||
| } | |||
|
|
|||
| const flags_unsupported = switch (system) { | |||
| linux => builtin.abi.isAndroid() //standard android programs run in a seccomp sandbox that seems to trigger signal 31 (SIGSYS) for faccessat2 | |||
There was a problem hiding this comment.
Gah, this is missing a comma now, but I feel bad to keep re-starting CI over this.
Warning that it needs to be fixed up (esp. if you drop the third commit).
| else => false, | ||
| }; | ||
| const need_flags = (flags != 0); | ||
| if (need_flags and flags_unsupported) return error.UnsupportedFlags; |
There was a problem hiding this comment.
So I forgot to check whether error.UnsupportedFlags is in the error set, and it's not.
I took it from the FanotifyInitError error set, because the description seemed to pretty much match the situation here.
Should we add error.UnsupportedFlags to the AccessError error set, or is one of the other errors close enough to be used here?
(On android it's basically a low-level permissions issue, so maybe error.PermissionDenied, but I kind of don't think users care about that technicality.)
This is my attempt to implement the suggestion in #24860 (comment).
I'm not too familiar with the
stdcoding style - I looked at surrounding code, but feel free to suggest or directly edit and apply improvements.This is my third attempt in trying to make the change not look too gnarly.
I split it up into 2 commits, but due to the indentation change the regular git diff in the second still comes out rather ugly.
I'm also not on linux / building anything for android, so couldn't actually test this locally - hoping there's no typo, or that this can otherwise at least serve as basis for discussion.
Should close #24860 , if it works as intended.