From 050f0f798212a0d0d2803e9faa9fdecfbc6dee3a Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 28 Apr 2017 01:25:00 +0200 Subject: [PATCH] unix: fail with ENAMETOOLONG in `uv_pipe_*` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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: https://github.com/nodejs/node/pull/12601 Ref: https://github.com/nodejs/node/issues/12708 --- src/unix/pipe.c | 19 +++++++++++++++---- test/test-list.h | 4 ++++ test/test-pipe-bind-error.c | 25 +++++++++++++++++++++++++ test/test-pipe-connect-error.c | 31 +++++++++++++++++++++++++++++++ 4 files changed, 75 insertions(+), 4 deletions(-) diff --git a/src/unix/pipe.c b/src/unix/pipe.c index b73994cb8de..232c7743564 100644 --- 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 index 3a1e82a9191..66fb70ef285 100644 --- 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 index 38b57db6991..9ddc302bd22 100644 --- a/test/test-pipe-bind-error.c +++ b/test/test-pipe-bind-error.c @@ -23,6 +23,7 @@ #include "task.h" #include #include +#include #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 index ebb2a6ca826..1074569e4ee 100644 --- a/test/test-pipe-connect-error.c +++ b/test/test-pipe-connect-error.c @@ -23,6 +23,7 @@ #include "task.h" #include #include +#include #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; +}