Enforce IO safety

[notgull]

It is possible to drop a file descriptor registered in the Poll before it's deregistered, which violates I/O safety.

[ids1024]

You can see this with something like this.

use calloop::{Interest, Mode, PostAction, Readiness, Token, TokenFactory};
use std::fs::File;
use std::os::unix::io::AsFd;

struct PollBadFd<T: AsFd>(Option<T>);

impl<T: AsFd> calloop::EventSource for PollBadFd<T> {
    type Event = calloop::Readiness;
    type Metadata = ();
    type Ret = ();
    type Error = std::io::Error;

    fn process_events<F>(
        &mut self,
        _: Readiness,
        _: Token,
        _: F,
    ) -> Result<PostAction, std::io::Error> {
        Ok(PostAction::Continue)
    }

    fn register(
        &mut self,
        poll: &mut calloop::Poll,
        token_factory: &mut TokenFactory,
    ) -> Result<(), calloop::Error> {
        if let Some(fd) = self.0.take() {
            let token = token_factory.token();
            // Fd is closed on drop after this
            poll.register(fd, Interest::READ, Mode::Level, token)
        } else {
            Ok(())
        }
    }

    fn reregister(
        &mut self,
        _: &mut calloop::Poll,
        _: &mut TokenFactory,
    ) -> Result<(), calloop::Error> {
        Ok(())
    }

    fn unregister(&mut self, _: &mut calloop::Poll) -> Result<(), calloop::Error> {
        Ok(())
    }
}

fn main() {
    let mut event_loop = calloop::EventLoop::try_new().unwrap();
    let handle = event_loop.handle();

    let file = File::open("/dev/random").unwrap();
    handle.insert_source(PollBadFd(Some(file)), |_, _, _| {}).unwrap();

    event_loop.run(None, &mut (), |_| {}).unwrap();
}

Built with RUSTFLAGS='--cfg polling_test_poll_backend' (or on a Unix platform without epoll/kqueue), and run in strace, this shows a flood of poll calls on an fd that is invalid, returning POLLNVAL in revents. This is technically a violation of IO soundness as defined by Rust, but as long as the fd is only polled, I don't think I can actually cause any unsound behavior. I imagine poll immediately returning with an error would interfere with an event loop based application though?

Currently it looks like https://github.com/Smithay/calloop/pull/141 simply addresses this by making Poll::register unsafe to call. But is there a way EventSource could be defined to make it safe to implement, but prevent even a broken and contrived implementation like this from making use of an invalid fd?

Standard EventSource implementations like Generic are sound to use. Since they own the fd. Probably any reasonable implementation of the trait would, but is they a good way to enforce that, without making the API more limited in an undesirable way?

[notgull]

141 also addresses it by rewriting the Generic source in a way that handles registration safely. I think that most users will use the Generic adaptor and then advanced users can use the unsafe register() function.

[ids1024]

Yeah, wrapping something like Generic should be the most common use case, so if such uses are safe it isn't too much of an issue if the lower level functions are unsafe.

[notgull]

It should also be mentioned that one of the end goals of I/O safety is to get an I/O safety checker into Miri. Even if practically there's no real undefined behavior that can occur (such as in the kqueue case), it's possible for theoretical undefined behavior to occur.

[elinorbgr]

I suppose this is now done following #141 ?

[notgull]

Yes it is