handle impossible errors from the kernel with error.Unexpected instead of unreachable

[jorangreef]

On Linux, e.g. in os.zig the std lib will often interrogate errno and map to Zig errors, or unreachable if the std lib wants to assert that the std lib implementation would never cause an EINVAL or EFAULT, e.g.:

EINVAL => unreachable,
EFAULT => unreachable,

However, while implementing https://github.com/ziglang/zig/pull/6356, I was about to follow this pattern but then I realized that the kernel often overloads errors in new kernel versions, which is particularly the case for the io_uring syscalls.

This means that we might think our std lib implementation cannot cause EINVAL, and then the kernel adds a new feature which could, leading to undefined behavior instead of a safe error.

In other words, we need to start going through the std lib and make this usage of unreachable an anti-pattern because there's no way we can assert what the kernel can or cannot be returning like this.

[jorangreef]

Either that, or we need an unreachable which doesn't become undefined behavior in fast builds, or another separate keyword. To be clear, I don't mean to suggest it's necessarily a good idea to add keywords...

[pfgithub]

Either that, or we need an unreachable which doesn't become undefined behavior in fast builds, or another separate keyword.

I think @panic() does this

[Rocknest]

Do you have any example of a syscall that have changed its behaviour, except ioring stuff? If its specific to that feature, then do not use this pattern with these apis.

[jorangreef]

I haven't looked, but it's not about changing behavior so much as adding behavior, i.e. two error states (one old, one new) map to one error code, and Zig only checks one error state (because that was the only error state that existed at the time in the man pages) or assumes unreachable.

[rohlem]

I don't know much about Linux; maybe I'm spouting nonsense, but couldn't the opposite, hypothetically, be true as well? If a syscall is "upgraded" to loosen its preconditions, then what was previously an error now succeeds. If the program relies on that logic, it breaks semantically (which is its own class of undefined behaviour).

To me it seems like the actual solution would be to instead put an upper limit on the supported kernel version (so do a version check and @panic at run time if it is too high/new). While it's a shame supporting every new release would mean having to manually verify none of the syscall behaviour has changed, fwict that's the only reliable way to go about it. (Really there'd need to be a "compatibility profile" mode when these changes happen, like OpenGL has, though it's of course easier to just not change the interface. Assuming the latter, I too would be inclined to think this is an io_uring-specific growth pain / symptom, though again, I know nothing.)

[jorangreef]

but couldn't the opposite, hypothetically, be true as well? If a syscall is "upgraded" to loosen its preconditions, then what was previously an error now succeeds.

Thanks @rohlem, for the interesting inverse example. That's certainly true, but I think at least it wouldn't lead to undefined behavior from a safety point of view? The program simply wouldn't receive an error from the kernel, so this kind of code branch wouldn't be reached.

Unless, then again, it could be a safety issue if the program required the kernel to return an error, and fell back to unreachable if it did not.

To me it seems like the actual solution would be to instead put an upper limit on the supported kernel version (so do a version check and @panic at run time if it is too high/new). While it's a shame supporting every new release would mean having to manually verify none of the syscall behaviour has changed, fwict that's the only reliable way to go about it.

Perhaps at first glance, but remember the kernel is free to backport features. Ultimately, checking kernel versions is a slippery slope and should be avoided in favor of relying on the error mechanism the kernel already exposes, albeit without using unreachable to assert what the kernel can/cannot return as an error.

[Rocknest]

Its expected that OSes have some backward compatibility, if the system has a breaking change thats not our problem (everything would break if for example read syscall suddenly changed). All documented errors should be handled and nothing more.

[andrewrk]

My only concern here is with the debug experience. For example the following application:

const std = @import("std");

test "close() twice" {
    var file = try std.fs.cwd().createFile("aoeuaoeu", .{});
    defer file.close();

    file.close();
}

The debug experience here is excellent:

[nix-shell:~/Downloads/zig/build-release]$ ./zig test test.zig 
Test [1/1] test "close() twice"... thread 2813 panic: reached unreachable code
/home/andy/Downloads/zig/lib/std/os.zig:252:18: 0x209312 in std.os.close (test)
        .BADF => unreachable, // Always a race condition.
                 ^
/home/andy/Downloads/zig/lib/std/fs/file.zig:188:21: 0x2080fa in std.fs.file.File.close (test)
            os.close(self.handle);
                    ^
/home/andy/Downloads/zig/build-release/test.zig:5:21: 0x20689d in test "close() twice" (test)
    defer file.close();
                    ^
/home/andy/Downloads/zig/lib/std/special/test_runner.zig:77:28: 0x22d302 in std.special.main (test)
        } else test_fn.func();
                           ^
/home/andy/Downloads/zig/lib/std/start.zig:525:22: 0x225eec in std.start.callMain (test)
            root.main();
                     ^
/home/andy/Downloads/zig/lib/std/start.zig:477:12: 0x20986e in std.start.callMainWithArgs (test)
    return @call(.{ .modifier = .always_inline }, callMain, .{});
           ^
