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

Taking RawFd by value is not sound #594

Open
kamalmarhubi opened this issue Apr 24, 2017 · 4 comments
Open

Taking RawFd by value is not sound #594

kamalmarhubi opened this issue Apr 24, 2017 · 4 comments
Labels

Comments

@kamalmarhubi
Copy link
Member

Raised by @lucab on #562

I think these all do an implicit copy+own of the RawFd (as it is just an alias to an int), and I fear this doesn't really guarantee that the fd has not been closed/dropped in the meanwhile by a close().

This is a problem. A RawFd value may be kept long enough to end up referring to a different file. We may wish to provide a non-copy wrapper type similar to mio's EventedFd that allows enforcing the borrowing relationship. From the docs:

Note that EventedFd takes a &RawFd. This is because EventedFd does not take ownership of the FD. Specifically, it will not manage any lifecycle related operations, such as closing the FD on drop.

@lucab
Copy link
Contributor

lucab commented Apr 24, 2017

Do you know if this concern has been raised within stdlib before? I think RawFd may benefit from being Clone via dup(2), but I miss the point of Copy (at least on linux).

@kamalmarhubi
Copy link
Member Author

dup(2) can fail, eg if a file descriptor limit is reached. I don't think it makes sense to have a fallible Clone impl. And I don't think the stdlib would be interested in this change. The wording Raw indicates moving outside of Rust's guarantees. Most every occurrence of raw in the stdlib has that connotation. For this case, note that the FromRawFd trait's method is unsafe.

More likely we should introduce a nix-specific wrapper that enforces safe usage. This needs a bit of thought!

@Kixunil
Copy link
Contributor

Kixunil commented Jul 4, 2017

More likely we should introduce a nix-specific wrapper that enforces safe usage. This needs a bit of thought!

I was thinking exactly like this.

  • Keep RawFd for interfacing with other crates and raw C libraries
  • Provide Fd which is a newtype for RawFd that impls Drop and {From,Into,As}Fd traits.
  • Implement all other wrappers for Fd (files, sockets, ...). This has nice benefit of not having to impl Drop on all other types
  • Methods that are common for all fds could be implemented on Fd and then forwarded (hopefully not using Deref) delegation of implementation would help here. Or maybe even better, use a trait. (That'd also enable using dup similarly as clone.)

@roblabla
Copy link
Contributor

Related to #678

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

Successfully merging a pull request may close this issue.

5 participants