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.

Sign up for GitHub

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

Jump to bottom

Fix decoding of abstract unix sockets #661

Merged
merged 8 commits into from
May 8, 2023
Merged

Fix decoding of abstract unix sockets #661

merged 8 commits into from
May 8, 2023

Conversation

psychon
Copy link
Contributor

@psychon psychon commented May 6, 2023

This is my attempt at fixing #660.

I am not sure what will happen on the libc backend, but I expect things to still be broken there. Enabling the use-libc feature makes things not compile for me:

error[E0432]: unresolved import `libc_errno`
 --> src/backend/libc/io/errno.rs:7:5
  |
7 | use libc_errno::errno;
  |     ^^^^^^^^^^ use of undeclared crate or module `libc_errno`

psychon added 7 commits May 6, 2023 15:58
@psychon
test_unix_msg: Remove thread synchronisation
b5e9770 
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]>
@psychon
Add test for abstract unix sockets
93f94e0 
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]>
@psychon
Fix recvmsg() on abstract unix sockets
d7f22fd 
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]>
@psychon
Strengthen recvmsg() test
e911574 
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]>
@psychon
Fix recvmsg() on abstract unix sockets on libc backend
8584d8a 
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]>
@psychon
Restrict test_abstract_unix_msg() test to Linux
97ca6a0 
Abstract unix sockets are a feature of the Linux kernel that is not
available elsewhere.

Signed-off-by: Uli Schlachter <[email protected]>
@psychon
Fix compilation on non-Linux
944bdef 
Abstract unix sockets are a Linux-only feature.

Signed-off-by: Uli Schlachter <[email protected]>
sunfishcode
sunfishcode reviewed May 6, 2023
View reviewed changes
Copy link
Member

@sunfishcode sunfishcode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix! Overall this looks good.

tests/net/unix.rs Show resolved Hide resolved
@psychon
Skip the test extension on FreeBSD
76adcfd 
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]>
@psychon psychon force-pushed the decode-abstract-unix-socket branch from fcd689c to 76adcfd Compare May 6, 2023 16:29
@sunfishcode
Copy link
Member

Thanks!

@sunfishcode sunfishcode merged commit 43025d8 into bytecodealliance:main May 8, 2023
@psychon psychon deleted the decode-abstract-unix-socket branch May 8, 2023 14:26
cgwalters pushed a commit to cgwalters/rustix that referenced this pull request May 23, 2023
@psychon
Fix decoding of abstract unix sockets (bytecodealliance#661)
7ff9129 
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sunfishcode sunfishcode sunfishcode left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@psychon @sunfishcode