-
Notifications
You must be signed in to change notification settings - Fork 83
Make add() safe #256
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
Make add() safe #256
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,9 +28,7 @@ | |
| //! | ||
| //! // Create a poller and register interest in readability on the socket. | ||
| //! let poller = Poller::new()?; | ||
| //! unsafe { | ||
| //! poller.add(&socket, Event::readable(key))?; | ||
| //! } | ||
| //! poller.add(&socket, Event::readable(key))?; | ||
| //! | ||
| //! // The event loop. | ||
| //! let mut events = Events::new(); | ||
|
|
@@ -343,16 +341,13 @@ impl Event { | |
| /// | ||
| /// ``` | ||
| /// use std::{io, net}; | ||
| /// // Assuming polling and socket2 are included as dependencies in Cargo.toml | ||
| /// use polling::Event; | ||
| /// use socket2::Type; | ||
| /// | ||
| /// fn main() -> io::Result<()> { | ||
| /// let socket = socket2::Socket::new(socket2::Domain::IPV4, Type::STREAM, None)?; | ||
| /// let poller = polling::Poller::new()?; | ||
| /// unsafe { | ||
| /// poller.add(&socket, Event::new(0, true, true))?; | ||
| /// } | ||
| /// poller.add(&socket, Event::new(0, true, true))?; | ||
| /// let addr = net::SocketAddr::new(net::Ipv4Addr::LOCALHOST.into(), 8080); | ||
| /// socket.set_nonblocking(true)?; | ||
| /// let _ = socket.connect(&addr.into()); | ||
|
|
@@ -495,12 +490,6 @@ impl Poller { | |
| /// will act as if the source was registered with another [`Poller`], with the same caveats | ||
| /// as above. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The source must be [`delete()`]d from this `Poller` before it is dropped. | ||
| /// | ||
| /// [`delete()`]: Poller::delete | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// This method returns an error in the following situations: | ||
|
|
@@ -520,13 +509,12 @@ impl Poller { | |
| /// let key = 7; | ||
| /// | ||
| /// let poller = Poller::new()?; | ||
| /// unsafe { | ||
| /// poller.add(&source, Event::all(key))?; | ||
| /// } | ||
| /// poller.add(&source, Event::all(key))?; | ||
| /// poller.delete(&source)?; | ||
| /// # std::io::Result::Ok(()) | ||
| /// ``` | ||
| pub unsafe fn add(&self, source: impl AsRawSource, interest: Event) -> io::Result<()> { | ||
| // TODO: At the next breaking change, change `AsRawSource` to `AsSource` | ||
| pub fn add(&self, source: impl AsRawSource, interest: Event) -> io::Result<()> { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not familiar with the
... and this API does exactly that, by taking 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 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 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As discussed in #255, the worst thing that can happen at the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I said, I'm not very familiar with the internals of 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 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.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, right, the edge state is per epoll instance. That part should be fine then. But as you say there may be other interactions.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @notgull Reading through the arguments here, I'm much less certain now that this is a good idea. Sorry! |
||
| self.add_with_mode(source, interest, PollMode::Oneshot) | ||
| } | ||
|
|
||
|
|
@@ -535,17 +523,11 @@ impl Poller { | |
| /// This is identical to the `add()` function, but allows specifying the | ||
| /// polling mode to use for this socket. | ||
| /// | ||
| /// # Safety | ||
| /// | ||
| /// The source must be [`delete()`]d from this `Poller` before it is dropped. | ||
| /// | ||
| /// [`delete()`]: Poller::delete | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// If the operating system does not support the specified mode, this function | ||
| /// will return an error. | ||
| pub unsafe fn add_with_mode( | ||
| pub fn add_with_mode( | ||
| &self, | ||
| source: impl AsRawSource, | ||
| interest: Event, | ||
|
|
@@ -588,7 +570,7 @@ impl Poller { | |
| /// # let source = std::net::TcpListener::bind("127.0.0.1:0")?; | ||
| /// # let key = 7; | ||
| /// # let poller = Poller::new()?; | ||
| /// # unsafe { poller.add(&source, Event::none(key))?; } | ||
| /// # poller.add(&source, Event::none(key))?; | ||
| /// poller.modify(&source, Event::all(key))?; | ||
| /// # std::io::Result::Ok(()) | ||
| /// ``` | ||
|
|
@@ -600,7 +582,7 @@ impl Poller { | |
| /// # let source = std::net::TcpListener::bind("127.0.0.1:0")?; | ||
| /// # let key = 7; | ||
| /// # let poller = Poller::new()?; | ||
| /// # unsafe { poller.add(&source, Event::none(key))?; } | ||
| /// # poller.add(&source, Event::none(key))?; | ||
| /// poller.modify(&source, Event::readable(key))?; | ||
| /// # poller.delete(&source)?; | ||
| /// # std::io::Result::Ok(()) | ||
|
|
@@ -613,7 +595,7 @@ impl Poller { | |
| /// # let poller = Poller::new()?; | ||
| /// # let key = 7; | ||
| /// # let source = std::net::TcpListener::bind("127.0.0.1:0")?; | ||
| /// # unsafe { poller.add(&source, Event::none(key))? }; | ||
| /// # poller.add(&source, Event::none(key))?; | ||
| /// poller.modify(&source, Event::writable(key))?; | ||
| /// # poller.delete(&source)?; | ||
| /// # std::io::Result::Ok(()) | ||
|
|
@@ -626,7 +608,7 @@ impl Poller { | |
| /// # let source = std::net::TcpListener::bind("127.0.0.1:0")?; | ||
| /// # let key = 7; | ||
| /// # let poller = Poller::new()?; | ||
| /// # unsafe { poller.add(&source, Event::none(key))?; } | ||
| /// # poller.add(&source, Event::none(key))?; | ||
| /// poller.modify(&source, Event::none(key))?; | ||
| /// # poller.delete(&source)?; | ||
| /// # std::io::Result::Ok(()) | ||
|
|
@@ -681,7 +663,7 @@ impl Poller { | |
| /// let key = 7; | ||
| /// | ||
| /// let poller = Poller::new()?; | ||
| /// unsafe { poller.add(&socket, Event::all(key))?; } | ||
| /// poller.add(&socket, Event::all(key))?; | ||
| /// poller.delete(&socket)?; | ||
| /// # std::io::Result::Ok(()) | ||
| /// ``` | ||
|
|
@@ -719,9 +701,7 @@ impl Poller { | |
| /// let key = 7; | ||
| /// | ||
| /// let poller = Poller::new()?; | ||
| /// unsafe { | ||
| /// poller.add(&socket, Event::all(key))?; | ||
| /// } | ||
| /// poller.add(&socket, Event::all(key))?; | ||
| /// | ||
| /// let mut events = Events::new(); | ||
| /// let n = poller.wait(&mut events, Some(Duration::from_secs(1)))?; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.