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

Add support for Unix domain sockets #64

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

astei
Copy link

@astei astei commented Jan 22, 2021

Motivation:

The io_uring transport does not currently support Unix domain sockets. The epoll transport does, though. There should be parity between the two.

Modifications:

  • Add IOUringDomainSocketChannel, IOUringDomainSocketChannelConfig, and IOUringServerDomainSocketChannel classes.
  • Add tests for io_uring domain sockets, including sanity checks with the known working epoll transport.

I've marked thie PR as a draft for a few reasons:

  • Transferring file descriptors isn't implemented yet. Appears to be a Linux kernel restriction for security reasons. A future version of Linux may allow for this.
  • IOUringDomainSocketShutdownOutputByPeerTest fails for some reason.
  • IOUringSocketConnectionAttemptTest fails with Invalid argument for a reason that is not clear to me. Fixed, reason was IPv6-mapped IPv4 addresses

Result:

The io_uring transport supports Unix domain sockets.

@normanmaurer
Copy link
Member

I will have a look soon...

@normanmaurer
Copy link
Member

@astei can you rebase as well ?

@astei astei force-pushed the feature/domain-sockets branch from 7c3bb09 to 0a5f04a Compare January 27, 2021 16:08
@astei
Copy link
Author

astei commented Jan 27, 2021

@normanmaurer rebased

@normanmaurer
Copy link
Member

@astei seems like all pass ?

@astei
Copy link
Author

astei commented Feb 2, 2021

@astei seems like all pass ?

They did not pass on my system (Fedora 33 Workstation, Linux 5.10.x kernel). It’s very strange the CI thinks it’s good. I don’t have a ready explanation for why.

@astei
Copy link
Author

astei commented Feb 2, 2021

@normanmaurer when I am back home I will rerun the tests again.

@normanmaurer
Copy link
Member

normanmaurer commented Feb 2, 2021 via email

@astei
Copy link
Author

astei commented Feb 3, 2021

@normanmaurer I have re-run the tests, and the same two tests are failing: https://gist.github.com/astei/e3fc19a705c56a086b83ef8b042ef070

@normanmaurer
Copy link
Member

normanmaurer commented Feb 3, 2021 via email

@astei
Copy link
Author

astei commented Feb 3, 2021

Do the same tests also fail when you try to run the epoll tests on netty ?

@normanmaurer all the epoll transport tests pass, which is very strange.

@LifeIsStrange
Copy link

Unrelated to this PR:
Openjdk 16 release next month and support natively unix domain socket
https://openjdk.java.net/jeps/380
https://inside.java/2021/02/03/jep380-unix-domain-sockets-channels/

@normanmaurer
Copy link
Member

@astei the connect error can be fixed by:

diff --git a/src/main/java/io/netty/incubator/channel/uring/AbstractIOUringChannel.java b/src/main/java/io/netty/incubator/channel/uring/AbstractIOUringChannel.java
index ebd5228..876cd8a 100644
--- a/src/main/java/io/netty/incubator/channel/uring/AbstractIOUringChannel.java
+++ b/src/main/java/io/netty/incubator/channel/uring/AbstractIOUringChannel.java
@@ -676,7 +676,7 @@ abstract class AbstractIOUringChannel extends AbstractChannel implements UnixCha
                 remoteAddressMemory = Buffer.allocateDirectWithNativeOrder(Native.SIZEOF_SOCKADDR_STORAGE);
                 long remoteAddressMemoryAddress = Buffer.memoryAddress(remoteAddressMemory);

-                int addrLen = Sockaddr.write(remoteAddressMemoryAddress, remoteAddress);
+                int addrLen = Sockaddr.write(socket.isIpv6(), remoteAddressMemoryAddress, remoteAddress);

                 final IOUringSubmissionQueue ioUringSubmissionQueue = submissionQueue();
                 ioUringSubmissionQueue.addConnect(socket.intValue(), remoteAddressMemoryAddress, addrLen, (short) 0);
diff --git a/src/main/java/io/netty/incubator/channel/uring/MsgHdrMemory.java b/src/main/java/io/netty/incubator/channel/uring/MsgHdrMemory.java
index 8fb3fa5..dacdb25 100644
--- a/src/main/java/io/netty/incubator/channel/uring/MsgHdrMemory.java
+++ b/src/main/java/io/netty/incubator/channel/uring/MsgHdrMemory.java
@@ -40,7 +40,7 @@ final class MsgHdrMemory {
             addressLength = socket.isIpv6() ? Native.SIZEOF_SOCKADDR_IN6 : Native.SIZEOF_SOCKADDR_IN;
             PlatformDependent.setMemory(sockAddress, Native.SIZEOF_SOCKADDR_STORAGE, (byte) 0);
         } else {
-            addressLength = Sockaddr.write(sockAddress, address);
+            addressLength = Sockaddr.write(socket.isIpv6(), sockAddress, address);
         }
         Iov.write(iovAddress, bufferAddress, length);
         MsgHdr.write(memory, sockAddress, addressLength, iovAddress, 1);
diff --git a/src/main/java/io/netty/incubator/channel/uring/Sockaddr.java b/src/main/java/io/netty/incubator/channel/uring/Sockaddr.java
index e0782a6..734389f 100644
--- a/src/main/java/io/netty/incubator/channel/uring/Sockaddr.java
+++ b/src/main/java/io/netty/incubator/channel/uring/Sockaddr.java
@@ -37,12 +37,11 @@ final class Sockaddr {

     private Sockaddr() { }

-    static int write(long memory, SocketAddress address) {
+    static int write(boolean ipv6, long memory, SocketAddress address) {
         if (address instanceof DomainSocketAddress) {
             return Sockaddr.writeDomain(memory, ((DomainSocketAddress) address).path());
         } else if (address instanceof InetSocketAddress) {
             InetSocketAddress inetSocketAddress = (InetSocketAddress) address;
-            boolean ipv6 = ((InetSocketAddress) address).getAddress() instanceof Inet6Address;
             if (ipv6) {
                 return Sockaddr.writeIPv6(memory, inetSocketAddress.getAddress(), inetSocketAddress.getPort());
             } else {

This is due the fact of ipv6 mapped ipv4 addresses

@normanmaurer
Copy link
Member

If you like I can also try to check the other error. But most likely not before next week. Let me know @astei

@victorstewart
Copy link

victorstewart commented Feb 5, 2021

saw this on jen's twitter. fyi you won't be able to transfer file descriptors because sendmsg cmsghdrs are only allowed on inet(6) + stream... and coming in 5.12 inet(6) + dgram.

there's a whole bunch of security issues that were solved with a blunt hammer.

@astei
Copy link
Author

astei commented Feb 5, 2021

Well, that fixes 2/3 issues. I'm not a huge fan of adding back socket.isIpv6() since it looks a little ugly (and quite out of place with a Unix domain socket).

@normanmaurer
Copy link
Member

@astei sorry for the slow turn around ... I will try to push this over the finish line next week.

@normanmaurer normanmaurer modified the milestones: 0.0.5.Final, 0.0.6.Final Mar 27, 2021
@Kamillaova
Copy link

bump

@franz1981
Copy link
Contributor

@astei do you think to rebase? If not I can pick this for the next weeks

@astei
Copy link
Author

astei commented Apr 20, 2023

@franz1981 I can try to get this moving again at some point, but it's not been high on my list of priorities. Since submitting this PR I've gone through a few job changes and my current job is pretty far removed from Netty.

@Kamillaova
Copy link

bump

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants