Repository: netty/netty
Issue: Issues on UDS path binding

I think there are couple issues relate to the UDS path binding logic:

1. In the JNI `netty_unix_socket_bindDomainSocket` method, when the upper level passes in a [longer-than-limit UDS path](https://github.com/netty/netty/blob/d58d8a1df832a5185193368e589cb4a488709ffe/transport-native-unix-common/src/main/c/netty_unix_socket.c#L768).
a. the `unlink()` is called on a different [path](https://github.com/netty/netty/blob/d58d8a1df832a5185193368e589cb4a488709ffe/transport-native-unix-common/src/main/c/netty_unix_socket.c#L772) than the [one](https://github.com/netty/netty/blob/d58d8a1df832a5185193368e589cb4a488709ffe/transport-native-unix-common/src/main/c/netty_unix_socket.c#L776) the code later `bind` to.
b. Netty is [quietly truncating the path](https://github.com/netty/netty/blob/d58d8a1df832a5185193368e589cb4a488709ffe/transport-native-unix-common/src/main/c/netty_unix_socket.c#L770) if user provides a over the limit path. This can be an issue if we have a non-Netty client which has a different path handling logic.

2. According to the manual, "a UNIX domain socket can be bound to a *null-terminated* filesystem pathname", but Netty is not doing anything about this requirement. Although there are some [messes](https://man.archlinux.org/man/core/man-pages/unix.7.en#BUGS) on handling this requirement, we should not simply ignore it. This is also an issue if we have a non-Netty client.

I am proposing a possible fix for all these issues. We can just validate the `socketPath` passed in and let Java throw exceptions if: 1) the `socketPath` is longer than `sizeof(addr.sun_path)`, 2) or, the `socketPath` has the same length as `sizeof(addr.sun_path)` but the last byte is not '\0'. A similar thing also need to be done on the client side. This can ensure the UDS paths Netty can use have maximum compatibility with other UDS servers/clients.

I can send a PR if the change mentioned above is desirable. Thank you.


Issue comments:
Sounds good . Will provide more feedback on the PR

Pull request review comments:

Patch that resolved the issue:
diff --git a/transport-native-unix-common/src/main/c/netty_unix_socket.c b/transport-native-unix-common/src/main/c/netty_unix_socket.c
index 2524b412209..a6cd08f3086 100644
--- a/transport-native-unix-common/src/main/c/netty_unix_socket.c
+++ b/transport-native-unix-common/src/main/c/netty_unix_socket.c
@@ -764,12 +764,13 @@ static jint netty_unix_socket_bindDomainSocket(JNIEnv* env, jclass clazz, jint f
 
     jbyte* socket_path = (*env)->GetByteArrayElements(env, socketPath, 0);
     jint socket_path_len = (*env)->GetArrayLength(env, socketPath);
-    if (socket_path_len > sizeof(addr.sun_path)) {
-        socket_path_len = sizeof(addr.sun_path);
+
+    if (socket_path_len > sizeof(addr.sun_path) || (socket_path_len == sizeof(addr.sun_path) && socket_path[socket_path_len] != '\0')) {
+        return -ENAMETOOLONG;
     }
     memcpy(addr.sun_path, socket_path, socket_path_len);
 
-    if (unlink((const char*) socket_path) == -1 && errno != ENOENT) {
+    if (unlink((const char*) addr.sun_path) == -1 && errno != ENOENT) {
         return -errno;
     }
 
@@ -791,8 +792,9 @@ static jint netty_unix_socket_connectDomainSocket(JNIEnv* env, jclass clazz, jin
 
     jbyte* socket_path = (*env)->GetByteArrayElements(env, socketPath, 0);
     socket_path_len = (*env)->GetArrayLength(env, socketPath);
-    if (socket_path_len > sizeof(addr.sun_path)) {
-        socket_path_len = sizeof(addr.sun_path);
+
+    if (socket_path_len > sizeof(addr.sun_path) || (socket_path_len == sizeof(addr.sun_path) && socket_path[socket_path_len] != '\0')) {
+        return -ENAMETOOLONG;
     }
     memcpy(addr.sun_path, socket_path, socket_path_len);
 


Test patch:
diff --git a/transport-native-epoll/src/test/java/io/netty/channel/epoll/LinuxSocketTest.java b/transport-native-epoll/src/test/java/io/netty/channel/epoll/LinuxSocketTest.java
index ff85d3ac12c..9f16b2bfbfe 100644
--- a/transport-native-epoll/src/test/java/io/netty/channel/epoll/LinuxSocketTest.java
+++ b/transport-native-epoll/src/test/java/io/netty/channel/epoll/LinuxSocketTest.java
@@ -15,6 +15,13 @@
  */
 package io.netty.channel.epoll;
 
+import io.netty.channel.unix.DomainSocketAddress;
+import io.netty.channel.unix.Errors.NativeIoException;
+import io.netty.channel.unix.Socket;
+import java.nio.charset.Charset;
+import java.util.Random;
+import java.util.UUID;
+import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeAll;
 
 import java.io.IOException;
@@ -65,4 +72,28 @@ public void execute() throws Throwable {
             socket.close();
         }
     }
+
+    @Test
+    public void testUnixDomainSocketTooLongPathFails() throws IOException {
+        // Most systems has a limit for UDS path of 108, 255 is generally too long.
+        StringBuilder socketPath = new StringBuilder("/tmp/");
+        while (socketPath.length() < 255) {
+            socketPath.append(UUID.randomUUID());
+        }
+
+        final DomainSocketAddress domainSocketAddress = new DomainSocketAddress(
+            socketPath.toString());
+        final Socket socket = Socket.newSocketDomain();
+        try {
+            Exception exception = Assertions.assertThrows(NativeIoException.class, new Executable() {
+                @Override
+                public void execute() throws Throwable {
+                    socket.bind(domainSocketAddress);
+                }
+            });
+            Assertions.assertTrue(exception.getMessage().contains("too long"));
+        } finally {
+            socket.close();
+        }
+    }
 }