From b5e97708f9f6f0ae9a57965ea3cb4779827fcd22 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 15:58:54 +0200 Subject: [PATCH 1/8] test_unix_msg: Remove thread synchronisation This refactors the test_unix_msg() test a bit. No change in behaviour is intended. Previously, two threads were started in parallel. The server thread used a mutex and a condition variable to indicate that it set up its listening socket. The client thread waited for this signal before it attempted to connect. This commit makes the code instead bind the server socket before starting the worker threads. That way, no synchronisation is necessary. Signed-off-by: Uli Schlachter --- tests/net/unix.rs | 77 ++++++++++++++--------------------------------- 1 file changed, 22 insertions(+), 55 deletions(-) diff --git a/tests/net/unix.rs b/tests/net/unix.rs index b69fc8453..510b5f5c7 100644 --- a/tests/net/unix.rs +++ b/tests/net/unix.rs @@ -150,31 +150,22 @@ fn test_unix_msg() { let tmpdir = tempfile::tempdir().unwrap(); let path = tmpdir.path().join("scp_4804"); - let ready = Arc::new((Mutex::new(false), Condvar::new())); + + let connection_socket = socket( + AddressFamily::UNIX, + SocketType::SEQPACKET, + Protocol::default(), + ) + .unwrap(); + + let name = SocketAddrUnix::new(&path).unwrap(); + bind_unix(&connection_socket, &name).unwrap(); + listen(&connection_socket, 1).unwrap(); let server = { - let ready = ready.clone(); let path = path.clone(); move || { - let connection_socket = socket( - AddressFamily::UNIX, - SocketType::SEQPACKET, - Protocol::default(), - ) - .unwrap(); - - let name = SocketAddrUnix::new(&path).unwrap(); - bind_unix(&connection_socket, &name).unwrap(); - listen(&connection_socket, 1).unwrap(); - - { - let (lock, cvar) = &*ready; - let mut started = lock.lock().unwrap(); - *started = true; - cvar.notify_all(); - } - let mut buffer = vec![0; BUFFER_SIZE]; 'exit: loop { let data_socket = accept(&connection_socket).unwrap(); @@ -214,14 +205,6 @@ fn test_unix_msg() { }; let client = move || { - { - let (lock, cvar) = &*ready; - let mut started = lock.lock().unwrap(); - while !*started { - started = cvar.wait(started).unwrap(); - } - } - let addr = SocketAddrUnix::new(path).unwrap(); let mut buffer = vec![0; BUFFER_SIZE]; let runs: &[(&[&str], i32)] = &[ @@ -318,31 +301,23 @@ fn test_unix_msg_with_scm_rights() { let tmpdir = tempfile::tempdir().unwrap(); let path = tmpdir.path().join("scp_4804"); - let ready = Arc::new((Mutex::new(false), Condvar::new())); let server = { - let ready = ready.clone(); let path = path.clone(); - move || { - let connection_socket = socket( - AddressFamily::UNIX, - SocketType::SEQPACKET, - Protocol::default(), - ) - .unwrap(); - let mut pipe_end = None; + let connection_socket = socket( + AddressFamily::UNIX, + SocketType::SEQPACKET, + Protocol::default(), + ) + .unwrap(); - let name = SocketAddrUnix::new(&path).unwrap(); - bind_unix(&connection_socket, &name).unwrap(); - listen(&connection_socket, 1).unwrap(); + let name = SocketAddrUnix::new(&path).unwrap(); + bind_unix(&connection_socket, &name).unwrap(); + listen(&connection_socket, 1).unwrap(); - { - let (lock, cvar) = &*ready; - let mut started = lock.lock().unwrap(); - *started = true; - cvar.notify_all(); - } + move || { + let mut pipe_end = None; let mut buffer = vec![0; BUFFER_SIZE]; let mut cmsg_space = vec![0; rustix::cmsg_space!(ScmRights(1))]; @@ -403,14 +378,6 @@ fn test_unix_msg_with_scm_rights() { }; let client = move || { - { - let (lock, cvar) = &*ready; - let mut started = lock.lock().unwrap(); - while !*started { - started = cvar.wait(started).unwrap(); - } - } - let addr = SocketAddrUnix::new(path).unwrap(); let (read_end, write_end) = pipe().unwrap(); let mut buffer = vec![0; BUFFER_SIZE]; From 93f94e0e1d861c5aab76e80c29c501af063236f6 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 16:08:42 +0200 Subject: [PATCH 2/8] Add test for abstract unix sockets This commit duplicates the existing test_unix_msg() tests. The duplicated version uses an abstract unix socket instead of a path based one. To make the code-duplication not too bad, a helper function do_test_unix_msg() is introduced that does "the actual work" and just needs as input a socket address. This new test currently fails. This reproduces https://github.com/bytecodealliance/rustix/issues/660. Signed-off-by: Uli Schlachter --- tests/net/unix.rs | 54 +++++++++++++++++++++++++++++------------------ 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/tests/net/unix.rs b/tests/net/unix.rs index 510b5f5c7..97b487faf 100644 --- a/tests/net/unix.rs +++ b/tests/net/unix.rs @@ -142,28 +142,19 @@ fn test_unix() { } #[cfg(not(any(target_os = "redox", target_os = "wasi")))] -#[test] -fn test_unix_msg() { +fn do_test_unix_msg(addr: SocketAddrUnix) { use rustix::io::{IoSlice, IoSliceMut}; use rustix::net::{recvmsg, sendmsg_noaddr, RecvFlags, SendFlags}; - use std::string::ToString; - - let tmpdir = tempfile::tempdir().unwrap(); - let path = tmpdir.path().join("scp_4804"); - - let connection_socket = socket( - AddressFamily::UNIX, - SocketType::SEQPACKET, - Protocol::default(), - ) - .unwrap(); - - let name = SocketAddrUnix::new(&path).unwrap(); - bind_unix(&connection_socket, &name).unwrap(); - listen(&connection_socket, 1).unwrap(); let server = { - let path = path.clone(); + let connection_socket = socket( + AddressFamily::UNIX, + SocketType::SEQPACKET, + Protocol::default(), + ) + .unwrap(); + bind_unix(&connection_socket, &addr).unwrap(); + listen(&connection_socket, 1).unwrap(); move || { let mut buffer = vec![0; BUFFER_SIZE]; @@ -199,13 +190,10 @@ fn test_unix_msg() { ) .unwrap(); } - - unlinkat(cwd(), path, AtFlags::empty()).unwrap(); } }; let client = move || { - let addr = SocketAddrUnix::new(path).unwrap(); let mut buffer = vec![0; BUFFER_SIZE]; let runs: &[(&[&str], i32)] = &[ (&["1", "2"], 3), @@ -288,6 +276,30 @@ fn test_unix_msg() { server.join().unwrap(); } +#[cfg(not(any(target_os = "redox", target_os = "wasi")))] +#[test] +fn test_unix_msg() { + let tmpdir = tempfile::tempdir().unwrap(); + let path = tmpdir.path().join("scp_4804"); + + let name = SocketAddrUnix::new(&path).unwrap(); + do_test_unix_msg(name); + + unlinkat(cwd(), path, AtFlags::empty()).unwrap(); +} + +#[cfg(not(any(target_os = "redox", target_os = "wasi")))] +#[test] +fn test_abstract_unix_msg() { + use std::os::unix::ffi::OsStrExt; + + let tmpdir = tempfile::tempdir().unwrap(); + let path = tmpdir.path().join("scp_4804"); + + let name = SocketAddrUnix::new_abstract_name(path.as_os_str().as_bytes()).unwrap(); + do_test_unix_msg(name); +} + #[cfg(not(any(target_os = "redox", target_os = "wasi")))] #[test] fn test_unix_msg_with_scm_rights() { From d7f22fd07c058f52ddacf087dcb749cb9c8de1e9 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 16:13:51 +0200 Subject: [PATCH 3/8] Fix recvmsg() on abstract unix sockets This case was just not implemented at all. Abstract unix sockets are not null-terminated and are indicated by sun_path beginning with a null byte. Fixes: https://github.com/bytecodealliance/rustix/issues/660 Signed-off-by: Uli Schlachter --- src/backend/linux_raw/net/read_sockaddr.rs | 36 ++++++++++++++-------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/src/backend/linux_raw/net/read_sockaddr.rs b/src/backend/linux_raw/net/read_sockaddr.rs index b9bc09b96..fdab8d108 100644 --- a/src/backend/linux_raw/net/read_sockaddr.rs +++ b/src/backend/linux_raw/net/read_sockaddr.rs @@ -155,19 +155,31 @@ pub(crate) unsafe fn read_sockaddr_os(storage: *const c::sockaddr, len: usize) - SocketAddrAny::Unix(SocketAddrUnix::new(&[][..]).unwrap()) } else { let decode = *storage.cast::(); - assert_eq!( - decode.sun_path[len - 1 - offsetof_sun_path], - b'\0' as c::c_char - ); - SocketAddrAny::Unix( - SocketAddrUnix::new( - decode.sun_path[..len - 1 - offsetof_sun_path] - .iter() - .map(|c| *c as u8) - .collect::>(), + if decode.sun_path[0] == 0 { + SocketAddrAny::Unix( + SocketAddrUnix::new_abstract_name( + &decode.sun_path[1..len - offsetof_sun_path] + .iter() + .map(|c| *c as u8) + .collect::>(), + ) + .unwrap(), + ) + } else { + assert_eq!( + decode.sun_path[len - 1 - offsetof_sun_path], + b'\0' as c::c_char + ); + SocketAddrAny::Unix( + SocketAddrUnix::new( + decode.sun_path[..len - 1 - offsetof_sun_path] + .iter() + .map(|c| *c as u8) + .collect::>(), + ) + .unwrap(), ) - .unwrap(), - ) + } } } other => unimplemented!("{:?}", other), From e911574c829d3f6cba718df6ca0a8b899c850a18 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 16:17:07 +0200 Subject: [PATCH 4/8] Strengthen recvmsg() test The fix in the previous commit added some parsing for abstract unix sockets. I wasn't sure whether I got all the indicies right, so this commit extends the existing test to check that the result from recvmsg() contains the expected socket address. Signed-off-by: Uli Schlachter --- tests/net/unix.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/net/unix.rs b/tests/net/unix.rs index 97b487faf..ab3539bbd 100644 --- a/tests/net/unix.rs +++ b/tests/net/unix.rs @@ -144,7 +144,7 @@ fn test_unix() { #[cfg(not(any(target_os = "redox", target_os = "wasi")))] fn do_test_unix_msg(addr: SocketAddrUnix) { use rustix::io::{IoSlice, IoSliceMut}; - use rustix::net::{recvmsg, sendmsg_noaddr, RecvFlags, SendFlags}; + use rustix::net::{recvmsg, sendmsg_noaddr, RecvFlags, SendFlags, SocketAddrAny}; let server = { let connection_socket = socket( @@ -228,18 +228,19 @@ fn do_test_unix_msg(addr: SocketAddrUnix) { ) .unwrap(); - let nread = recvmsg( + let result = recvmsg( &data_socket, &mut [IoSliceMut::new(&mut buffer)], &mut Default::default(), RecvFlags::empty(), ) - .unwrap() - .bytes; + .unwrap(); + let nread = result.bytes; assert_eq!( i32::from_str(&String::from_utf8_lossy(&buffer[..nread])).unwrap(), *sum ); + assert_eq!(Some(SocketAddrAny::Unix(addr.clone())), result.address); } let data_socket = socket( From 8584d8a8e52443eb60ce39a00c86b43082ee9861 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 16:28:44 +0200 Subject: [PATCH 5/8] Fix recvmsg() on abstract unix sockets on libc backend The previous commit for this only fixed the linux_raw backend. This commit applies the same fix to the libc backend. Fixes: https://github.com/bytecodealliance/rustix/issues/660 Signed-off-by: Uli Schlachter --- src/backend/libc/net/read_sockaddr.rs | 39 ++++++++++++++++++--------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/src/backend/libc/net/read_sockaddr.rs b/src/backend/libc/net/read_sockaddr.rs index 575102c27..37e48df41 100644 --- a/src/backend/libc/net/read_sockaddr.rs +++ b/src/backend/libc/net/read_sockaddr.rs @@ -187,22 +187,35 @@ unsafe fn inner_read_sockaddr_os( SocketAddrAny::Unix(SocketAddrUnix::new(&[][..]).unwrap()) } else { let decode = *storage.cast::(); - assert_eq!( - decode.sun_path[len - 1 - offsetof_sun_path], - b'\0' as c::c_char - ); - let path_bytes = &decode.sun_path[..len - 1 - offsetof_sun_path]; + if decode.sun_path[0] == 0 { + let address_bytes = &decode.sun_path[1..len - offsetof_sun_path]; + SocketAddrAny::Unix( + SocketAddrUnix::new_abstract_name( + &address_bytes.iter().map(|c| *c as u8).collect::>(), + ) + .unwrap(), + ) + } else { + assert_eq!( + decode.sun_path[len - 1 - offsetof_sun_path], + b'\0' as c::c_char + ); + let path_bytes = &decode.sun_path[..len - 1 - offsetof_sun_path]; - // FreeBSD sometimes sets the length to longer than the length - // of the NUL-terminated string. Find the NUL and truncate the - // string accordingly. - #[cfg(target_os = "freebsd")] - let path_bytes = &path_bytes[..path_bytes.iter().position(|b| *b == 0).unwrap()]; + // FreeBSD sometimes sets the length to longer than the length + // of the NUL-terminated string. Find the NUL and truncate the + // string accordingly. + #[cfg(target_os = "freebsd")] + let path_bytes = + &path_bytes[..path_bytes.iter().position(|b| *b == 0).unwrap()]; - SocketAddrAny::Unix( - SocketAddrUnix::new(path_bytes.iter().map(|c| *c as u8).collect::>()) + SocketAddrAny::Unix( + SocketAddrUnix::new( + path_bytes.iter().map(|c| *c as u8).collect::>(), + ) .unwrap(), - ) + ) + } } } other => unimplemented!("{:?}", other), From 97ca6a0141b48046ebe8cc297d84a6fb798e2a15 Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 16:36:31 +0200 Subject: [PATCH 6/8] Restrict test_abstract_unix_msg() test to Linux Abstract unix sockets are a feature of the Linux kernel that is not available elsewhere. Signed-off-by: Uli Schlachter --- tests/net/unix.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/net/unix.rs b/tests/net/unix.rs index ab3539bbd..8279ad67f 100644 --- a/tests/net/unix.rs +++ b/tests/net/unix.rs @@ -289,7 +289,7 @@ fn test_unix_msg() { unlinkat(cwd(), path, AtFlags::empty()).unwrap(); } -#[cfg(not(any(target_os = "redox", target_os = "wasi")))] +#[cfg(any(target_os = "android", target_os = "linux"))] #[test] fn test_abstract_unix_msg() { use std::os::unix::ffi::OsStrExt; From 944bdef6889cb2c78d31bbe238b9da39edfae00b Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 17:04:36 +0200 Subject: [PATCH 7/8] Fix compilation on non-Linux Abstract unix sockets are a Linux-only feature. Signed-off-by: Uli Schlachter --- src/backend/libc/net/read_sockaddr.rs | 47 ++++++++++++++++++--------- 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/src/backend/libc/net/read_sockaddr.rs b/src/backend/libc/net/read_sockaddr.rs index 37e48df41..4ce4f0ab2 100644 --- a/src/backend/libc/net/read_sockaddr.rs +++ b/src/backend/libc/net/read_sockaddr.rs @@ -186,16 +186,34 @@ unsafe fn inner_read_sockaddr_os( if len == offsetof_sun_path { SocketAddrAny::Unix(SocketAddrUnix::new(&[][..]).unwrap()) } else { - let decode = *storage.cast::(); - if decode.sun_path[0] == 0 { - let address_bytes = &decode.sun_path[1..len - offsetof_sun_path]; - SocketAddrAny::Unix( - SocketAddrUnix::new_abstract_name( - &address_bytes.iter().map(|c| *c as u8).collect::>(), + #[cfg(not(any(target_os = "android", target_os = "linux")))] + fn try_decode_abstract_socket( + _sockaddr: &c::sockaddr_un, + _len: usize, + ) -> Option { + None + } + #[cfg(any(target_os = "android", target_os = "linux"))] + fn try_decode_abstract_socket( + decode: &c::sockaddr_un, + len: usize, + ) -> Option { + if decode.sun_path[0] != 0 { + None + } else { + let offsetof_sun_path = super::addr::offsetof_sun_path(); + let address_bytes = &decode.sun_path[1..len - offsetof_sun_path]; + Some( + SocketAddrUnix::new_abstract_name( + &address_bytes.iter().map(|c| *c as u8).collect::>(), + ) + .unwrap(), ) - .unwrap(), - ) - } else { + } + } + + let decode = *storage.cast::(); + let result = try_decode_abstract_socket(&decode, len).unwrap_or_else(|| { assert_eq!( decode.sun_path[len - 1 - offsetof_sun_path], b'\0' as c::c_char @@ -209,13 +227,10 @@ unsafe fn inner_read_sockaddr_os( let path_bytes = &path_bytes[..path_bytes.iter().position(|b| *b == 0).unwrap()]; - SocketAddrAny::Unix( - SocketAddrUnix::new( - path_bytes.iter().map(|c| *c as u8).collect::>(), - ) - .unwrap(), - ) - } + SocketAddrUnix::new(path_bytes.iter().map(|c| *c as u8).collect::>()) + .unwrap() + }); + SocketAddrAny::Unix(result) } } other => unimplemented!("{:?}", other), From 76adcfd9d5ba96f8cccd301653420de1b0cbe95a Mon Sep 17 00:00:00 2001 From: Uli Schlachter Date: Sat, 6 May 2023 18:29:11 +0200 Subject: [PATCH 8/8] Skip the test extension on FreeBSD Commit "Strengthen recvmsg() test" added a new assertion to this test. For unknown reasons, this test failed in Currus CI on x86_64-unknown-freebsd-13 as follows: ---- unix::test_unix_msg stdout ---- thread 'client' panicked at 'assertion failed: `(left == right)` left: `Some("/tmp/.tmpy5Fj4e/scp_4804")`, right: `Some("(unnamed)")`', tests/net/unix.rs:243:13 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace thread 'unix::test_unix_msg' panicked at 'called `Result::unwrap()` on an `Err` value: Any { .. }', tests/net/unix.rs:276:19 The text "(unnamed)" comes from the Debug impl of SocketAddrUnix. This text is generated when SocketAddrUnix::path() returns None. I do not know why this happens. I am just trying to get CI not to complain. A random guess would be that recvmsg() does not return the endpoint for connected sockets. This is because the "recvmsg" man page for FreeBSD says: > Here msg_name and msg_namelen specify the source address if the socket > is unconnected; Signed-off-by: Uli Schlachter --- tests/net/unix.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tests/net/unix.rs b/tests/net/unix.rs index 8279ad67f..e63bfabc7 100644 --- a/tests/net/unix.rs +++ b/tests/net/unix.rs @@ -144,7 +144,7 @@ fn test_unix() { #[cfg(not(any(target_os = "redox", target_os = "wasi")))] fn do_test_unix_msg(addr: SocketAddrUnix) { use rustix::io::{IoSlice, IoSliceMut}; - use rustix::net::{recvmsg, sendmsg_noaddr, RecvFlags, SendFlags, SocketAddrAny}; + use rustix::net::{recvmsg, sendmsg_noaddr, RecvFlags, SendFlags}; let server = { let connection_socket = socket( @@ -240,7 +240,13 @@ fn do_test_unix_msg(addr: SocketAddrUnix) { i32::from_str(&String::from_utf8_lossy(&buffer[..nread])).unwrap(), *sum ); - assert_eq!(Some(SocketAddrAny::Unix(addr.clone())), result.address); + // Don't ask me why, but this was seen to fail on FreeBSD. SocketAddrUnix::path() + // returned None for some reason. + #[cfg(not(target_os = "freebsd"))] + assert_eq!( + Some(rustix::net::SocketAddrAny::Unix(addr.clone())), + result.address + ); } let data_socket = socket(