-
Notifications
You must be signed in to change notification settings - Fork 654
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
nix 0.27 Epoll
API should be marked unsafe
#2115
Comments
Fd: AsFd
the write API for Epoll::add() etc?
There is an explanation behind the purpose of Nix is moving to this approach, it is unfortunately slow however which can introduce inconsistency that makes it a bit awkward. |
Fd: AsFd
the write API for Epoll::add() etc?Fd: AsFd
the right API for Epoll::add() etc?
hmm, it's all a bit awkward. cf smol-rs/polling#123 in which
I think that's probably more principled. Unless I misunderstand, this crate is currently exposing a purportedly safe API - but callers still can add file descriptors to an Is that true? |
Due to limitations in Rust's compile time evaluation I do not beleive it is currently possible to implement It is true you can add a file descriptor with I am not sure what code changes would make sense here to better illustrate this. |
I don't think it's only a documentation thing: it's unsafe but the API has no The suggestion - via That would be the place to put the documentation saying that callers are responsible for making sure that they |
I would suggest using I do not disagree with adding
I do not think this actually matters. If you attempt to de-register a file descriptor that isn't registered it is safe. It will error, but it is safe. The safety concern can be written
|
cool, I think we're agreeing. I don't intend to submit an MR, but I'll update the title and thread-opener to clarify where we've got to. |
With your update
They do not need to call |
Fd: AsFd
the right API for Epoll::add() etc?Epoll
API should be marked unsafe
I think that struct Epoll<'fd> {
/// The actual epoll file descriptor
pollfd: OwnedFd,
PhantomData<'fd>
}
impl<'fd> Epoll {
pub fn add<Fd: AsFd + 'fd>(&self, fd: &Fd) -> Result<()> {...}
pub unsafe fn add_raw(&self, fd: RawFd) -> Result<()> {...}
} |
I reckon that just doesn't match how Certainly in my code - but, I claim, this is the typical and intended use - the pattern is a long-lived The corresponding analysis in |
There are definitely APIs which can be used with I/O Safety, but in their most compelling use cases require the
|
#2232 will probably fix this issue |
... but does it also make |
Well, I guess so. And unfortunately, even though you remove a fd from the Epoll with use std::fs::File;
use std::marker::PhantomData;
use std::os::fd::{AsFd, BorrowedFd};
struct FdSet<'a> {
_marker: PhantomData<*mut &'a ()>,
}
impl<'a> FdSet<'a> {
fn new() -> Self {
Self {
_marker: PhantomData
}
}
fn add(&self, fd: BorrowedFd<'a>) {}
fn remove(&self, fd: BorrowedFd<'a>) {}
}
fn main() {
let set = FdSet::new();
let file = File::open("Cargo.toml").unwrap();
set.add(file.as_fd());
set.remove(file.as_fd());
// It should be safe to drop the file here
drop(file);
drop(set);
} |
The new
Epoll
API (as at nix 0.27) should be markedunsafe
, probably onEpoll::add()
.Currently the API has no
unsafe
on it, uses types from the io-safety proposal - but is in fact unsafe. Because one can add a file descriptor to anEpoll
's set of interests, and then drop that file descriptor - which is just what io-safety was trying to prevent.Useful discussion starts at #2115 (comment).
(Original report preserved below)
I'm interacting with a C library so I have a
RawFd
all along.Previously
epoll_ctl()
etc took aRawFd
so that was very convenient. But the new API takesFd: AsFd
which is not so convenient: I have to unsafely borrow myRawFd
; and then nix is only going to.as_raw_fd()
it again anyway. It's an awkward and unnecessary dance.Fd: AsRawFd
might make more sense, I reckon that's what is actually needed?#1882, cc @JonathanWoollett-Light
The text was updated successfully, but these errors were encountered: