Skip to content

Commit

Permalink
Fix decoding of abstract unix sockets (bytecodealliance#661)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>

* 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
bytecodealliance#660.

Signed-off-by: Uli Schlachter <[email protected]>

* 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: bytecodealliance#660
Signed-off-by: Uli Schlachter <[email protected]>

* 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 <[email protected]>

* 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: bytecodealliance#660
Signed-off-by: Uli Schlachter <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

---------

Signed-off-by: Uli Schlachter <[email protected]>
  • Loading branch information
@psychon
psychon authored May 8, 2023
1 parent 707742b commit 7ff9129
Show file tree
Hide file tree
Showing 3 changed files with 121 additions and 95 deletions.
58 changes: 43 additions & 15 deletions src/backend/libc/net/read_sockaddr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,23 +186,51 @@ unsafe fn inner_read_sockaddr_os(
if len == offsetof_sun_path {
SocketAddrAny::Unix(SocketAddrUnix::new(&[][..]).unwrap())
} else {
#[cfg(not(any(target_os = "android", target_os = "linux")))]
fn try_decode_abstract_socket(
_sockaddr: &c::sockaddr_un,
_len: usize,
) -> Option<SocketAddrUnix> {
None
}
#[cfg(any(target_os = "android", target_os = "linux"))]
fn try_decode_abstract_socket(
decode: &c::sockaddr_un,
len: usize,
) -> Option<SocketAddrUnix> {
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::<Vec<u8>>(),
)
.unwrap(),
)
}
}

let decode = *storage.cast::<c::sockaddr_un>();
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()];

SocketAddrAny::Unix(
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
);
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()];

SocketAddrUnix::new(path_bytes.iter().map(|c| *c as u8).collect::<Vec<u8>>())
.unwrap(),
)
.unwrap()
});
SocketAddrAny::Unix(result)
}
}
other => unimplemented!("{:?}", other),
Expand Down
36 changes: 24 additions & 12 deletions src/backend/linux_raw/net/read_sockaddr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::<c::sockaddr_un>();
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::<Vec<u8>>(),
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::<Vec<u8>>(),
)
.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::<Vec<u8>>(),
)
.unwrap(),
)
.unwrap(),
)
}
}
}
other => unimplemented!("{:?}", other),
Expand Down
122 changes: 54 additions & 68 deletions tests/net/unix.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,39 +142,21 @@ 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 ready = Arc::new((Mutex::new(false), Condvar::new()));

let server = {
let ready = ready.clone();
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 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();
Expand Down Expand Up @@ -208,21 +190,10 @@ fn test_unix_msg() {
)
.unwrap();
}

unlinkat(cwd(), path, AtFlags::empty()).unwrap();
}
};

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)] = &[
(&["1", "2"], 3),
Expand Down Expand Up @@ -257,18 +228,25 @@ fn test_unix_msg() {
)
.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
);
// 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(
Expand Down Expand Up @@ -305,6 +283,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(any(target_os = "android", target_os = "linux"))]
#[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() {
Expand All @@ -318,31 +320,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))];
Expand Down Expand Up @@ -403,14 +397,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];
Expand Down

0 comments on commit 7ff9129

Please sign in to comment.