/home/andy/Downloads/zig/lib/std/start.zig:391:17: 0x208486 in std.start.posixCallMainAndExit (test)
    std.os.exit(@call(.{ .modifier = .always_inline }, callMainWithArgs, .{ argc, argv, envp }));
                ^
/home/andy/Downloads/zig/lib/std/start.zig:304:5: 0x208292 in std.start._start (test)
    @call(.{ .modifier = .never_inline }, posixCallMainAndExit, .{});
    ^
error: the following test command crashed:
zig-cache/o/82032e287c7c598235bf900b138d0986/test /home/andy/Downloads/zig/build-release/zig

Let's just make sure it remains excellent after the changes incurred by this proposal.

[jorangreef]

My only concern here is with the debug experience.

Yes, thanks for the example.

On second thought I think we should actually leave the status quo as is, because it's a better experience, and safe in Debug and ReleaseSafe even if the kernel does overload errors. It's also something that can be maintained across different kernel versions and they're not going to be overloading errors that often.

Alternatively, simply a @panic with clear explanation.

[richiejp]

Just to complicate things. Any syscall can produce any error due to seccompbpf or an LSM although usually it would be PERM, but maybe some people would use NOSYS. This is a very common scenario now with sandboxing and containers.

Syscalls that act on FDs can produce any error because FDs can point to anything. You can create a file system that represents DNS and start returning NSRCNAMELOOP on writes or reads. It doesn't have to be a mainline kernel, it can be an external module or FUSE.

Even with just the mainline kernel, if you actually mapped all the error codes that a syscall like read or setsockopt can return and someone may want to deal with, then you'll have a big long list of expected errors.

[lumi2021]

we need an unreachable which doesn't become undefined behavior in fast builds

Whould not be a option use comptime conditionals for solve it? Something like

const builtin = @import("builtin"):
// ...

.SUCCESS => return nread,
.INVAL => {
    comptime if (builtin.mode == .ReleaseFast) {
        unreachable;
    } else {
        @panic("Invalid file descriptor");
    },
.FAULT => {
    comptime if (builtin.mode == .ReleaseFast) {
        unreachable;
    } else {
        @panic("Fault during file reading");
    }
},
// ...
.BADF => return error.NotOpenForReading, // Can be a race condition.

should solve the problem of implement it as undefined behavior (default unreachable) in fast builds and as a specific panic in other build modes. No more keywords, only already implemented features.

The debug experience should not change (if not improved when not releaseFast) as the stack stays the same.

I see here only two problems: verbosity and maybe a slightly increased build time as some more operations need to be made for every condition, but i think that spend some time in a comptime condition check is better than add a keyword that will be rarelly used (if not only used in these posix implementations).

[lumi2021]

Thanks @rohlem, for the interesting inverse example. That's certainly true, but I think at least it wouldn't lead to undefined behavior from a safety point of view? The program simply wouldn't receive an error from the kernel, so this kind of code branch wouldn't be reached.

System call changes should not generally be expected. Even not following the Unix standards, the linux kernel should not change it already implemented behavior for reasons of compatibility. Knowing that the kernew will (at least following a common sense) never change the behavior of a already implemented system call, adding some behavior (mainly more specific error messages) are still possible. In this case, in my opinion, the best way to solve this problem is just thinking it do not exist. It whould be much effort to include every case of every kernel version as sugested by @rohlem as it rarelly happens and will represent a real problem, as in the end a fatal error is a fatal error. For me what should be done is implement a generic fallback and manually update the library as changes will rarelly being made. something like

else => {
    comptime if (builtin.mode == .ReleaseFast) {
        unreachable;
    } else {
        @panic("Unexpected errno value");
    }

as an adition to my previous comment should be the best to handle these cases if it even happen to have to be handled.

[Sothatsit]

I think returning Unexpected is a much nicer default. It allows people who are happy to ignore the error to ignore it, and people who need to worry about the specific error can always make the syscall themselves. This would help for tools like ncdu, where ignoring a file that led to a weird error is perfectly fine (https://github.com/ziglang/zig/issues/23463#issuecomment-2781312912).

[GalaxySnail]

Also discussed in #10776 and #23514. Any of seccomp, eBPF, or LSMs can return their own error codes under the correct conditions. As said in #10776:

Furthermore a new release of the kernel may return new error codes. Figuring out the actual set of error codes each system call may return is a "Sisyphean" task that is best left unattempted.

[alexrp]

I guess I'll take a stab at this since it's been accepted.

[alexrp]

We discussed this in the meeting today and decided that we'll start by changing all => unreachable to => return error.Unexpected in std.posix and std.os.

We'll then consider a follow-up enhancement: Add an std.Options field which enables panicking in code paths that, barring silly seccomp behavior and the like, always indicate programmer error.

[alexrp]

We discussed this in the meeting today and decided that we'll start by changing all => unreachable to => return error.Unexpected in std.posix and std.os.

Just noting that I'm postponing this until wrangle-writer-buffering is merged to avoid making Andrew's work on this branch even more of a slog than it already is.