unix: fail with ENAMETOOLONG in uv_pipe_*

addaleax · addaleax · commit 3da120be748b · 2017-04-28T01:50:26.000+02:00
Fail with ENAMETOOLONG when the name of a Unix socket exceeds `sizeof(saddr.sun_path)`. Previously the path was just truncated, which could result in nasty bugs, and even though that behaviour has been always been around, it’s hard to imagine a situation in which ending up with an incorrect path is better than not creating a socket at all. Ref: nodejs/node#12601 Ref: nodejs/node#12708
diff --git a/docs/src/pipe.rst b/docs/src/pipe.rst
@@ -49,16 +49,16 @@ API
     Bind the pipe to a file path (Unix) or a name (Windows).
 
     .. note::
-        Paths on Unix get truncated to ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
-        92 and 108 bytes.
+        If a path on Unix exceeds ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
+        92 and 108 bytes, ``uv_pipe_bind`` will fail with ``UV_ENAMETOOLONG``.
 
 .. c:function:: void uv_pipe_connect(uv_connect_t* req, uv_pipe_t* handle, const char* name, uv_connect_cb cb)
 
     Connect to the Unix domain socket or the named pipe.
 
     .. note::
-        Paths on Unix get truncated to ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
-        92 and 108 bytes.
+        If a path on Unix exceeds ``sizeof(sockaddr_un.sun_path)`` bytes, typically between
+        92 and 108 bytes, ``uv_pipe_bind`` will fail with ``UV_ENAMETOOLONG``.
 
 .. c:function:: int uv_pipe_getsockname(const uv_pipe_t* handle, char* buffer, size_t* size)
 
diff --git a/src/unix/pipe.c b/src/unix/pipe.c
@@ -45,9 +45,14 @@ int uv_pipe_bind(uv_pipe_t* handle, const char* name) {
   const char* pipe_fname;
   int sockfd;
   int err;
+  size_t name_len;
 
   pipe_fname = NULL;
   sockfd = -1;
+  name_len = strlen(name) + 1;
+
+  if (name_len > sizeof(saddr.sun_path))
+    return -ENAMETOOLONG;
 
   /* Already bound? */
   if (uv__stream_fd(handle) >= 0)
@@ -67,8 +72,7 @@ int uv_pipe_bind(uv_pipe_t* handle, const char* name) {
   sockfd = err;
 
   memset(&saddr, 0, sizeof saddr);
-  strncpy(saddr.sun_path, pipe_fname, sizeof(saddr.sun_path) - 1);
-  saddr.sun_path[sizeof(saddr.sun_path) - 1] = '\0';
+  memcpy(saddr.sun_path, pipe_fname, name_len);
   saddr.sun_family = AF_UNIX;
 
   if (bind(sockfd, (struct sockaddr*)&saddr, sizeof saddr)) {
@@ -160,6 +164,14 @@ void uv_pipe_connect(uv_connect_t* req,
   int new_sock;
   int err;
   int r;
+  size_t name_len;
+
+  name_len = strlen(name) + 1;
+  
+  if (name_len > sizeof(saddr.sun_path)) {
+    err = -ENAMETOOLONG;
+    goto out;
+  }
 
   new_sock = (uv__stream_fd(handle) == -1);
 
@@ -171,8 +183,7 @@ void uv_pipe_connect(uv_connect_t* req,
   }
 
   memset(&saddr, 0, sizeof saddr);
-  strncpy(saddr.sun_path, name, sizeof(saddr.sun_path) - 1);
-  saddr.sun_path[sizeof(saddr.sun_path) - 1] = '\0';
+  memcpy(saddr.sun_path, name, name_len);
   saddr.sun_family = AF_UNIX;
 
   do {
diff --git a/test/test-list.h b/test/test-list.h
@@ -135,10 +135,12 @@ TEST_DECLARE   (udp_try_send)
 TEST_DECLARE   (pipe_bind_error_addrinuse)
 TEST_DECLARE   (pipe_bind_error_addrnotavail)
 TEST_DECLARE   (pipe_bind_error_inval)
+TEST_DECLARE   (pipe_bind_error_long_path)
 TEST_DECLARE   (pipe_connect_multiple)
 TEST_DECLARE   (pipe_listen_without_bind)
 TEST_DECLARE   (pipe_connect_bad_name)
 TEST_DECLARE   (pipe_connect_to_file)
+TEST_DECLARE   (pipe_connect_to_long_path)
 TEST_DECLARE   (pipe_connect_on_prepare)
 TEST_DECLARE   (pipe_getsockname)
 TEST_DECLARE   (pipe_getsockname_abstract)
@@ -398,6 +400,7 @@ TASK_LIST_START
 
   TEST_ENTRY  (pipe_connect_bad_name)
   TEST_ENTRY  (pipe_connect_to_file)
+  TEST_ENTRY  (pipe_connect_to_long_path)
   TEST_ENTRY  (pipe_connect_on_prepare)
 
   TEST_ENTRY  (pipe_server_close)
@@ -523,6 +526,7 @@ TASK_LIST_START
   TEST_ENTRY  (pipe_bind_error_addrinuse)
   TEST_ENTRY  (pipe_bind_error_addrnotavail)
   TEST_ENTRY  (pipe_bind_error_inval)
+  TEST_ENTRY  (pipe_bind_error_long_path)
   TEST_ENTRY  (pipe_connect_multiple)
   TEST_ENTRY  (pipe_listen_without_bind)
   TEST_ENTRY  (pipe_getsockname)
diff --git a/test/test-pipe-bind-error.c b/test/test-pipe-bind-error.c
@@ -23,6 +23,7 @@
 #include "task.h"
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 
 
 #ifdef _WIN32
@@ -134,3 +135,27 @@ TEST_IMPL(pipe_listen_without_bind) {
   MAKE_VALGRIND_HAPPY();
   return 0;
 }
+
+
+TEST_IMPL(pipe_bind_error_long_path) {
+  char path[256];
+  uv_pipe_t server;
+  int r;
+
+  memset(path, '.', 255);
+  path[255] = '\0';
+
+  r = uv_pipe_init(uv_default_loop(), &server, 0);
+  ASSERT(r == 0);
+  r = uv_pipe_bind(&server, path);
+  ASSERT(r == UV_ENAMETOOLONG);
+
+  uv_close((uv_handle_t*)&server, close_cb);
+
+  uv_run(uv_default_loop(), UV_RUN_DEFAULT);
+
+  ASSERT(close_cb_called == 1);
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}
diff --git a/test/test-pipe-connect-error.c b/test/test-pipe-connect-error.c
@@ -23,6 +23,7 @@
 #include "task.h"
 #include <stdio.h>
 #include <stdlib.h>
+#include <string.h>
 
 
 #ifdef _WIN32
@@ -56,6 +57,13 @@ static void connect_cb_file(uv_connect_t* connect_req, int status) {
 }
 
 
+static void connect_cb_long_path(uv_connect_t* connect_req, int status) {
+  ASSERT(status == UV_ENAMETOOLONG);
+  uv_close((uv_handle_t*)connect_req->handle, close_cb);
+  connect_cb_called++;
+}
+
+
 TEST_IMPL(pipe_connect_bad_name) {
   uv_pipe_t client;
   uv_connect_t req;
@@ -93,3 +101,26 @@ TEST_IMPL(pipe_connect_to_file) {
   MAKE_VALGRIND_HAPPY();
   return 0;
 }
+
+
+TEST_IMPL(pipe_connect_to_long_path) {
+  char path[256];
+  uv_pipe_t client;
+  uv_connect_t req;
+  int r;
+
+  memset(path, '.', 255);
+  path[255] = '\0';
+
+  r = uv_pipe_init(uv_default_loop(), &client, 0);
+  ASSERT(r == 0);
+  uv_pipe_connect(&req, &client, path, connect_cb_long_path);
+
+  uv_run(uv_default_loop(), UV_RUN_DEFAULT);
+
+  ASSERT(close_cb_called == 1);
+  ASSERT(connect_cb_called == 1);
+
+  MAKE_VALGRIND_HAPPY();
+  return 0;
+}
