Skip to content
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

SockaddrStorage broken for variable-length addresses #1866

Closed
stevenengler opened this issue Nov 14, 2022 · 2 comments · Fixed by #1871
Closed

SockaddrStorage broken for variable-length addresses #1866

stevenengler opened this issue Nov 14, 2022 · 2 comments · Fixed by #1871

Comments

@stevenengler
Copy link
Contributor

stevenengler commented Nov 14, 2022

The SockaddrStorage type "forgets" the address length, which causes incorrect and unexpected behaviour. I think this is a more fundamental issue with how nix represents socket addresses.

Example:

use std::os::unix::io::AsRawFd;
use std::os::unix::net::UnixListener;

use nix::sys::socket::{connect, getsockname, socket};
use nix::sys::socket::{AddressFamily, SockFlag, SockType, SockaddrStorage};

let listener = UnixListener::bind("/tmp/example.sock").unwrap();
let listener_fd = listener.as_raw_fd();
let listener_addr: SockaddrStorage = getsockname(listener_fd).unwrap();

let peer_fd = socket(AddressFamily::Unix, SockType::Stream, SockFlag::empty(), None).unwrap();

// this panics
connect(peer_fd, &listener_addr).unwrap();

If we look at the strace, we can see that nix is proving the wrong sockaddr length to connect(), which represents an incorrect address. This is equally important for abstract named and unnamed unix sockets.

socket(AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0) = 3
bind(3, {sa_family=AF_UNIX, sun_path="/tmp/example.sock"}, 20) = 0
listen(3, 4294967295)                   = 0
getsockname(3, {sa_family=AF_UNIX, sun_path="/tmp/example.sock"}, [128->20]) = 0
write(1, "SockaddrStorage { ss: sockaddr_s"..., 91SockaddrStorage { ss: sockaddr_storage { ss_family: 1, __ss_align: 8299682679707492728 } }
) = 91
socket(AF_UNIX, SOCK_STREAM, 0)         = 4
connect(4, {sa_family=AF_UNIX, sun_path="/tmp/example.sock"}, 128) = -1 EINVAL (Invalid argument)

When you're writing code that has to support many different socket address types, this means that SockaddrStorage is unusable. We're currently sticking with the deprecated nix::sys::socket::SockAddr for now as it doesn't have this issue.

@asomers
Copy link
Member

asomers commented Nov 19, 2022

I see. This isn't a problem on BSDs, where sockaddr_storage does know its own size. That's probably why I never noticed this before, because that's where I do most of my development. UnixAddr already has a workaround for Linux-like OSes, and a similar workaround is probably needed for SockaddrStorage. As far as you know, is the problem limited to Unix-domain socket addresses?

@asomers asomers added this to the 0.26.0 milestone Nov 19, 2022
asomers added a commit to asomers/nix that referenced this issue Nov 19, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes nix-rust#1866
@stevenengler
Copy link
Contributor Author

I see. This isn't a problem on BSDs, where sockaddr_storage does know its own size. That's probably why I never noticed this before, because that's where I do most of my development. UnixAddr already has a workaround for Linux-like OSes, and a similar workaround is probably needed for SockaddrStorage.

Unfortunately I have no experience with any BSDs, so I can see how it's a pain trying to abstract around both socket address implementations.

As far as you know, is the problem limited to Unix-domain socket addresses?

I think so, I don't know of any other variable-length socket addresses on Linux, but there are a lot of protocol families I've never used.

bors bot added a commit that referenced this issue Nov 21, 2022
1871: Fix using SockaddrStorage to store Unix domain addresses on Linux r=rtzoeller a=asomers

Since it has variable length, the user of a sockaddr_un must keep track of its true length.  On the BSDs, this is handled by the builtin sun_len field.  But on Linux-like operating systems it isn't.  Fix this bug by explicitly tracking it for SockaddrStorage just like we already do for UnixAddr.

Fixes #1866

Co-authored-by: Alan Somers <asomers@gmail.com>
@bors bors bot closed this as completed in fa62534 Nov 21, 2022
rtzoeller pushed a commit to rtzoeller/nix that referenced this issue Nov 29, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes nix-rust#1866
rtzoeller pushed a commit to rtzoeller/nix that referenced this issue Nov 29, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes nix-rust#1866
rtzoeller pushed a commit that referenced this issue Nov 29, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes #1866
rtzoeller pushed a commit to rtzoeller/nix that referenced this issue Nov 29, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes nix-rust#1866
rtzoeller pushed a commit to rtzoeller/nix that referenced this issue Nov 29, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes nix-rust#1866
rtzoeller pushed a commit to rtzoeller/nix that referenced this issue Nov 29, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes nix-rust#1866
rtzoeller pushed a commit to rtzoeller/nix that referenced this issue Dec 2, 2022
Since it has variable length, the user of a sockaddr_un must keep track
of its true length.  On the BSDs, this is handled by the builtin sun_len
field.  But on Linux-like operating systems it isn't.  Fix this bug by
explicitly tracking it for SockaddrStorage just like we already do for
UnixAddr.

Fixes nix-rust#1866
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants