-
Notifications
You must be signed in to change notification settings - Fork 705
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
Replace Unix SocketAddr with std version #1749
base: master
Are you sure you want to change the base?
Conversation
add55da
to
fee7fcd
Compare
Replaces mio::net::SocketAddr with std::os::unix::net::SocketAddr. With this change the path based API to create the sockets ({UnixDatagram,UnixListener}::bind and UnixStream::connect) will no longer work with abstract namespaces. For that the _addr variant must be used and the address created by using SocketAddr::from_abstract_name (part of the std::os::linux::net::SocketAddrExt trait). Adds UnixDatagram::bind_addr to match UnixStream::connect_addr and UnixListener::bind_addr. Expands the unix_listener_abstract_namespace test to actually create a listener and use it. Closes #1527
fee7fcd
to
1c646e1
Compare
It seems to return a length of 16 and an all zero address for unnamed Unix addresses.
// Darwin is being weird, it return a length of 16, but other an unnamed | ||
// (all zero) address. Map that to a length of 0 to match other OS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Darwin is being weird, it return a length of 16, but other an unnamed | |
// (all zero) address. Map that to a length of 0 to match other OS. | |
// Darwin is being weird, it returns a length of 16, but otherwise an unnamed | |
// (all zero) address. Map that to a length of 0 to match other OS. |
let sockaddr = mem::MaybeUninit::<libc::sockaddr_un>::zeroed(); | ||
|
||
// This is safe to assume because a `libc::sockaddr_un` filled with `0` | ||
// bytes is properly initialized. | ||
// | ||
// `0` is a valid value for `sockaddr_un::sun_family`; it is | ||
// `libc::AF_UNSPEC`. | ||
// | ||
// `[0; 108]` is a valid value for `sockaddr_un::sun_path`; it begins an | ||
// abstract path. | ||
let mut sockaddr = unsafe { sockaddr.assume_init() }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would probably just do mem::zeroed()
here, but both are okay.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on
Lines 26 to 36 in 2d7b454
let sockaddr = mem::MaybeUninit::<libc::sockaddr_un>::zeroed(); | |
// This is safe to assume because a `libc::sockaddr_un` filled with `0` | |
// bytes is properly initialized. | |
// | |
// `0` is a valid value for `sockaddr_un::sun_family`; it is | |
// `libc::AF_UNSPEC`. | |
// | |
// `[0; 108]` is a valid value for `sockaddr_un::sun_path`; it begins an | |
// abstract path. | |
let mut sockaddr = unsafe { sockaddr.assume_init() }; |
mem::zeroed
was deprecated in favour of this code using MaybeUninit
, but I think that was reverted)
#[cfg(target_os = "linux")] | ||
None => match address.as_abstract_name() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not missing Android?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, except the std::os::linux::net::SocketAddrExt
trait (where this method comes from) doesn't work on Android, even though it probably should.
#[cfg(target_os = "linux")] | ||
None => match address.as_abstract_name() { | ||
Some(name) => { | ||
offset += 1; | ||
name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that technically this change does not mean that the MSRV cannot be lower than 1.70.0. You can use a build script to detect the rustc version and only include this branch if the rustc is at least 1.70.0. This works because the user cannot construct a SocketAddr
with an abstract name on older rustc.
We used this technique in Tokio to support the IO safety traits before our MSRV supported them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's increasing the MSRV, that was the goal for Mio v1.
I'm not going to mess around with a build script. I've seen it in the log and libc crates as well as a few others and it's always been a pain to maintain them. Mio v1 is simply going to have a higher MSRV.
// SAFETY: since `addr` is a valid Unix address, it must not be larger than | ||
// `SUN_LEN` bytes, thus we won't overwrite the size of sockaddr.sun_path. | ||
// SAFETY: null byte is already written because we zeroed the address above. | ||
debug_assert!(addr.len() <= sockaddr.sun_path.len()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug_assert!(addr.len() <= sockaddr.sun_path.len()); | |
debug_assert!(offset + addr.len() <= sockaddr.sun_path.len()); |
let mut addrlen = path_offset(&sockaddr) + addr.len(); | ||
match addr.first() { | ||
Some(&0) | None => {} | ||
Some(_) => addrlen += 1, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what's going on here. Would you not add one in the case where it starts with zero?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is based on
Lines 64 to 69 in 2d7b454
match bytes.first() { | |
// The struct has already been zeroes so the null byte for pathname | |
// addresses is already there. | |
Some(&0) | None => {} | |
Some(_) => socklen += 1, | |
} |
Replaces mio::net::SocketAddr with std::os::unix::net::SocketAddr.
With this change the path based API to create the sockets ({UnixDatagram,UnixListener}::bind and UnixStream::connect) will no longer work with abstract namespaces. For that the _addr variant must be used and the address created by using SocketAddr::from_abstract_name (part of the std::os::linux::net::SocketAddrExt trait).
Adds UnixDatagram::bind_addr to match UnixStream::connect_addr and UnixListener::bind_addr.
Expands the unix_listener_abstract_namespace test to actually create a listener and use it.
Closes #1527