Fix decoding of abstract unix sockets #661
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
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]>
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]>
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]>
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]>
Abstract unix sockets are a feature of the Linux kernel that is not available elsewhere. Signed-off-by: Uli Schlachter <[email protected]>
Abstract unix sockets are a Linux-only feature. Signed-off-by: Uli Schlachter <[email protected]>
sunfishcode
reviewed
May 6, 2023
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]>
fcd689c
to
76adcfd
Compare
|
Thanks! |
cgwalters
pushed a commit
to cgwalters/rustix
that referenced
this pull request
May 23, 2023
* 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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-libcfeature makes things not compile for me: