I/O safety of signal-hook self-pipe APIs

[hanna-kruppe]

In recent years, Rust has adopted a notion of I/O safety that conflicts with some traditional APIs dealing with raw file descriptors (or Win32 handles). In particular, std::io documentation says:

To uphold I/O safety, it is crucial that no code acts on file descriptors it does not own or borrow, and no code closes file descriptors it does not own. In other words, a safe function that takes a regular integer, treats it as a file descriptor, and acts on it, is unsound.

Some safe APIs in signal-hook predate this decree and conflict with it:

• signal_hook::low_level::pipe::register
• signal_hook::low_level::pipe::register_raw
• signal_hook::iterator::backend::SignalDelivery::with_pipe

All of these take an FD as RawFd (alias for c_int) directly or via conversion traits , so they can be called with any plain old integer and are thus considered unsound today. For example, pipe::register_raw(signal, 1) compiles, puts stdout into non-blocking mode, and writes an X to stdout whenever the signal occurs. Unfortunately there's no way to fix this without a semver-breaking change (i.e., bumping signal-hook 0.3 -> 0.4):

• Mark these functions unsafe and pass the I/O safety requirements to callers. This is a breaking change, and requires all callers to write some extra unsafe themselves. This I/O safety concept also isn't the most intuitive in practice (e.g., it's non-obvious that impl IntoRawFd is the wrong trait bound for a safe function taking ownership of an FD), so I wouldn't be surprised if it just pushes the unsoundness into callers.
• Change the parameter types / trait bounds to make the functions safe. For example, I believe pipe::register_raw could accept OwnedFd (since it ultimately closes the FD if the action is later unregistered). This is also a breaking change, harder to get right on the library side, and requires bumping MSRV to 1.66 to get access to the new vocabulary types. On the bright side, it's more friendly to users of the library in the long run, since it allows doing a lot of things without any unsafe (since many types in std and the ecosystem implement AsFd and Into<OwnedFd>).

[vorner]

Hmm, I'm starting to feel old… back when I was writing the library, there wasn't such a thing around I think and I'm not sure that messing with „random file descriptors“ was considered within the Rust safety guarantees - in that sense, it would have been „fine“ to just document the function does take the ownership. I don't think it was documented to be considered unsound back than…

Being grumpy aside. It sounds like something worth fixing. And I think it's fine to bump version / MSRV at the signal-hook crate (unlike signal-hook-registry). The signal-hook-registry kind of needs to exist in just one instance (therefore, only one version) in the dependency graph, but signal-hook can exist in multiple parallel versions (all of them depending on the same signal-hook). The split exists exactly for this reason - that I wanted to be able to eventually bump the version of signal-hook that contains the larger part of the API. Bumping to 3 years old version (1.66) here seems fine.

Should I take a look at it, or do you want to take it?

[hanna-kruppe]

Your memory is correct, this whole I/O safety business is a relatively new idea (the RFC dates to 2021) that retroactively declared a lot of previously-fine APIs "unsound" and has caused some churn across the ecosystem. But given that large parts of the ecosystem have adapted by now, I agree it makes sense to adapt signal-hook as well.

Should I take a look at it, or do you want to take it?

I might take a stab at it if it turns out easy, but no promises. I'm not using signal-hook myself (only signal-hook-registry) so I haven't looked at its implementation much and don't have much motivation for spending much time on doing so.

[vorner]

I hope it should be kind of easy - just replacing the external interface with just one function taking OwnedFd, or taking From. Then the implementation of WakeFd more or less matches OwnedFd - it could either wrap it (and remove the Drop implementation) or just use the OwnedFd directly.

Anyway, if you decide not to have a look, just ping me, I'll take it over 😇

[hanna-kruppe]

I got far enough to fix all the compile errors from changing register_raw to take OwnedFd, pass tests on Linux, and even satisfy clippy. But now I'm out of steam and allotted time with this. Feel free to take over my WIP branch and finish whatever is still missing (at the very least, the version matrix tested by CI needs changes).

[vorner]

Thank you, I've kept your code intact, only dealt with the tests and published.

[hanna-kruppe]

Thank you, too! I hate to be the bearer of bad news but signal-hook 0.4 pins libc and cc versions and someone's bound to complain about this conflicting with their own version requirements as soon as they upgrade signal-hook. It would definitely be a problem for the project where I'm using signal-hook-registry if I was also using signal-hook there.

[vorner]

Maybe I should pay more attention to things :-|. Fixed in 0.4.1.