Replace Unix SocketAddr with std version #1749

Thomasdezeeuw · 2023-12-24T17:47:08Z

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

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

It seems to return a length of 16 and an all zero address for unnamed Unix addresses.

Thomasdezeeuw · 2023-12-25T11:49:44Z

src/sys/unix/uds/listener.rs

+    // 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.


Suggested change

// 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.

Darksonn · 2023-12-31T11:36:57Z

src/sys/unix/uds/mod.rs

+    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() };


I would probably just do mem::zeroed() here, but both are okay.

This is based on

mio/src/sys/unix/uds/mod.rs

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() };

, but we can probably change it. (at the time mem::zeroed was deprecated in favour of this code using MaybeUninit, but I think that was reverted)

Darksonn · 2023-12-31T11:40:23Z

src/sys/unix/uds/mod.rs

+        #[cfg(target_os = "linux")]
+        None => match address.as_abstract_name() {


Is this not missing Android?

Yes, except the std::os::linux::net::SocketAddrExt trait (where this method comes from) doesn't work on Android, even though it probably should.

Darksonn · 2023-12-31T11:40:59Z

src/sys/unix/uds/mod.rs

+        #[cfg(target_os = "linux")]
+        None => match address.as_abstract_name() {
+            Some(name) => {
+                offset += 1;
+                name


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.

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.

Darksonn · 2023-12-31T11:43:01Z

src/sys/unix/uds/mod.rs

+    // 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());


Suggested change

debug_assert!(addr.len() <= sockaddr.sun_path.len());

debug_assert!(offset + addr.len() <= sockaddr.sun_path.len());

Darksonn · 2023-12-31T11:47:12Z

src/sys/unix/uds/mod.rs

+    let mut addrlen = path_offset(&sockaddr) + addr.len();
+    match addr.first() {
+        Some(&0) | None => {}
+        Some(_) => addrlen += 1,
+    }


I don't understand what's going on here. Would you not add one in the case where it starts with zero?

This is based on

mio/src/sys/unix/uds/mod.rs

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,

}

.

Thomasdezeeuw mentioned this pull request Dec 24, 2023

Fix path validation for abstract unix socket. #1605

Closed

Thomasdezeeuw force-pushed the thomas/1527 branch from add55da to fee7fcd Compare December 24, 2023 18:02

Thomasdezeeuw force-pushed the thomas/1527 branch from fee7fcd to 1c646e1 Compare December 24, 2023 18:06

Work around Darwn (macOS) weirdness

71b80e3

It seems to return a length of 16 and an all zero address for unnamed Unix addresses.

Thomasdezeeuw commented Dec 25, 2023

View reviewed changes

Thomasdezeeuw mentioned this pull request Dec 30, 2023

Replace Unix SocketAddr with std version Thomasdezeeuw/heph#610

Open

Darksonn reviewed Dec 31, 2023

View reviewed changes

Thomasdezeeuw mentioned this pull request Dec 31, 2023

Implement I/O safety traits #1745

Open

Thomasdezeeuw mentioned this pull request Mar 14, 2024

doc: Fix link for SocketAddr #1765

Closed

Thomasdezeeuw mentioned this pull request Mar 26, 2024

Update socket types API to match std lib #1750

Open

Thomasdezeeuw added this to the v1.0 milestone Apr 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Unix SocketAddr with std version #1749

Replace Unix SocketAddr with std version #1749

Thomasdezeeuw commented Dec 24, 2023 •

edited

Thomasdezeeuw Dec 25, 2023

Darksonn Dec 31, 2023

Thomasdezeeuw Dec 31, 2023

Darksonn Dec 31, 2023

Thomasdezeeuw Dec 31, 2023

Darksonn Dec 31, 2023

Thomasdezeeuw Dec 31, 2023

Darksonn Dec 31, 2023

Darksonn Dec 31, 2023

Thomasdezeeuw Dec 31, 2023

		// 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.

	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() };

		#[cfg(target_os = "linux")]
		None => match address.as_abstract_name() {

	debug_assert!(addr.len() <= sockaddr.sun_path.len());
	debug_assert!(offset + addr.len() <= sockaddr.sun_path.len());

	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,
	}

Replace Unix SocketAddr with std version #1749

Are you sure you want to change the base?

Replace Unix SocketAddr with std version #1749

Conversation

Thomasdezeeuw commented Dec 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Thomasdezeeuw commented Dec 24, 2023 •

edited