Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate to I/O safety traits #1588

Open
Thomasdezeeuw opened this issue Jun 23, 2022 · 8 comments · May be fixed by #1745
Open

Migrate to I/O safety traits #1588

Thomasdezeeuw opened this issue Jun 23, 2022 · 8 comments · May be fixed by #1745
Milestone

Comments

@Thomasdezeeuw
Copy link
Collaborator

See rust-lang/rust#87074 and sunfishcode/io-lifetimes#38.

@notgull
Copy link

notgull commented Jun 28, 2023

At polling, we're trying to do the same thing, but we've run into a problem. If you register() a Source but then drop it before deregister()ing it, a dangling file descriptor is left in the selector. This is especially a problem for the Windows and kqueue backends, which do not have provisions for automatically removing a file descriptor when it's closed.

This would be especially bad if I/O-safety is added to miri; it's the moral equivalent of dereferencing a dangling pointer. I'm not sure if there is a way around this aside from making register() unsafe. I was hoping that the mio team had a plan of action for this.

@Noah-Kennedy
Copy link

This is tough, because you can't exactly just use something semantically analogous to Arc, as you end up leaking Fds rather than dangling them. I don't think there's a good way od solving this without basically having the poller own the sockets, or the sockets share state with poll to allow for deregistration on drop.

@Thomasdezeeuw
Copy link
Collaborator Author

At polling, we're trying to do the same thing, but we've run into a problem. If you register() a Source but then drop it before deregister()ing it, a dangling file descriptor is left in the selector. This is especially a problem for the Windows and kqueue backends, which do not have provisions for automatically removing a file descriptor when it's closed.

For Mio at least kqueue won't be a problem as we don't maintain (user space) state for it. But indeed IOCP, wasm and poll do and thus become a problem.

However how do the I/O safety traits change this? I'm pretty this already the case with (Mio's) I/O types already.

This would be especially bad if I/O-safety is added to miri; it's the moral equivalent of dereferencing a dangling pointer. I'm not sure if there is a way around this aside from making register() unsafe. I was hoping that the mio team had a plan of action for this.

I don't think it really changes much from Mio's perspective, it's currently already required to deregister resource properly.

This is tough, because you can't exactly just use something semantically analogous to Arc, as you end up leaking Fds rather than dangling them. I don't think there's a good way od solving this without basically having the poller own the sockets, or the sockets share state with poll to allow for deregistration on drop.

I rather keep the I/O type just the fds, especially on epoll/kqueue where this isn't a problem.

@fogti
Copy link

fogti commented Jun 28, 2023

on epoll where this isn't a problem.

didn't epoll also have a problem with this when file descriptors get cloned (or used in forked processes)?

@notgull
Copy link

notgull commented Jun 29, 2023

For Mio at least kqueue won't be a problem as we don't maintain (user space) state for it. But indeed IOCP, wasm and poll do and thus become a problem.

Unfortunately kqueue keeps track of resources through their file descriptor rather than their file description, meaning that the fd is morally borrowed by the API for the duration of its registration. See here for further discussion that led to this conclusion.

However how do the I/O safety traits change this? I'm pretty this already the case with (Mio's) I/O types already.

Right now it's just a correctness issue, but now with I/O safety it becomes a soundness issue, meaning that unsafe would have to be involved.

I don't think it really changes much from Mio's perspective, it's currently already required to deregister resource properly.

Ditto as above.

@Thomasdezeeuw
Copy link
Collaborator Author

Thomasdezeeuw commented Jun 29, 2023

@notgull

Unfortunately kqueue keeps track of resources through their file descriptor rather than their file description, meaning that the fd is morally borrowed by the API for the duration of its registration. See here for further discussion that led to this conclusion.

Doesn't that discussion reach the same conclusion that I did? That closing the fd without deregisting isn't an issue, even in case of duplication, because kqueue works with file descriptors (user space) not on file descriptions (kernel space). For epoll it's the opposite as it does work on file descriptions, so duplicating a file descriptor means you'll still get events for it until both fds (and thus the file description) are deregistered or closed.

However how do the I/O safety traits change this? I'm pretty this already the case with (Mio's) I/O types already.

Right now it's just a correctness issue, but now with I/O safety it becomes a soundness issue, meaning that unsafe would have to be involved.

I don't think it really changes much from Mio's perspective, it's currently already required to deregister resource properly.

Ditto as above.

Where does the unsoundness come from?

@fogti

on epoll where this isn't a problem.

didn't epoll also have a problem with this when file descriptors get cloned (or used in forked processes)?

Yes, also see above, but the worst of it is that we get events for a Token that is not really used any more. Compared to the poll/wasm implementation where we pass an fd to the poll(2) call that is closed (or reused). That's a bigger problem as that can cause Poll:poll to return an error where you wouldn't really expect one, furthermore you can't fix the error because you don't know which file descriptor is causing the issues, so you can't deregister it to fix the problem.

@fogti
Copy link

fogti commented Jun 29, 2023

that is really used any more

which has the added side effect that we can't deregister it to fix the problem in that case also.

but overall I don't think the differences between the models matter much here, because it causes issues regardless of that (and potential pre-denial-of-service like states where we are bombarded with events or errors which we can't fix, causing unnecessary load which can't be fixed without reinitializing the entire reactor and re-registering all actually wanted fds(handles again.

@Thomasdezeeuw
Copy link
Collaborator Author

which has the added side effect that we can't deregister it to fix the problem in that case also.

but overall I don't think the differences between the models matter much here, because it causes issues regardless of that (and potential pre-denial-of-service like states where we are bombarded with events or errors which we can't fix, causing unnecessary load which can't be fixed without reinitializing the entire reactor and re-registering all actually wanted fds(handles again.

Note that I don't think Mio is not susceptible to those kind of DOS attacks since we use edge triggers, in the worst case we get a single event, maybe two if we get one for readable and another for closed/error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants