Make add() safe

[notgull]

As per discussion in #255, the add() function can be made safe with no
other modifications to the API. This allows the polling crate to be
used entirely safely again.

I don't like AsRawSource still being in the public API, but that
can't really be helped at this point. If we do another breaking change
in the future this can be fixed.

[notgull]

Is this an stdlib bug? Filed rust-lang/rust#150294

[zeenix]

The change itself seems good. Please also add details to commit messages so all details of changes are available offline in git history. 🙏

[hanna-kruppe]

I'm not familiar with the polling crate, but I don't think this is signature is sound w.r.t. I/O safety, even outside the "backend may still poll the fd after it's closed" aspect discussed in #255. Standard library documentation says explicitly:

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.

... and this API does exactly that, by taking impl AsRawSource which includes RawFd = c_int. For example, poller.add(666, ev) acts on the regular integer 666 as if was an fd.

Note that it's not enough that there happens to be an fd with this numeric value at the time of call. Whoever "owns" the fd must consent to giving up exclusive ownership for as long as the poller may still use it, by either passing ownership of the fd (unlikely since they'd still want to act on it when getting events) or borrowing the fd to the poller. Conversely, they must promise to not close the fd while the poller is using it, otherwise something else could reuse the fd for something that's supposed to be owned). For example, a function like this would violate I/O safety because a caller who has an OwnedFd would be allowed to close the fd after calling this function:

fn add_to_poller(fd: BorrowedFd<'_>, poller: &Poller, interest: Event) -> io::Result<()> {
    // SAFETY: `fd` is a valid file descriptor
    unsafe {
        poller.add(fd.as_raw_fd(), interest)
    }
}

Edit: also, due to lifetime aspect (how long the fd is borrowed), I don't think changing the bound to AsSource will suffice to make this function safe. Nothing about this API bounds how long the fd is borrowed, so you could write something like:

let fd: OwnedFd = /* ... */;
poller.add(fd.as_fd(), interest);
drop(fd);

... and then the poller will act on an fd that may have been reused for something else entirely, whose owner relies on I/O safety granting them exclusive ownership of that fd.

[notgull]

As discussed in #255, the worst thing that can happen at the polling boundary is that the wrong event gets delivered. This is an acceptable compromise to ensure public API safety, especially since (as discussed up-thread) I/O safety is more of a suggestion.

[hanna-kruppe]

As I said, I'm not very familiar with the internals of polling nor with all its backends. If there's really no way that spurious polling of a random integer that happens to match the numeric value of someone's OwnedFd can negatively impact what the owner is doing with their fd, fair enough. But this doesn't really seem obvious to me from the discussion in #255 and from what I know about e.g. epoll.

Past discussion here and here was about what happens if you have an fd, add it to an epoll instance, and then close the fd. I understand why that is fine. However, it's not clear to me that it's a non-issue for other backends where the OS won't automatically remove the fd on close. Even with epoll, it seems surprising and potentially problematic if an OwnedFd you just created and never shared with anyone was already added (causing you to get EEXIST when trying to add it) or modified/deleted (e.g., causing you to miss events you'd otherwise be guaranteed to get). There's also at least one epoll flag (EPOLLEXCLUSIVE) that has spooky action at a distance between different epoll instances that all contain the same fd, though I think this one's harmless as long as polling doesn't use it itself (but will that also be true for the next epoll flag Linux adds in the future?).

I don't think it's necessarily unreasonable to say these kinds of problems aren't safety concerns and ergonomics win. But that does means that polling is potentially incompatible with other libraries (existing or yet-to-be-written) that try to do fancy things with polling based on I/O safety. If you say the worst thing that can happen is delivering wrong events, I can see where you're coming from, but it seems a bit too simplistic when considering composability of separately-developed safe abstractions.

especially since (as discussed up-thread) I/O safety is more of a suggestion.

I don't see any discussion on this PR (or in #255 for that matter) that I would summarize as "I/O safety is more of a suggestion". Perhaps you're referring to @RalfJung explaining that I/O safety is library invariants and thus can't be checked by miri, and that there's currently no satisfying way to account for fds passed by env variables. But taking that to mean "I/O safety is just a suggestion" seems like a big leap to me.

[RalfJung]

Regarding the question of whether it is safe to poll on file descriptors you don't know -- I would say no. When one uses edge triggers, it is crucial to control all the polling to an FD, or else one may miss some events and fail to wake up.

But in the end this is something the libs-api team has to define, they "own" the "protocol" of what exactly is and is not allowed under I/O safety. I would suggest opening an issue in the Rust repo.

[hanna-kruppe]

I'm not actually sure if edge triggered polling is relevant in this case. At least with the epoll backend, my understanding is that epoll instances are independent w.r.t. event delivery (other than EPOLLEXCLUSIVE) and level/edge choice, i.e., edge-triggered polling of an fd in your own epoll instance still gets all the events even if the same fd was added to another epoll instance via polling. I don't think polling itself makes any API guarantees that would make it sound to build any safety invariants on reasoning like "I added it fd that I own and didn't share with anyone to this Poller, thus [...]" -- e.g., if a bug in polling causes it to sometimes not poll an fd that was registered, this probably wouldn't be treated as a soundness bug in the library.

I don't have encyclopedic knowledge of different polling APIs (and even my epoll knowledge is limited) so maybe I'm wrong or there are similar problems with other polling backends. But as long as there's no side effects on other uses of the same fd, then it's probably harmless w.r.t. I/O safety. However, that's a rather open-ended question, and the answer may change as operating systems add new features and APIs (cf. EPOLLEXCLUSIVE added in Linux 4.5). Asking libs-api seems like a good idea in any case.

[RalfJung]

Ah, right, the edge state is per epoll instance. That part should be fine then. But as you say there may be other interactions.

[zeenix]

@notgull Reading through the arguments here, I'm much less certain now that this is a good idea. Sorry!

[notgull]

@taiki-e When you have a chance, can you take another look at this one?

[RalfJung]

I/O safety is a hard invariant that code can rely on for soundness. It is indeed a library invariant, but it is definitely wrong to characterize library invariants as just a suggestion. By that argument, Vec::set_len could be made safe as the invariant on Vec is also just a suggestion.

[notgull]

Acknowledged, if this breaks soundness invariants, then this is a bad idea. Closing this PR.