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

why is it possible to create a mio TcpStream from std TcpStream but going the other way around is unsafe? #1754

Closed
softstream-link opened this issue Jan 22, 2024 · 13 comments · Fixed by #1767

Comments

@softstream-link
Copy link

softstream-link commented Jan 22, 2024

Example:

unsafe { std::net::TcpStream::from_raw_fd(mio::net::TcpStream::connect(...).unwrap().into_raw_fd()) }

however this is perfectly safe

mio::net::TcpStream::from_std( std::net::TcpStream::connect(....).unwrap())

@Thomasdezeeuw
Copy link
Collaborator

Because your using FromRawFd::from_raw_fd, which is unsafe.

@softstream-link
Copy link
Author

Perhaps I should have rephrased it as why is a save function not included in the mio implementation, surely to go from std to mio stream it is also unsafe and is likely, not sure that is the case, also done via file descriptors. However to go from std to mio is provided as a safe method in the mio interface but not the other way around. I have a use case where I need to clone tcp stream so that I can use a separate handle for the read thread vs write thread however because I use mio I end up fist creating a std stream using unsafe method and then cloning and then going back to mio using provided save method. Is it possible for mio to provide an api which includes a safe method to go to std or there is a reason it is left unsafe?

@Thomasdezeeuw
Copy link
Collaborator

surely to go from std to mio stream it is also unsafe and is likely, not sure that is the case, also done via file descriptors.

Why would this be unsafe?

However to go from std to mio is provided as a safe method in the mio interface but not the other way around. I have a use case where I need to clone tcp stream so that I can use a separate handle for the read thread vs write thread however because I use mio I end up fist creating a std stream using unsafe method and then cloning and then going back to mio using provided save method. Is it possible for mio to provide an api which includes a safe method to go to std or there is a reason it is left unsafe?

We simply didn't implement this, we add something like it though. I think we can implement From<mio::net::TcpStream> for std::net::TcpStream.

@softstream-link
Copy link
Author

That would be great!

@Thomasdezeeuw
Copy link
Collaborator

Pr for it would be welcome.

@tglane
Copy link
Contributor

tglane commented Mar 22, 2024

Hey, I would like to work on this.

I think we should also implement the From trait for the other socket types as well? @Thomasdezeeuw, what's your opinion on this?

@Thomasdezeeuw
Copy link
Collaborator

Hey, I would like to work on this.

👍

I think we should also implement the From trait for the other socket types as well? @Thomasdezeeuw, what's your opinion on this?

Yes I think we can.

@softstream-link
Copy link
Author

Also worth noting that the reason I need to go back and forth between std & mio Steam is because I cannot clone a MIO stream, perhaps cloning it would also be a good idea to implement and not just going back and forth for the sake of using std Steam clone method.

@Thomasdezeeuw
Copy link
Collaborator

@softstream-link I wouldn't clone any file descriptors used with Mio. epoll(2) on Linux does not handle cloned file descriptors properly and Mio can't do anything for this.

@softstream-link
Copy link
Author

@Thomasdezeeuw

What is the issue? Do you know where I can read more about it?

Because that is kind of exactly what I am doing and I have not seen the problem.

My steps are:

  1. accept non blocking Mio Stream
  2. convert to std like so
  3. try_clone using std Stream and convert using safe back to std like so
  4. Use one mio Stream to register with mio poll like so
  5. Use second mio Stream to only write from a separate user thread and never register with poll

Is the issue you are describing affecting epoll scenarios when you register both of the clones with mio poll or it also including my case?

@Thomasdezeeuw
Copy link
Collaborator

What is the issue? Do you know where I can read more about it?

See the manual page: https://www.man7.org/linux/man-pages/man7/epoll.7.html, can't link directly but see the "Will closing a file descriptor cause it to be removed from all epoll interest lists?" question in the Q&A section.

@tglane
Copy link
Contributor

tglane commented Mar 22, 2024

See the manual page: https://www.man7.org/linux/man-pages/man7/epoll.7.html, can't link directly but see the "Will closing a file descriptor cause it to be removed from all epoll interest lists?" question in the Q&A section.

Keep in mind that this will only remove the one file descriptor and not the duplicate from the cloned TcpStream. Duplicating a file descriptor in linux will create a second FD to the same underlying handle in the kernel. When one of the FDs that are associated to the same handle is closed and removed from the poll set the other FD to the underlying handle and its epoll registration is still valid.

See the man page you linked under the Q&A section:

What happens if you register the same file descriptor on an
epoll instance twice?
You will probably get EEXIST. However, it is possible to add
a duplicate (dup(2), dup2(2), fcntl(2) F_DUPFD) file
descriptor to the same epoll instance. This can be a useful
technique for filtering events, if the duplicate file
descriptors are registered with different events masks.

@softstream-link
Copy link
Author

In my case it is only possible to close the original or clone by dropping the writer Stream which is the only available to the user and when it is dropped it issues a shutdown on the stream which I understand will propagate to clone per rust documentation, which in turn shall trigger the epool to indicate Readable event which yields no bytes, which means no further reads will produce data, at which point I also deregister the clone from the mio poll/backed by epoll on linux and that makes it ok, I think.

Do you think my logic is solid? There fore the issue describe does not apply to my case.

I wonder if that is the reason why there is no safe method to convert mio back to std, as it would open up this potential undefined behavior because std can clone.

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.

3 participants