-
Notifications
You must be signed in to change notification settings - Fork 225
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
SockRef::from
, Socket::sendfile
and other functions that operate on arbitrary file descriptors or SOCKET
s potentially should be unsafe
#218
Comments
I have no idea what you mean with private, but if you mean shouldn't be modified by external (outside of the type's
The Furthermore you're dereferencing a raw pointer using So, I do agree that creating |
Fair. I think at least something in the |
It turns out that rust-lang/rust#72175 is still being discussed, and looks like it may lead to an RFC. If that happens, and if the RFC is accepted, functions that operate on That said, But of course, it makes sense to wait and see how the discussion in rust-lang/rust#72175 and the possible RFC turn out before making major changes here. |
I'm working on a draft of a proposal to add new official wording about In particular, this proposal would mean functions like |
I don't have time to look through the entire proposal, but
The second sentence says to me that the implementation needs to ensure the file descriptor is valid (while the object is alive). Unfortunately I think the problem is that the creation of |
Instead of making it
And to be sure, this isn't urgent; I'm still in the process of exploring the options. |
I don't think socket2 should dependencies outside of libc/winapi. Furthermore exposing those dependencies in a public API is a bad idea as we're then tied to a fixed version of the crate (i.e. we couldn't update to unsafe-io v2 without also updating to socket2 v2). I'm also not convinced that
|
Right now, it's possible for code in one library to use functions like One option would be to propose adding the parts of unsafe-io needed to fix this to |
This is not true. If the encapsulation implements
It's not a loophole, it's by design. It's required to deal with C/Unix/Windows I/O model of using integers/void pointers as identifiers to a kernel file description.
I still don't get the point of |
Suppose library A holds a let s = socket2::SockRef::from(&4);
let mut buf = vec![std::mem::MaybeUninit::new(0u8); 32];
s.peek(&mut buf[..])?; This compiles on stable Rust today. B can read A's otherwise encapsulated data, with no Another example would be B accidentally holding a There's a way to fix this:
|
This comment by withoutboats (not pinging) points to the core issue: rust-lang/rust#76969 (comment), that We should clearly document this footgun of using integer literals as |
Integer literals are one example. Use-after- I agree, we could better document the current situation. At the same time, this problem has the same form as problems with raw pointers, such as dangling and aliasing, which Rust does more than just document. Fixing it seems desirable in principle. And it seems possible. Is it practical? I agree with withoutboats; the underlying problem is in std. |
I strongly disagree. The situation here is entirely analogous:
Again, consider the analogy with references and integers: just because Taking a step back, there are two options for how the Rust ecosystem as a whole could treat raw FDs:
Both options are self-consistent and internally coherent. The standard library quite clearly took option 2, as a consequence
Here you are presuming option 1 -- but that is simply not the option the standard library took. Under option 2, there is nothing wrong with creating a |
I see your point and partially agree. I don't working with fd should be as unsafe as it can/should never cause memory unsafety as the OS always checks the validity of the file descriptor and returns an error if it's invalid. But I do see the point your making.
I have to disagree here. If a type needs to hold some invariant it shouldn't allow arbitrary access to the underlying data. Take
I see your point. What do you (not just @RalfJung) suggest the new API would look like? I have some requirements:
|
To eliminate the need for a dependency, I've now added the use std::io::OwnsRaw;
...
impl<'s, S> From<&'s S> for SockRef<'s>
where
S: AsRawFd + OwnsRaw, That would meet all three requirements. Does that look reasonable? |
I think so after a could look at the RFC (https://github.com/sunfishcode/rfcs/blob/3abe7cc704decf63beee16489123e0f2c996f6f3/text/0000-io-safety.md). Also a side note: you might want to include (parts of) @RalfJung comment #218 (comment) in the RFC. In the mean time any suggestions? We could simply add a trait like |
Cool, and good idea, I've now incorporated more of those ideas.
You could do that if you want, however I also think it'd be ok to just wait to see how the RFC goes before making any changes here. |
Note that the file descriptor in
I guess your proposal uses the (Moved other things to the IRLO thread.) |
What I meant was that
I was referring to mutable access to
Exactly, that's why it only implements
Could you link to that I can't find it. |
I think here we are getting to the bottom of the problem -- the ownership associated with the return value of You interpret it to be actually returning some form of ownership (even though I am not sure what the I interpret it to be like this: type RawInt = usize;
pub fn as_raw_integer<T>(x: &mut T) -> RawInt { x as *mut _ as usize } IOW, I do not think any ownership is returned here. I do think that there are some indications in std that That said, I also admit that the
Sure, sorry: https://internals.rust-lang.org/t/pre-rfc-i-o-safety/14585 |
I interpret it not about ownership, but it's about access. With
Indeed that why
In a sense, but file descriptor are of course special. They aren't about the value themselves, but what give you access to the file description. Returning a file descriptor (to me at least) also implicitly returning access to the file description.
I agree
I indeed think file descriptor can be exclusively owned.
The API in question An ideal trait, perhaps as a safer alternative to struct BorrowedFd<'fd> {
fd: RawFd,
_lifetime: PhantomData<&'fd RawFd>, // Lifetime of the file descriptor we're borrowing from
}
trait AsBorrowedFd {
// Perhaps some types need mutable access, so `&'fd mut self`.
fn as_borrow_fd<'fd>(&'fd self) -> BorrowedFd<'fd>;
}
|
One could say the same thing about pointers. :) You don't care about the address itself, you care about the data the ptr gives you access to. FDs might be special but not more special than pointers. In Rust, we all agree that returning a
The thing is, without the "I/O safety" proposal, FDs cannot be exclusively owned. You need to accept that proposal to even have the notion of "owning an FD" in the vocabulary of Rust type invariants. If anyone can just take the integer "5" and treat it as an FD and write to it, all in safe code -- that is a counterexample to ownership of FDs.
You can also do It looks like
|
Indeed both pointer and fds are special cases on integers.
But we can't write to integer 5 without unsafe code (well we can with the current
Agreed.
Yes.
Great.
Done.
|
Pr #237 adds an example and some more docs around the unsafe/unsound usage of |
Could we add an |
@notgull Anything that implements |
Looks like I/O safety might be stabilised in 1.62: rust-lang/rust#95118. |
I/O Safety has been stabilized in 1.63.0. I would be willing to write a PR that changes this crate to use the new I/O-safe traits, if the MSRV bump is acceptable. |
@notgull we're going to bump to 1.63 in v0.5 for that reason, see #320. We do have to be careful about the change in API though, we'll want to give people and nice path to upgrade. So, maybe we add the owned variants of the traits and leave the current (unsound) ones in place an deprecate them, removing them in the next version. |
See rust-lang/rust#72175 and nix-rust/nix#1421: it seems an external library is allowed to assume a file descriptor is private, for example:
Here, the safe
SockRef::from
allows to crash a (fake) "external library" by letting it dereference a null pointer using only safe code. Making functions that accept arbitrary file descriptors orSOCKET
s unsafe can probably solve this problem.The text was updated successfully, but these errors were encountered: