From e4a68bb5cb1eff1df1358d348b33e973eb9962d9 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 29 Jun 2012 18:21:50 +0200 Subject: [PATCH 01/24] unix: move uv__connect() to tcp.c --- src/unix/internal.h | 2 - src/unix/stream.c | 59 ---------------------------- src/unix/tcp.c | 95 +++++++++++++++++++++++++++++++++++---------- 3 files changed, 75 insertions(+), 81 deletions(-) diff --git a/src/unix/internal.h b/src/unix/internal.h index 23f16f7b..dd46c95e 100644 --- a/src/unix/internal.h +++ b/src/unix/internal.h @@ -139,8 +139,6 @@ int uv__stream_open(uv_stream_t*, int fd, int flags); void uv__stream_destroy(uv_stream_t* stream); void uv__server_io(uv_loop_t* loop, uv__io_t* watcher, int events); int uv__accept(int sockfd); -int uv__connect(uv_connect_t* req, uv_stream_t* stream, struct sockaddr* addr, - socklen_t addrlen, uv_connect_cb cb); /* tcp */ int uv_tcp_listen(uv_tcp_t* tcp, int backlog, uv_connection_cb cb); diff --git a/src/unix/stream.c b/src/unix/stream.c index 8c33ee09..e26b8723 100644 --- a/src/unix/stream.c +++ b/src/unix/stream.c @@ -797,65 +797,6 @@ static void uv__stream_connect(uv_stream_t* stream) { } -int uv__connect(uv_connect_t* req, uv_stream_t* stream, struct sockaddr* addr, - socklen_t addrlen, uv_connect_cb cb) { - int sockfd; - int r; - - if (stream->type != UV_TCP) - return uv__set_sys_error(stream->loop, ENOTSOCK); - - if (stream->connect_req) - return uv__set_sys_error(stream->loop, EALREADY); - - if (stream->fd <= 0) { - sockfd = uv__socket(addr->sa_family, SOCK_STREAM, 0); - - if (sockfd == -1) - return uv__set_sys_error(stream->loop, errno); - - if (uv__stream_open(stream, - sockfd, - UV_STREAM_READABLE | UV_STREAM_WRITABLE)) { - close(sockfd); - return -1; - } - } - - stream->delayed_error = 0; - - do - r = connect(stream->fd, addr, addrlen); - while (r == -1 && errno == EINTR); - - if (r == -1) { - if (errno == EINPROGRESS) - ; /* not an error */ - else if (errno == ECONNREFUSED) - /* If we get a ECONNREFUSED wait until the next tick to report the - * error. Solaris wants to report immediately--other unixes want to - * wait. - */ - stream->delayed_error = errno; - else - return uv__set_sys_error(stream->loop, errno); - } - - uv__req_init(stream->loop, req, UV_CONNECT); - req->cb = cb; - req->handle = stream; - ngx_queue_init(&req->queue); - stream->connect_req = req; - - uv__io_start(stream->loop, &stream->write_watcher); - - if (stream->delayed_error) - uv__io_feed(stream->loop, &stream->write_watcher, UV__IO_WRITE); - - return 0; -} - - int uv_write2(uv_write_t* req, uv_stream_t* stream, uv_buf_t bufs[], int bufcnt, uv_stream_t* send_handle, uv_write_cb cb) { int empty_queue; diff --git a/src/unix/tcp.c b/src/unix/tcp.c index 07ad2d9e..223bb048 100644 --- a/src/unix/tcp.c +++ b/src/unix/tcp.c @@ -79,6 +79,67 @@ out: } +static int uv__connect(uv_connect_t* req, + uv_tcp_t* handle, + struct sockaddr* addr, + socklen_t addrlen, + uv_connect_cb cb) { + int sockfd; + int r; + + assert(handle->type == UV_TCP); + + if (handle->connect_req) + return uv__set_sys_error(handle->loop, EALREADY); + + if (handle->fd <= 0) { + sockfd = uv__socket(addr->sa_family, SOCK_STREAM, 0); + + if (sockfd == -1) + return uv__set_sys_error(handle->loop, errno); + + if (uv__stream_open((uv_stream_t*)handle, + sockfd, + UV_STREAM_READABLE | UV_STREAM_WRITABLE)) { + close(sockfd); + return -1; + } + } + + handle->delayed_error = 0; + + do + r = connect(handle->fd, addr, addrlen); + while (r == -1 && errno == EINTR); + + if (r == -1) { + if (errno == EINPROGRESS) + ; /* not an error */ + else if (errno == ECONNREFUSED) + /* If we get a ECONNREFUSED wait until the next tick to report the + * error. Solaris wants to report immediately--other unixes want to + * wait. + */ + handle->delayed_error = errno; + else + return uv__set_sys_error(handle->loop, errno); + } + + uv__req_init(handle->loop, req, UV_CONNECT); + req->cb = cb; + req->handle = (uv_stream_t*) handle; + ngx_queue_init(&req->queue); + handle->connect_req = req; + + uv__io_start(handle->loop, &handle->write_watcher); + + if (handle->delayed_error) + uv__io_feed(handle->loop, &handle->write_watcher, UV__IO_WRITE); + + return 0; +} + + int uv__tcp_bind(uv_tcp_t* handle, struct sockaddr_in addr) { return uv__bind(handle, AF_INET, @@ -209,37 +270,31 @@ int uv_tcp_listen(uv_tcp_t* tcp, int backlog, uv_connection_cb cb) { int uv__tcp_connect(uv_connect_t* req, - uv_tcp_t* handle, - struct sockaddr_in address, - uv_connect_cb cb) { - int saved_errno = errno; + uv_tcp_t* handle, + struct sockaddr_in addr, + uv_connect_cb cb) { + int saved_errno; int status; - status = uv__connect(req, - (uv_stream_t*)handle, - (struct sockaddr*)&address, - sizeof address, - cb); - + saved_errno = errno; + status = uv__connect(req, handle, (struct sockaddr*)&addr, sizeof addr, cb); errno = saved_errno; + return status; } int uv__tcp_connect6(uv_connect_t* req, - uv_tcp_t* handle, - struct sockaddr_in6 address, - uv_connect_cb cb) { - int saved_errno = errno; + uv_tcp_t* handle, + struct sockaddr_in6 addr, + uv_connect_cb cb) { + int saved_errno; int status; - status = uv__connect(req, - (uv_stream_t*)handle, - (struct sockaddr*)&address, - sizeof address, - cb); - + saved_errno = errno; + status = uv__connect(req, handle, (struct sockaddr*)&addr, sizeof addr, cb); errno = saved_errno; + return status; } From 1a6b6b781c1b30ed6ff694d934c7dac4f975cf81 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 29 Jun 2012 18:43:44 +0200 Subject: [PATCH 02/24] unix: deduplicate socket creation code in tcp.c Incidentally fixes a rather obscure bug where uv_tcp_connect() reconnected and leaked a file descriptor when the handle was already busy connecting, handle->fd was zero (unlikely) and uv_tcp_connect() got called again. --- src/unix/tcp.c | 85 ++++++++++++++++++-------------------------------- 1 file changed, 31 insertions(+), 54 deletions(-) diff --git a/src/unix/tcp.c b/src/unix/tcp.c index 223bb048..233be825 100644 --- a/src/unix/tcp.c +++ b/src/unix/tcp.c @@ -34,6 +34,26 @@ int uv_tcp_init(uv_loop_t* loop, uv_tcp_t* tcp) { } +static int maybe_new_socket(uv_tcp_t* handle, int domain, int flags) { + int sockfd; + + if (handle->fd != -1) + return 0; + + sockfd = uv__socket(domain, SOCK_STREAM, 0); + + if (sockfd == -1) + return uv__set_sys_error(handle->loop, errno); + + if (uv__stream_open((uv_stream_t*)handle, sockfd, flags)) { + close(sockfd); + return -1; + } + + return 0; +} + + static int uv__bind(uv_tcp_t* tcp, int domain, struct sockaddr* addr, @@ -44,23 +64,8 @@ static int uv__bind(uv_tcp_t* tcp, saved_errno = errno; status = -1; - if (tcp->fd < 0) { - if ((tcp->fd = uv__socket(domain, SOCK_STREAM, 0)) == -1) { - uv__set_sys_error(tcp->loop, errno); - goto out; - } - - if (uv__stream_open((uv_stream_t*)tcp, - tcp->fd, - UV_STREAM_READABLE | UV_STREAM_WRITABLE)) { - close(tcp->fd); - tcp->fd = -1; - status = -2; - goto out; - } - } - - assert(tcp->fd >= 0); + if (maybe_new_socket(tcp, domain, UV_STREAM_READABLE|UV_STREAM_WRITABLE)) + return -1; tcp->delayed_error = 0; if (bind(tcp->fd, addr, addrsize) == -1) { @@ -84,7 +89,6 @@ static int uv__connect(uv_connect_t* req, struct sockaddr* addr, socklen_t addrlen, uv_connect_cb cb) { - int sockfd; int r; assert(handle->type == UV_TCP); @@ -92,18 +96,10 @@ static int uv__connect(uv_connect_t* req, if (handle->connect_req) return uv__set_sys_error(handle->loop, EALREADY); - if (handle->fd <= 0) { - sockfd = uv__socket(addr->sa_family, SOCK_STREAM, 0); - - if (sockfd == -1) - return uv__set_sys_error(handle->loop, errno); - - if (uv__stream_open((uv_stream_t*)handle, - sockfd, - UV_STREAM_READABLE | UV_STREAM_WRITABLE)) { - close(sockfd); - return -1; - } + if (maybe_new_socket(handle, + addr->sa_family, + UV_STREAM_READABLE|UV_STREAM_WRITABLE)) { + return -1; } handle->delayed_error = 0; @@ -231,33 +227,14 @@ out: int uv_tcp_listen(uv_tcp_t* tcp, int backlog, uv_connection_cb cb) { - int r; + if (tcp->delayed_error) + return uv__set_sys_error(tcp->loop, tcp->delayed_error); - if (tcp->delayed_error) { - uv__set_sys_error(tcp->loop, tcp->delayed_error); + if (maybe_new_socket(tcp, AF_INET, UV_STREAM_READABLE)) return -1; - } - if (tcp->fd < 0) { - if ((tcp->fd = uv__socket(AF_INET, SOCK_STREAM, 0)) == -1) { - uv__set_sys_error(tcp->loop, errno); - return -1; - } - - if (uv__stream_open((uv_stream_t*)tcp, tcp->fd, UV_STREAM_READABLE)) { - close(tcp->fd); - tcp->fd = -1; - return -1; - } - } - - assert(tcp->fd >= 0); - - r = listen(tcp->fd, backlog); - if (r < 0) { - uv__set_sys_error(tcp->loop, errno); - return -1; - } + if (listen(tcp->fd, backlog)) + return uv__set_sys_error(tcp->loop, errno); tcp->connection_cb = cb; From 0971598d025a4ef13430912e7b5cb931438a2333 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 29 Jun 2012 19:16:40 +0200 Subject: [PATCH 03/24] unix: fix EINPROGRESS busy loop Don't make the event loop spin when connect() returns EINPROGRESS. Test case: #include "uv.h" static void connect_cb(uv_connect_t* req, int status) { // ... } int main() { uv_tcp_t handle; uv_connect_t req; struct sockaddr_in addr; addr = uv_ip4_addr("8.8.8.8", 1234); // unreachable uv_tcp_init(uv_default_loop(), &handle); uv_tcp_connect(&req, (uv_stream_t*)&handle, addr, connect_cb); uv_run(uv_default_loop()); // busy loops until connection times out return 0; } After EINPROGRESS, there are zero active handles and one active request. That in turn makes uv__poll_timeout() believe that it should merely poll, not block, in epoll() / kqueue() / port_getn(). Sidestep that by artificially starting the handle on connect() and stopping it again once the TCP handshake completes / is rejected / times out. It's a slightly hacky approach because I don't want to change the ABI of the stable branch. I'll address it properly in the master branch. --- src/unix/internal.h | 3 ++- src/unix/stream.c | 6 ++++++ src/unix/tcp.c | 13 +++++++++++-- 3 files changed, 19 insertions(+), 3 deletions(-) diff --git a/src/unix/internal.h b/src/unix/internal.h index dd46c95e..382c2722 100644 --- a/src/unix/internal.h +++ b/src/unix/internal.h @@ -93,7 +93,8 @@ enum { UV_STREAM_WRITABLE = 0x40, /* The stream is writable */ UV_STREAM_BLOCKING = 0x80, /* Synchronous writes. */ UV_TCP_NODELAY = 0x100, /* Disable Nagle. */ - UV_TCP_KEEPALIVE = 0x200 /* Turn on keep-alive. */ + UV_TCP_KEEPALIVE = 0x200, /* Turn on keep-alive. */ + UV_TCP_CONNECTING = 0x400 /* Not alway set. See uv_connect() in tcp.c */ }; inline static void uv__req_init(uv_loop_t* loop, diff --git a/src/unix/stream.c b/src/unix/stream.c index e26b8723..0dbf1d78 100644 --- a/src/unix/stream.c +++ b/src/unix/stream.c @@ -790,6 +790,12 @@ static void uv__stream_connect(uv_stream_t* stream) { stream->connect_req = NULL; uv__req_unregister(stream->loop, req); + /* Hack. See uv__connect() in tcp.c */ + if (stream->flags & UV_TCP_CONNECTING) { + assert(stream->type == UV_TCP); + uv__handle_stop(stream); + } + if (req->cb) { uv__set_sys_error(stream->loop, error); req->cb(req, error ? -1 : 0); diff --git a/src/unix/tcp.c b/src/unix/tcp.c index 233be825..1aeba0ef 100644 --- a/src/unix/tcp.c +++ b/src/unix/tcp.c @@ -109,8 +109,17 @@ static int uv__connect(uv_connect_t* req, while (r == -1 && errno == EINTR); if (r == -1) { - if (errno == EINPROGRESS) - ; /* not an error */ + if (errno == EINPROGRESS) { + /* Not an error. However, we need to keep the event loop from spinning + * while the connection is in progress. Artificially start the handle + * and stop it again in uv__stream_connect() in stream.c. Yes, it's a + * hack but there's no good alternative, the v0.8 ABI is frozen. + */ + if (!uv__is_active(handle)) { + handle->flags |= UV_TCP_CONNECTING; + uv__handle_start(handle); + } + } else if (errno == ECONNREFUSED) /* If we get a ECONNREFUSED wait until the next tick to report the * error. Solaris wants to report immediately--other unixes want to From 1d1dd9bb7dc0da40296ab02e011b610bca4d93c5 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 30 Jun 2012 02:59:29 +0200 Subject: [PATCH 04/24] test: add 'unexpected read' tcp test Regression test that verifies that the event loop doesn't busy loop when the server sends a message and the client isn't reading. --- test/test-list.h | 2 + test/test-tcp-unexpected-read.c | 111 ++++++++++++++++++++++++++++++++ uv.gyp | 1 + 3 files changed, 114 insertions(+) create mode 100644 test/test-tcp-unexpected-read.c diff --git a/test/test-list.h b/test/test-list.h index c1836fbd..76651499 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -56,6 +56,7 @@ TEST_DECLARE (tcp_close) TEST_DECLARE (tcp_flags) TEST_DECLARE (tcp_write_error) TEST_DECLARE (tcp_write_to_half_open_connection) +TEST_DECLARE (tcp_unexpected_read) TEST_DECLARE (tcp_bind6_error_addrinuse) TEST_DECLARE (tcp_bind6_error_addrnotavail) TEST_DECLARE (tcp_bind6_error_fault) @@ -251,6 +252,7 @@ TASK_LIST_START TEST_ENTRY (tcp_flags) TEST_ENTRY (tcp_write_error) TEST_ENTRY (tcp_write_to_half_open_connection) + TEST_ENTRY (tcp_unexpected_read) TEST_ENTRY (tcp_bind6_error_addrinuse) TEST_ENTRY (tcp_bind6_error_addrnotavail) diff --git a/test/test-tcp-unexpected-read.c b/test/test-tcp-unexpected-read.c new file mode 100644 index 00000000..7adb4dae --- /dev/null +++ b/test/test-tcp-unexpected-read.c @@ -0,0 +1,111 @@ +/* Copyright Joyent, Inc. and other Node contributors. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include "uv.h" +#include "task.h" + +static uv_check_t check_handle; +static uv_timer_t timer_handle; +static uv_tcp_t server_handle; +static uv_tcp_t client_handle; +static uv_tcp_t peer_handle; +static uv_write_t write_req; +static uv_connect_t connect_req; + +static unsigned long ticks; /* event loop ticks */ + + +static void check_cb(uv_check_t* handle, int status) { + ticks++; +} + + +static void timer_cb(uv_timer_t* handle, int status) { + uv_close((uv_handle_t*) &check_handle, NULL); + uv_close((uv_handle_t*) &timer_handle, NULL); + uv_close((uv_handle_t*) &server_handle, NULL); + uv_close((uv_handle_t*) &client_handle, NULL); + uv_close((uv_handle_t*) &peer_handle, NULL); +} + + +static uv_buf_t alloc_cb(uv_handle_t* handle, size_t suggested_size) { + ASSERT(0 && "alloc_cb should not have been called"); +} + + +static void read_cb(uv_stream_t* handle, ssize_t nread, uv_buf_t buf) { + ASSERT(0 && "read_cb should not have been called"); +} + + +static void connect_cb(uv_connect_t* req, int status) { + ASSERT(req->handle == (uv_stream_t*) &client_handle); + ASSERT(0 == status); +} + + +static void write_cb(uv_write_t* req, int status) { + ASSERT(req->handle == (uv_stream_t*) &peer_handle); + ASSERT(0 == status); +} + + +static void connection_cb(uv_stream_t* handle, int status) { + uv_buf_t buf; + + buf = uv_buf_init("PING", 4); + + ASSERT(0 == status); + ASSERT(0 == uv_tcp_init(uv_default_loop(), &peer_handle)); + ASSERT(0 == uv_accept(handle, (uv_stream_t*) &peer_handle)); + ASSERT(0 == uv_read_start((uv_stream_t*) &peer_handle, alloc_cb, read_cb)); + ASSERT(0 == uv_write(&write_req, (uv_stream_t*) &peer_handle, + &buf, 1, write_cb)); +} + + +TEST_IMPL(tcp_unexpected_read) { + struct sockaddr_in addr; + uv_loop_t* loop; + + addr = uv_ip4_addr("127.0.0.1", TEST_PORT); + loop = uv_default_loop(); + + ASSERT(0 == uv_timer_init(loop, &timer_handle)); + ASSERT(0 == uv_timer_start(&timer_handle, timer_cb, 1000, 0)); + ASSERT(0 == uv_check_init(loop, &check_handle)); + ASSERT(0 == uv_check_start(&check_handle, check_cb)); + ASSERT(0 == uv_tcp_init(loop, &server_handle)); + ASSERT(0 == uv_tcp_init(loop, &client_handle)); + ASSERT(0 == uv_tcp_bind(&server_handle, addr)); + ASSERT(0 == uv_listen((uv_stream_t*) &server_handle, 1, connection_cb)); + ASSERT(0 == uv_tcp_connect(&connect_req, &client_handle, addr, connect_cb)); + ASSERT(0 == uv_run(loop)); + + /* This is somewhat inexact but the idea is that the event loop should not + * start busy looping when the server sends a message and the client isn't + * reading. + */ + ASSERT(ticks <= 10); + + return 0; +} diff --git a/uv.gyp b/uv.gyp index 5b9273ab..0fa50338 100644 --- a/uv.gyp +++ b/uv.gyp @@ -355,6 +355,7 @@ 'test/test-tcp-write-error.c', 'test/test-tcp-write-to-half-open-connection.c', 'test/test-tcp-writealot.c', + 'test/test-tcp-unexpected-read.c', 'test/test-threadpool.c', 'test/test-mutexes.c', 'test/test-thread.c', From 3b8c0da5a53b47174e173d2cba3c824d307b20c4 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 30 Jun 2012 03:10:08 +0200 Subject: [PATCH 05/24] unix: fix busy loop on unexpected tcp message Don't start reading immediately after connecting. If the server sends a message and the client hasn't called uv_read_start() yet, the event loop will busy loop because the pending message keeps waking it up. --- src/unix/stream.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/unix/stream.c b/src/unix/stream.c index 0dbf1d78..bab65807 100644 --- a/src/unix/stream.c +++ b/src/unix/stream.c @@ -784,9 +784,6 @@ static void uv__stream_connect(uv_stream_t* stream) { if (error == EINPROGRESS) return; - if (error == 0) - uv__io_start(stream->loop, &stream->read_watcher); - stream->connect_req = NULL; uv__req_unregister(stream->loop, req); From 889ab216aeffa6ea3ceee946cdff04f8c9e5041a Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sun, 1 Jul 2012 23:54:24 +0200 Subject: [PATCH 06/24] unix: fix 'zero handles, one request' busy loop Fixes #484. --- src/unix/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/unix/core.c b/src/unix/core.c index bf429948..9f20d04b 100644 --- a/src/unix/core.c +++ b/src/unix/core.c @@ -228,7 +228,7 @@ void uv_loop_delete(uv_loop_t* loop) { static unsigned int uv__poll_timeout(uv_loop_t* loop) { - if (!uv__has_active_handles(loop)) + if (!uv__has_active_handles(loop) && !uv__has_active_reqs(loop)) return 0; if (!ngx_queue_empty(&loop->idle_handles)) From cc1b3de2473231edfb8a078e90c2cbb14392316e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 2 Jul 2012 00:00:20 +0200 Subject: [PATCH 07/24] unix: revert 0971598, obsoleted by 889ab21 --- src/unix/internal.h | 3 +-- src/unix/stream.c | 6 ------ src/unix/tcp.c | 13 ++----------- 3 files changed, 3 insertions(+), 19 deletions(-) diff --git a/src/unix/internal.h b/src/unix/internal.h index 382c2722..dd46c95e 100644 --- a/src/unix/internal.h +++ b/src/unix/internal.h @@ -93,8 +93,7 @@ enum { UV_STREAM_WRITABLE = 0x40, /* The stream is writable */ UV_STREAM_BLOCKING = 0x80, /* Synchronous writes. */ UV_TCP_NODELAY = 0x100, /* Disable Nagle. */ - UV_TCP_KEEPALIVE = 0x200, /* Turn on keep-alive. */ - UV_TCP_CONNECTING = 0x400 /* Not alway set. See uv_connect() in tcp.c */ + UV_TCP_KEEPALIVE = 0x200 /* Turn on keep-alive. */ }; inline static void uv__req_init(uv_loop_t* loop, diff --git a/src/unix/stream.c b/src/unix/stream.c index bab65807..284643e4 100644 --- a/src/unix/stream.c +++ b/src/unix/stream.c @@ -787,12 +787,6 @@ static void uv__stream_connect(uv_stream_t* stream) { stream->connect_req = NULL; uv__req_unregister(stream->loop, req); - /* Hack. See uv__connect() in tcp.c */ - if (stream->flags & UV_TCP_CONNECTING) { - assert(stream->type == UV_TCP); - uv__handle_stop(stream); - } - if (req->cb) { uv__set_sys_error(stream->loop, error); req->cb(req, error ? -1 : 0); diff --git a/src/unix/tcp.c b/src/unix/tcp.c index 1aeba0ef..233be825 100644 --- a/src/unix/tcp.c +++ b/src/unix/tcp.c @@ -109,17 +109,8 @@ static int uv__connect(uv_connect_t* req, while (r == -1 && errno == EINTR); if (r == -1) { - if (errno == EINPROGRESS) { - /* Not an error. However, we need to keep the event loop from spinning - * while the connection is in progress. Artificially start the handle - * and stop it again in uv__stream_connect() in stream.c. Yes, it's a - * hack but there's no good alternative, the v0.8 ABI is frozen. - */ - if (!uv__is_active(handle)) { - handle->flags |= UV_TCP_CONNECTING; - uv__handle_start(handle); - } - } + if (errno == EINPROGRESS) + ; /* not an error */ else if (errno == ECONNREFUSED) /* If we get a ECONNREFUSED wait until the next tick to report the * error. Solaris wants to report immediately--other unixes want to From 5031a5b85a3f22836d41a2e86620ed72836c8824 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 22 Jun 2012 18:31:29 +0200 Subject: [PATCH 08/24] unix: rename linux/core.c to linux/linux-core.c This is a back-port of commit e1320757 from the master branch. Newer versions of gyp do not support files with the same basenames (example: core.c and linux/core.c). The nominal reason is consistency across build systems. Apparently, msbuild doesn't support it either. Somewhere, someplace, baby Jesus cries sad little tears... Fixes #464. --- config-unix.mk | 4 +++- src/unix/linux/{core.c => linux-core.c} | 0 uv.gyp | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) rename src/unix/linux/{core.c => linux-core.c} (100%) diff --git a/config-unix.mk b/config-unix.mk index 7220f108..0581b511 100644 --- a/config-unix.mk +++ b/config-unix.mk @@ -67,7 +67,9 @@ EIO_CONFIG=config_linux.h CSTDFLAG += -D_GNU_SOURCE CPPFLAGS += -Isrc/ares/config_linux LINKFLAGS+=-ldl -lrt -OBJS += src/unix/linux/core.o src/unix/linux/inotify.o src/unix/linux/syscalls.o +OBJS += src/unix/linux/linux-core.o \ + src/unix/linux/inotify.o \ + src/unix/linux/syscalls.o endif ifeq (FreeBSD,$(uname_S)) diff --git a/src/unix/linux/core.c b/src/unix/linux/linux-core.c similarity index 100% rename from src/unix/linux/core.c rename to src/unix/linux/linux-core.c diff --git a/uv.gyp b/uv.gyp index 0fa50338..96c6b724 100644 --- a/uv.gyp +++ b/uv.gyp @@ -233,7 +233,7 @@ [ 'OS=="linux"', { 'include_dirs': [ 'src/ares/config_linux' ], 'sources': [ - 'src/unix/linux/core.c', + 'src/unix/linux/linux-core.c', 'src/unix/linux/inotify.c', 'src/unix/linux/syscalls.c', 'src/unix/linux/syscalls.h', From 68b0c85c0958e7b057c28c2ef0c50dfb632a28aa Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 3 Jul 2012 21:25:10 +0200 Subject: [PATCH 09/24] test: allow 80 ms intervals in hrtime test The hrtimer functionality on my FreeBSD 9 system is fairly coarse, it's usually just over the 60 ms that we tested for before this commit. --- test/test-hrtime.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test-hrtime.c b/test/test-hrtime.c index 2a9156ec..72a4d4b1 100644 --- a/test/test-hrtime.c +++ b/test/test-hrtime.c @@ -47,7 +47,7 @@ TEST_IMPL(hrtime) { /* Check that the difference between the two hrtime values is somewhat in */ /* the range we expect it to be. */ ASSERT(diff > (uint64_t) 25 * NANOSEC / MILLISEC); - ASSERT(diff < (uint64_t) 60 * NANOSEC / MILLISEC); + ASSERT(diff < (uint64_t) 80 * NANOSEC / MILLISEC); --i; } return 0; From be09be7f3efd349d3bb9e7ed07d08b9155cabbb1 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 4 Jul 2012 14:06:35 +0200 Subject: [PATCH 10/24] unix: fix memory corruption in freebsd.c --- src/unix/freebsd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/unix/freebsd.c b/src/unix/freebsd.c index d916b0b1..f6f441ff 100644 --- a/src/unix/freebsd.c +++ b/src/unix/freebsd.c @@ -261,7 +261,7 @@ uv_err_t uv_cpu_info(uv_cpu_info_t** cpu_infos, int* count) { return uv__new_sys_error(ENOMEM); } - if (sysctlbyname("kern.cp_times", &cp_times, &size, NULL, 0) < 0) { + if (sysctlbyname("kern.cp_times", cp_times, &size, NULL, 0) < 0) { free(cp_times); free(*cpu_infos); return uv__new_sys_error(errno); From 3726dee5e932563bdfce5b545023efe326b18544 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Sun, 8 Jul 2012 11:44:05 -0400 Subject: [PATCH 11/24] unix: thread: use mach semaphores on osx --- include/uv-private/uv-unix.h | 13 +++++++++++- src/unix/thread.c | 40 ++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/include/uv-private/uv-unix.h b/include/uv-private/uv-unix.h index da185e29..7b86c0ea 100644 --- a/include/uv-private/uv-unix.h +++ b/include/uv-private/uv-unix.h @@ -40,10 +40,17 @@ #include #include -#include #include #include +#if defined(__APPLE__) && defined(__MACH__) +# include +# include +# include +#else +# include +#endif + #if __sun # include # include @@ -67,7 +74,11 @@ typedef pthread_once_t uv_once_t; typedef pthread_t uv_thread_t; typedef pthread_mutex_t uv_mutex_t; typedef pthread_rwlock_t uv_rwlock_t; +#if defined(__APPLE__) && defined(__MACH__) +typedef semaphore_t uv_sem_t; +#else typedef sem_t uv_sem_t; +#endif /* Platform-specific definitions for uv_spawn support. */ typedef gid_t uv_gid_t; diff --git a/src/unix/thread.c b/src/unix/thread.c index cd4e3333..a267d337 100644 --- a/src/unix/thread.c +++ b/src/unix/thread.c @@ -167,6 +167,44 @@ void uv_once(uv_once_t* guard, void (*callback)(void)) { abort(); } +#if defined(__APPLE__) && defined(__MACH__) + +int uv_sem_init(uv_sem_t* sem, unsigned int value) { + return semaphore_create(mach_task_self(), sem, SYNC_POLICY_FIFO, value); +} + + +void uv_sem_destroy(uv_sem_t* sem) { + if (semaphore_destroy(mach_task_self(), *sem)) + abort(); +} + + +void uv_sem_post(uv_sem_t* sem) { + if (semaphore_signal(*sem)) + abort(); +} + + +void uv_sem_wait(uv_sem_t* sem) { + if (semaphore_wait(*sem)) + abort(); +} + + +int uv_sem_trywait(uv_sem_t* sem) { + mach_timespec_t interval; + + interval.tv_sec = 0; + interval.tv_nsec = 0; + + if (semaphore_timedwait(*sem, interval) == KERN_SUCCESS) + return 0; + else + return -1; +} + +#else /* !(defined(__APPLE__) && defined(__MACH__)) */ int uv_sem_init(uv_sem_t* sem, unsigned int value) { return sem_init(sem, 0, value); @@ -209,3 +247,5 @@ int uv_sem_trywait(uv_sem_t* sem) { return r; } + +#endif /* defined(__APPLE__) && defined(__MACH__) */ From dc97d44c561c2e2b6a38d78ae814f4df7e8e20b5 Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Wed, 11 Jul 2012 19:54:29 +0400 Subject: [PATCH 12/24] unix: move uv_set_process_title() to proctitle.c Use hijacking argv array to change process' title. It seems to be working fine on almost every platform (at least it should not break anything as it's used in nginx in a similar way). --- config-unix.mk | 4 ++ src/unix/darwin.c | 27 ---------- src/unix/linux/linux-core.c | 72 ------------------------- src/unix/openbsd.c | 30 ----------- src/unix/proctitle.c | 102 ++++++++++++++++++++++++++++++++++++ src/unix/sunos.c | 18 ------- uv.gyp | 3 ++ 7 files changed, 109 insertions(+), 147 deletions(-) create mode 100644 src/unix/proctitle.c diff --git a/config-unix.mk b/config-unix.mk index 0581b511..231d8844 100644 --- a/config-unix.mk +++ b/config-unix.mk @@ -128,6 +128,10 @@ else RUNNER_LINKFLAGS += -pthread endif +ifneq (FreeBSD,$(uname_S)) +OBJS += src/unix/proctitle.o +endif + RUNNER_LIBS= RUNNER_SRC=test/runner-unix.c diff --git a/src/unix/darwin.c b/src/unix/darwin.c index e6deb301..b3cd0122 100644 --- a/src/unix/darwin.c +++ b/src/unix/darwin.c @@ -41,8 +41,6 @@ #include #include /* sysconf */ -static char *process_title; - #if TARGET_OS_IPHONE /* see: http://developer.apple.com/library/mac/#qa/qa1398/_index.html */ uint64_t uv_hrtime() { @@ -138,31 +136,6 @@ void uv_loadavg(double avg[3]) { } -char** uv_setup_args(int argc, char** argv) { - process_title = argc ? strdup(argv[0]) : NULL; - return argv; -} - - -uv_err_t uv_set_process_title(const char* title) { - /* TODO implement me */ - return uv__new_artificial_error(UV_ENOSYS); -} - - -uv_err_t uv_get_process_title(char* buffer, size_t size) { - if (process_title) { - strncpy(buffer, process_title, size); - } else { - if (size > 0) { - buffer[0] = '\0'; - } - } - - return uv_ok_; -} - - uv_err_t uv_resident_set_memory(size_t* rss) { struct task_basic_info t_info; mach_msg_type_number_t t_info_count = TASK_BASIC_INFO_COUNT; diff --git a/src/unix/linux/linux-core.c b/src/unix/linux/linux-core.c index 34f48a94..9cde6a1e 100644 --- a/src/unix/linux/linux-core.c +++ b/src/unix/linux/linux-core.c @@ -58,11 +58,6 @@ static char buf[MAXPATHLEN + 1]; -static struct { - char *str; - size_t len; -} process_title; - /* * There's probably some way to get time from Linux than gettimeofday(). What @@ -112,73 +107,6 @@ uint64_t uv_get_total_memory(void) { } -char** uv_setup_args(int argc, char** argv) { - char **new_argv; - char **new_env; - size_t size; - int envc; - char *s; - int i; - - for (envc = 0; environ[envc]; envc++); - - s = envc ? environ[envc - 1] : argv[argc - 1]; - - process_title.str = argv[0]; - process_title.len = s + strlen(s) + 1 - argv[0]; - - size = process_title.len; - size += (argc + 1) * sizeof(char **); - size += (envc + 1) * sizeof(char **); - - if ((s = (char *) malloc(size)) == NULL) { - process_title.str = NULL; - process_title.len = 0; - return argv; - } - - new_argv = (char **) s; - new_env = new_argv + argc + 1; - s = (char *) (new_env + envc + 1); - memcpy(s, process_title.str, process_title.len); - - for (i = 0; i < argc; i++) - new_argv[i] = s + (argv[i] - argv[0]); - new_argv[argc] = NULL; - - s += environ[0] - argv[0]; - - for (i = 0; i < envc; i++) - new_env[i] = s + (environ[i] - environ[0]); - new_env[envc] = NULL; - - environ = new_env; - return new_argv; -} - - -uv_err_t uv_set_process_title(const char* title) { - /* No need to terminate, last char is always '\0'. */ - if (process_title.len) - strncpy(process_title.str, title, process_title.len - 1); - - return uv_ok_; -} - - -uv_err_t uv_get_process_title(char* buffer, size_t size) { - if (process_title.str) { - strncpy(buffer, process_title.str, size); - } else { - if (size > 0) { - buffer[0] = '\0'; - } - } - - return uv_ok_; -} - - uv_err_t uv_resident_set_memory(size_t* rss) { FILE* f; int itmp; diff --git a/src/unix/openbsd.c b/src/unix/openbsd.c index 865f8e9e..0c5d8162 100644 --- a/src/unix/openbsd.c +++ b/src/unix/openbsd.c @@ -40,9 +40,6 @@ #define NANOSEC ((uint64_t) 1e9) -static char *process_title; - - uint64_t uv_hrtime(void) { struct timespec ts; clock_gettime(CLOCK_MONOTONIC, &ts); @@ -137,33 +134,6 @@ uint64_t uv_get_total_memory(void) { } -char** uv_setup_args(int argc, char** argv) { - process_title = argc ? strdup(argv[0]) : NULL; - return argv; -} - - -uv_err_t uv_set_process_title(const char* title) { - if (process_title) free(process_title); - process_title = strdup(title); - setproctitle(title); - return uv_ok_; -} - - -uv_err_t uv_get_process_title(char* buffer, size_t size) { - if (process_title) { - strncpy(buffer, process_title, size); - } else { - if (size > 0) { - buffer[0] = '\0'; - } - } - - return uv_ok_; -} - - uv_err_t uv_resident_set_memory(size_t* rss) { kvm_t *kd = NULL; struct kinfo_proc *kinfo = NULL; diff --git a/src/unix/proctitle.c b/src/unix/proctitle.c new file mode 100644 index 00000000..616501fb --- /dev/null +++ b/src/unix/proctitle.c @@ -0,0 +1,102 @@ +/* Copyright Joyent, Inc. and other Node contributors. All rights reserved. + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to + * deal in the Software without restriction, including without limitation the + * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or + * sell copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + */ + +#include "uv.h" +#include "internal.h" + +#include +#include +#include + +/* NOTE: FreeBSD is using it's own implementation of those functions */ + +static struct { + char *str; + size_t len; +} process_title; + +extern char** environ; + + +char** uv_setup_args(int argc, char** argv) { + char **new_argv; + char **new_env; + size_t size; + int envc; + char *s; + int i; + + for (envc = 0; environ[envc]; envc++); + + s = envc ? environ[envc - 1] : argv[argc - 1]; + + process_title.str = argv[0]; + process_title.len = s + strlen(s) + 1 - argv[0]; + + size = process_title.len; + size += (argc + 1) * sizeof(char **); + size += (envc + 1) * sizeof(char **); + + if ((s = (char *) malloc(size)) == NULL) { + process_title.str = NULL; + process_title.len = 0; + return argv; + } + + new_argv = (char **) s; + new_env = new_argv + argc + 1; + s = (char *) (new_env + envc + 1); + memcpy(s, process_title.str, process_title.len); + + for (i = 0; i < argc; i++) + new_argv[i] = s + (argv[i] - argv[0]); + new_argv[argc] = NULL; + + s += environ[0] - argv[0]; + + for (i = 0; i < envc; i++) + new_env[i] = s + (environ[i] - environ[0]); + new_env[envc] = NULL; + + environ = new_env; + return new_argv; +} + + +uv_err_t uv_set_process_title(const char* title) { + /* No need to terminate, last char is always '\0'. */ + if (process_title.len) + strncpy(process_title.str, title, process_title.len - 1); + + return uv_ok_; +} + + +uv_err_t uv_get_process_title(char* buffer, size_t size) { + if (process_title.str) { + strncpy(buffer, process_title.str, size); + } else { + if (size > 0) { + buffer[0] = '\0'; + } + } + + return uv_ok_; +} diff --git a/src/unix/sunos.c b/src/unix/sunos.c index b95a89b4..1a439d94 100644 --- a/src/unix/sunos.c +++ b/src/unix/sunos.c @@ -238,24 +238,6 @@ void uv__fs_event_close(uv_fs_event_t* handle) { #endif /* HAVE_PORTS_FS */ -char** uv_setup_args(int argc, char** argv) { - return argv; -} - - -uv_err_t uv_set_process_title(const char* title) { - return uv_ok_; -} - - -uv_err_t uv_get_process_title(char* buffer, size_t size) { - if (size > 0) { - buffer[0] = '\0'; - } - return uv_ok_; -} - - uv_err_t uv_resident_set_memory(size_t* rss) { psinfo_t psinfo; uv_err_t err; diff --git a/uv.gyp b/uv.gyp index 96c6b724..1ed3b6bc 100644 --- a/uv.gyp +++ b/uv.gyp @@ -287,6 +287,9 @@ [ 'OS=="mac" or OS=="freebsd" or OS=="openbsd" or OS=="netbsd"', { 'sources': [ 'src/unix/kqueue.c' ], }], + [ 'OS!="win" and OS!="freebsd"', { + 'sources': [ 'src/unix/proctitle.c' ], + }], ] }, From a87abc7070dee4b7896c3c499bc3f0ba0a600b5a Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 13 Jul 2012 15:03:37 +0200 Subject: [PATCH 13/24] unix: avoid buffer overflow in proctitle.c Get/set process title with uv_strlcpy(), not strncpy(). The latter won't zero-terminate the result if the destination buffer is too small. --- src/unix/proctitle.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/unix/proctitle.c b/src/unix/proctitle.c index 616501fb..29099710 100644 --- a/src/unix/proctitle.c +++ b/src/unix/proctitle.c @@ -81,22 +81,12 @@ char** uv_setup_args(int argc, char** argv) { uv_err_t uv_set_process_title(const char* title) { - /* No need to terminate, last char is always '\0'. */ - if (process_title.len) - strncpy(process_title.str, title, process_title.len - 1); - + uv_strlcpy(process_title.str, title, process_title.len); return uv_ok_; } uv_err_t uv_get_process_title(char* buffer, size_t size) { - if (process_title.str) { - strncpy(buffer, process_title.str, size); - } else { - if (size > 0) { - buffer[0] = '\0'; - } - } - + uv_strlcpy(buffer, process_title.str ? process_title.str : "", size); return uv_ok_; } From a9f6f06feaf02ebb48d4b41bd2ac47fcb2096a00 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 13 Jul 2012 17:07:11 +0200 Subject: [PATCH 14/24] unix: fix format string vulnerability in freebsd.c uv_set_process_title() was susceptible to a format string vulnerability: $ node -e 'process.title = Array(42).join("%s")' Segmentation fault: 11 (core dumped) The fix is trivial - call setproctitle("%s", s) instead of setproctitle(s) - but valgrind complains loudly about reads from and writes to uninitialized memory in libc. It's not a libuv bug because the test case below triggers the same warnings: #include #include #include #include int main(void) { setproctitle("%s", "test"); return 0; } That's why this commit replaces setproctitle() with sysctl(KERN_PROC_ARGS). --- src/unix/freebsd.c | 16 +++++++++++++++- test/test-process-title.c | 13 ++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/unix/freebsd.c b/src/unix/freebsd.c index f6f441ff..be8006c5 100644 --- a/src/unix/freebsd.c +++ b/src/unix/freebsd.c @@ -139,9 +139,23 @@ char** uv_setup_args(int argc, char** argv) { uv_err_t uv_set_process_title(const char* title) { + int oid[4]; + if (process_title) free(process_title); process_title = strdup(title); - setproctitle(title); + + oid[0] = CTL_KERN; + oid[1] = KERN_PROC; + oid[2] = KERN_PROC_ARGS; + oid[3] = getpid(); + + sysctl(oid, + ARRAY_SIZE(oid), + NULL, + NULL, + process_title, + strlen(process_title) + 1); + return uv_ok_; } diff --git a/test/test-process-title.c b/test/test-process-title.c index 59fceda3..13d9dddf 100644 --- a/test/test-process-title.c +++ b/test/test-process-title.c @@ -23,20 +23,27 @@ #include "task.h" #include -TEST_IMPL(process_title) { + +static void set_title(const char* title) { char buffer[512]; uv_err_t err; err = uv_get_process_title(buffer, sizeof(buffer)); ASSERT(UV_OK == err.code); - err = uv_set_process_title("new title"); + err = uv_set_process_title(title); ASSERT(UV_OK == err.code); err = uv_get_process_title(buffer, sizeof(buffer)); ASSERT(UV_OK == err.code); - ASSERT(strcmp(buffer, "new title") == 0); + ASSERT(strcmp(buffer, title) == 0); +} + +TEST_IMPL(process_title) { + /* Check for format string vulnerabilities. */ + set_title("%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s"); + set_title("new title"); return 0; } From b49d6f7c30420d843a84c7afbe6c3498fbc3ac57 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 18 Jul 2012 00:22:06 +0200 Subject: [PATCH 15/24] unix: fix uv_set_process_title() Use strncpy() to set the process title, it pads the remainder with nul bytes. Avoids garbage output on systems where `ps aux` prints the entire proctitle buffer, not just the characters up to the first '\0'. Fixes joyent/node#3726. --- src/unix/proctitle.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/unix/proctitle.c b/src/unix/proctitle.c index 29099710..9057074a 100644 --- a/src/unix/proctitle.c +++ b/src/unix/proctitle.c @@ -81,7 +81,14 @@ char** uv_setup_args(int argc, char** argv) { uv_err_t uv_set_process_title(const char* title) { - uv_strlcpy(process_title.str, title, process_title.len); + /* proctitle doesn't need to be zero terminated, last char is always '\0'. + * Use strncpy(), it pads the remainder with nul bytes. Avoids garbage output + * on systems where `ps aux` prints the entire proctitle buffer, not just the + * characters up to the first '\0'. + */ + if (process_title.len > 0) + strncpy(process_title.str, title, process_title.len - 1); + return uv_ok_; } From 69a6afea63faba510e83cefb53cf4cd77d8dcd6f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Wed, 18 Jul 2012 22:49:49 +0200 Subject: [PATCH 16/24] unix: undo changes to uv_set_process_title() It's making node.js crash when run as root. Backtrace: (gdb) bt #0 0x00007fff856e3ff9 in __findenv () #1 0x00007fff856e404c in getenv () #2 0x000000010004c850 in loop_init (loop=0x10045a792, flags=8) at ev.c:1707 #3 0x000000010004cb3b in ev_backend [inlined] () at /Users/tjfontaine/Development/node/deps/uv/src/unix/ev/ev.c:2090 #4 0x000000010004cb3b in ev_default_loop (flags=1606417108) at ev.c:2092 #5 0x000000010004e5c6 in uv__loop_init (loop=0x10066e330, default_loop=1) at loop.c:52 #6 0x0000000100044367 in uv_default_loop () at core.c:196 #7 0x0000000100004625 in node::Init (argc=1606417456, argv=0x100b0f490) at node.cc:2761 #8 0x000000010000797d in node::Start (argc=1606417600, argv=0x0) at node.cc:2888 #9 0x0000000100000ca4 in start () This reverts commits: b49d6f7 unix: fix uv_set_process_title() a9f6f06 unix: fix format string vulnerability in freebsd.c a87abc7 unix: avoid buffer overflow in proctitle.c dc97d44 unix: move uv_set_process_title() to proctitle.c --- config-unix.mk | 4 -- src/unix/darwin.c | 27 ++++++++++ src/unix/freebsd.c | 16 +----- src/unix/linux/linux-core.c | 72 +++++++++++++++++++++++++++ src/unix/openbsd.c | 30 +++++++++++ src/unix/proctitle.c | 99 ------------------------------------- src/unix/sunos.c | 18 +++++++ test/test-process-title.c | 13 ++--- uv.gyp | 3 -- 9 files changed, 151 insertions(+), 131 deletions(-) delete mode 100644 src/unix/proctitle.c diff --git a/config-unix.mk b/config-unix.mk index 231d8844..0581b511 100644 --- a/config-unix.mk +++ b/config-unix.mk @@ -128,10 +128,6 @@ else RUNNER_LINKFLAGS += -pthread endif -ifneq (FreeBSD,$(uname_S)) -OBJS += src/unix/proctitle.o -endif - RUNNER_LIBS= RUNNER_SRC=test/runner-unix.c diff --git a/src/unix/darwin.c b/src/unix/darwin.c index b3cd0122..e6deb301 100644 --- a/src/unix/darwin.c +++ b/src/unix/darwin.c @@ -41,6 +41,8 @@ #include #include /* sysconf */ +static char *process_title; + #if TARGET_OS_IPHONE /* see: http://developer.apple.com/library/mac/#qa/qa1398/_index.html */ uint64_t uv_hrtime() { @@ -136,6 +138,31 @@ void uv_loadavg(double avg[3]) { } +char** uv_setup_args(int argc, char** argv) { + process_title = argc ? strdup(argv[0]) : NULL; + return argv; +} + + +uv_err_t uv_set_process_title(const char* title) { + /* TODO implement me */ + return uv__new_artificial_error(UV_ENOSYS); +} + + +uv_err_t uv_get_process_title(char* buffer, size_t size) { + if (process_title) { + strncpy(buffer, process_title, size); + } else { + if (size > 0) { + buffer[0] = '\0'; + } + } + + return uv_ok_; +} + + uv_err_t uv_resident_set_memory(size_t* rss) { struct task_basic_info t_info; mach_msg_type_number_t t_info_count = TASK_BASIC_INFO_COUNT; diff --git a/src/unix/freebsd.c b/src/unix/freebsd.c index be8006c5..f6f441ff 100644 --- a/src/unix/freebsd.c +++ b/src/unix/freebsd.c @@ -139,23 +139,9 @@ char** uv_setup_args(int argc, char** argv) { uv_err_t uv_set_process_title(const char* title) { - int oid[4]; - if (process_title) free(process_title); process_title = strdup(title); - - oid[0] = CTL_KERN; - oid[1] = KERN_PROC; - oid[2] = KERN_PROC_ARGS; - oid[3] = getpid(); - - sysctl(oid, - ARRAY_SIZE(oid), - NULL, - NULL, - process_title, - strlen(process_title) + 1); - + setproctitle(title); return uv_ok_; } diff --git a/src/unix/linux/linux-core.c b/src/unix/linux/linux-core.c index 9cde6a1e..34f48a94 100644 --- a/src/unix/linux/linux-core.c +++ b/src/unix/linux/linux-core.c @@ -58,6 +58,11 @@ static char buf[MAXPATHLEN + 1]; +static struct { + char *str; + size_t len; +} process_title; + /* * There's probably some way to get time from Linux than gettimeofday(). What @@ -107,6 +112,73 @@ uint64_t uv_get_total_memory(void) { } +char** uv_setup_args(int argc, char** argv) { + char **new_argv; + char **new_env; + size_t size; + int envc; + char *s; + int i; + + for (envc = 0; environ[envc]; envc++); + + s = envc ? environ[envc - 1] : argv[argc - 1]; + + process_title.str = argv[0]; + process_title.len = s + strlen(s) + 1 - argv[0]; + + size = process_title.len; + size += (argc + 1) * sizeof(char **); + size += (envc + 1) * sizeof(char **); + + if ((s = (char *) malloc(size)) == NULL) { + process_title.str = NULL; + process_title.len = 0; + return argv; + } + + new_argv = (char **) s; + new_env = new_argv + argc + 1; + s = (char *) (new_env + envc + 1); + memcpy(s, process_title.str, process_title.len); + + for (i = 0; i < argc; i++) + new_argv[i] = s + (argv[i] - argv[0]); + new_argv[argc] = NULL; + + s += environ[0] - argv[0]; + + for (i = 0; i < envc; i++) + new_env[i] = s + (environ[i] - environ[0]); + new_env[envc] = NULL; + + environ = new_env; + return new_argv; +} + + +uv_err_t uv_set_process_title(const char* title) { + /* No need to terminate, last char is always '\0'. */ + if (process_title.len) + strncpy(process_title.str, title, process_title.len - 1); + + return uv_ok_; +} + + +uv_err_t uv_get_process_title(char* buffer, size_t size) { + if (process_title.str) { + strncpy(buffer, process_title.str, size); + } else { + if (size > 0) { + buffer[0] = '\0'; + } + } + + return uv_ok_; +} + + uv_err_t uv_resident_set_memory(size_t* rss) { FILE* f; int itmp; diff --git a/src/unix/openbsd.c b/src/unix/openbsd.c index 0c5d8162..865f8e9e 100644 --- a/src/unix/openbsd.c +++ b/src/unix/openbsd.c @@ -40,6 +40,9 @@ #define NANOSEC ((uint64_t) 1e9) +static char *process_title; + + uint64_t uv_hrtime(void) { struct timespec ts; clock_gettime(CLOCK_MONOTONIC, &ts); @@ -134,6 +137,33 @@ uint64_t uv_get_total_memory(void) { } +char** uv_setup_args(int argc, char** argv) { + process_title = argc ? strdup(argv[0]) : NULL; + return argv; +} + + +uv_err_t uv_set_process_title(const char* title) { + if (process_title) free(process_title); + process_title = strdup(title); + setproctitle(title); + return uv_ok_; +} + + +uv_err_t uv_get_process_title(char* buffer, size_t size) { + if (process_title) { + strncpy(buffer, process_title, size); + } else { + if (size > 0) { + buffer[0] = '\0'; + } + } + + return uv_ok_; +} + + uv_err_t uv_resident_set_memory(size_t* rss) { kvm_t *kd = NULL; struct kinfo_proc *kinfo = NULL; diff --git a/src/unix/proctitle.c b/src/unix/proctitle.c deleted file mode 100644 index 9057074a..00000000 --- a/src/unix/proctitle.c +++ /dev/null @@ -1,99 +0,0 @@ -/* Copyright Joyent, Inc. and other Node contributors. All rights reserved. - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to - * deal in the Software without restriction, including without limitation the - * rights to use, copy, modify, merge, publish, distribute, sublicense, and/or - * sell copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in - * all copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING - * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS - * IN THE SOFTWARE. - */ - -#include "uv.h" -#include "internal.h" - -#include -#include -#include - -/* NOTE: FreeBSD is using it's own implementation of those functions */ - -static struct { - char *str; - size_t len; -} process_title; - -extern char** environ; - - -char** uv_setup_args(int argc, char** argv) { - char **new_argv; - char **new_env; - size_t size; - int envc; - char *s; - int i; - - for (envc = 0; environ[envc]; envc++); - - s = envc ? environ[envc - 1] : argv[argc - 1]; - - process_title.str = argv[0]; - process_title.len = s + strlen(s) + 1 - argv[0]; - - size = process_title.len; - size += (argc + 1) * sizeof(char **); - size += (envc + 1) * sizeof(char **); - - if ((s = (char *) malloc(size)) == NULL) { - process_title.str = NULL; - process_title.len = 0; - return argv; - } - - new_argv = (char **) s; - new_env = new_argv + argc + 1; - s = (char *) (new_env + envc + 1); - memcpy(s, process_title.str, process_title.len); - - for (i = 0; i < argc; i++) - new_argv[i] = s + (argv[i] - argv[0]); - new_argv[argc] = NULL; - - s += environ[0] - argv[0]; - - for (i = 0; i < envc; i++) - new_env[i] = s + (environ[i] - environ[0]); - new_env[envc] = NULL; - - environ = new_env; - return new_argv; -} - - -uv_err_t uv_set_process_title(const char* title) { - /* proctitle doesn't need to be zero terminated, last char is always '\0'. - * Use strncpy(), it pads the remainder with nul bytes. Avoids garbage output - * on systems where `ps aux` prints the entire proctitle buffer, not just the - * characters up to the first '\0'. - */ - if (process_title.len > 0) - strncpy(process_title.str, title, process_title.len - 1); - - return uv_ok_; -} - - -uv_err_t uv_get_process_title(char* buffer, size_t size) { - uv_strlcpy(buffer, process_title.str ? process_title.str : "", size); - return uv_ok_; -} diff --git a/src/unix/sunos.c b/src/unix/sunos.c index 1a439d94..b95a89b4 100644 --- a/src/unix/sunos.c +++ b/src/unix/sunos.c @@ -238,6 +238,24 @@ void uv__fs_event_close(uv_fs_event_t* handle) { #endif /* HAVE_PORTS_FS */ +char** uv_setup_args(int argc, char** argv) { + return argv; +} + + +uv_err_t uv_set_process_title(const char* title) { + return uv_ok_; +} + + +uv_err_t uv_get_process_title(char* buffer, size_t size) { + if (size > 0) { + buffer[0] = '\0'; + } + return uv_ok_; +} + + uv_err_t uv_resident_set_memory(size_t* rss) { psinfo_t psinfo; uv_err_t err; diff --git a/test/test-process-title.c b/test/test-process-title.c index 13d9dddf..59fceda3 100644 --- a/test/test-process-title.c +++ b/test/test-process-title.c @@ -23,27 +23,20 @@ #include "task.h" #include - -static void set_title(const char* title) { +TEST_IMPL(process_title) { char buffer[512]; uv_err_t err; err = uv_get_process_title(buffer, sizeof(buffer)); ASSERT(UV_OK == err.code); - err = uv_set_process_title(title); + err = uv_set_process_title("new title"); ASSERT(UV_OK == err.code); err = uv_get_process_title(buffer, sizeof(buffer)); ASSERT(UV_OK == err.code); - ASSERT(strcmp(buffer, title) == 0); -} + ASSERT(strcmp(buffer, "new title") == 0); - -TEST_IMPL(process_title) { - /* Check for format string vulnerabilities. */ - set_title("%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s"); - set_title("new title"); return 0; } diff --git a/uv.gyp b/uv.gyp index 1ed3b6bc..96c6b724 100644 --- a/uv.gyp +++ b/uv.gyp @@ -287,9 +287,6 @@ [ 'OS=="mac" or OS=="freebsd" or OS=="openbsd" or OS=="netbsd"', { 'sources': [ 'src/unix/kqueue.c' ], }], - [ 'OS!="win" and OS!="freebsd"', { - 'sources': [ 'src/unix/proctitle.c' ], - }], ] }, From e3a28508b230b902f996045c0f852d1d728b5724 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 19 Jul 2012 16:13:42 +0200 Subject: [PATCH 17/24] unix: fix errno reporting in uv_pipe_connect() Remember the errno when the socket() syscall fails. --- src/unix/pipe.c | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/unix/pipe.c b/src/unix/pipe.c index 317ac67c..9e63949d 100644 --- a/src/unix/pipe.c +++ b/src/unix/pipe.c @@ -171,17 +171,15 @@ void uv_pipe_connect(uv_connect_t* req, struct sockaddr_un saddr; int saved_errno; int sockfd; - int status; + int err; int r; saved_errno = errno; sockfd = -1; - status = -1; + err = -1; - if ((sockfd = uv__socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { - uv__set_sys_error(handle->loop, errno); + if ((sockfd = uv__socket(AF_UNIX, SOCK_STREAM, 0)) == -1) goto out; - } memset(&saddr, 0, sizeof saddr); uv_strlcpy(saddr.sun_path, name, sizeof(saddr.sun_path)); @@ -196,7 +194,7 @@ void uv_pipe_connect(uv_connect_t* req, while (r == -1 && errno == EINTR); if (r == -1) { - status = errno; + err = errno; close(sockfd); goto out; } @@ -206,10 +204,10 @@ void uv_pipe_connect(uv_connect_t* req, UV_STREAM_READABLE | UV_STREAM_WRITABLE); uv__io_start(handle->loop, &handle->read_watcher); uv__io_start(handle->loop, &handle->write_watcher); - status = 0; + err = 0; out: - handle->delayed_error = status; /* Passed to callback. */ + handle->delayed_error = err ? errno : 0; /* Passed to callback. */ handle->connect_req = req; uv__req_init(handle->loop, req, UV_CONNECT); From ff59525c7e70b0c1c1ba11ed65ad3f1c6ddf92f1 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Thu, 19 Jul 2012 16:25:49 +0200 Subject: [PATCH 18/24] unix: fix uv_pipe_connect() with existing fd Don't create a new socket descriptor if one has been previously assigned with uv_pipe_open(). --- src/unix/pipe.c | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/src/unix/pipe.c b/src/unix/pipe.c index 9e63949d..957e96f8 100644 --- a/src/unix/pipe.c +++ b/src/unix/pipe.c @@ -170,16 +170,17 @@ void uv_pipe_connect(uv_connect_t* req, uv_connect_cb cb) { struct sockaddr_un saddr; int saved_errno; - int sockfd; + int new_sock; int err; int r; saved_errno = errno; - sockfd = -1; + new_sock = (handle->fd == -1); err = -1; - if ((sockfd = uv__socket(AF_UNIX, SOCK_STREAM, 0)) == -1) - goto out; + if (new_sock) + if ((handle->fd = uv__socket(AF_UNIX, SOCK_STREAM, 0)) == -1) + goto out; memset(&saddr, 0, sizeof saddr); uv_strlcpy(saddr.sun_path, name, sizeof(saddr.sun_path)); @@ -189,19 +190,19 @@ void uv_pipe_connect(uv_connect_t* req, * is either there or not. */ do { - r = connect(sockfd, (struct sockaddr*)&saddr, sizeof saddr); + r = connect(handle->fd, (struct sockaddr*)&saddr, sizeof saddr); } while (r == -1 && errno == EINTR); - if (r == -1) { - err = errno; - close(sockfd); + if (r == -1) goto out; - } - uv__stream_open((uv_stream_t*)handle, - sockfd, - UV_STREAM_READABLE | UV_STREAM_WRITABLE); + if (new_sock) + if (uv__stream_open((uv_stream_t*)handle, + handle->fd, + UV_STREAM_READABLE | UV_STREAM_WRITABLE)) + goto out; + uv__io_start(handle->loop, &handle->read_watcher); uv__io_start(handle->loop, &handle->write_watcher); err = 0; From 94355e4718a788c5f2d97f1d9da34cbcd2c74b03 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 13 Jul 2012 17:07:11 +0200 Subject: [PATCH 19/24] unix: fix format string vulnerability in freebsd.c uv_set_process_title() was susceptible to a format string vulnerability: $ node -e 'process.title = Array(42).join("%s")' Segmentation fault: 11 (core dumped) The fix is trivial - call setproctitle("%s", s) instead of setproctitle(s) - but valgrind complains loudly about reads from and writes to uninitialized memory in libc. It's not a libuv bug because the test case below triggers the same warnings: #include #include int main(void) { setproctitle("%s", "test"); return 0; } That's why this commit replaces setproctitle() with sysctl(KERN_PROC_ARGS). This commit reapplies commit a9f6f06, which got reverted in 69a6afe. The revert turned out to be unnecessary. --- src/unix/freebsd.c | 16 +++++++++++++++- test/test-process-title.c | 13 ++++++++++--- 2 files changed, 25 insertions(+), 4 deletions(-) diff --git a/src/unix/freebsd.c b/src/unix/freebsd.c index f6f441ff..be8006c5 100644 --- a/src/unix/freebsd.c +++ b/src/unix/freebsd.c @@ -139,9 +139,23 @@ char** uv_setup_args(int argc, char** argv) { uv_err_t uv_set_process_title(const char* title) { + int oid[4]; + if (process_title) free(process_title); process_title = strdup(title); - setproctitle(title); + + oid[0] = CTL_KERN; + oid[1] = KERN_PROC; + oid[2] = KERN_PROC_ARGS; + oid[3] = getpid(); + + sysctl(oid, + ARRAY_SIZE(oid), + NULL, + NULL, + process_title, + strlen(process_title) + 1); + return uv_ok_; } diff --git a/test/test-process-title.c b/test/test-process-title.c index 59fceda3..13d9dddf 100644 --- a/test/test-process-title.c +++ b/test/test-process-title.c @@ -23,20 +23,27 @@ #include "task.h" #include -TEST_IMPL(process_title) { + +static void set_title(const char* title) { char buffer[512]; uv_err_t err; err = uv_get_process_title(buffer, sizeof(buffer)); ASSERT(UV_OK == err.code); - err = uv_set_process_title("new title"); + err = uv_set_process_title(title); ASSERT(UV_OK == err.code); err = uv_get_process_title(buffer, sizeof(buffer)); ASSERT(UV_OK == err.code); - ASSERT(strcmp(buffer, "new title") == 0); + ASSERT(strcmp(buffer, title) == 0); +} + +TEST_IMPL(process_title) { + /* Check for format string vulnerabilities. */ + set_title("%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s%s"); + set_title("new title"); return 0; } From 22f004db61c11f63ed07da2a326c7edbbfee5730 Mon Sep 17 00:00:00 2001 From: Shuhei Tanuma Date: Sat, 21 Jul 2012 22:29:55 +0900 Subject: [PATCH 20/24] unix: don't abort() when trylock functions return EBUSY Fixes #500. --- src/unix/thread.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/unix/thread.c b/src/unix/thread.c index a267d337..67c78769 100644 --- a/src/unix/thread.c +++ b/src/unix/thread.c @@ -78,7 +78,7 @@ int uv_mutex_trylock(uv_mutex_t* mutex) { r = pthread_mutex_trylock(mutex); - if (r && r != EAGAIN) + if (r && r != EBUSY && r != EAGAIN) abort(); if (r) @@ -119,7 +119,7 @@ int uv_rwlock_tryrdlock(uv_rwlock_t* rwlock) { r = pthread_rwlock_tryrdlock(rwlock); - if (r && r != EAGAIN) + if (r && r != EBUSY && r != EAGAIN) abort(); if (r) @@ -146,7 +146,7 @@ int uv_rwlock_trywrlock(uv_rwlock_t* rwlock) { r = pthread_rwlock_trywrlock(rwlock); - if (r && r != EAGAIN) + if (r && r != EBUSY && r != EAGAIN) abort(); if (r) From b5b8ead8089ed3eef039c68bccd8021cfc092446 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 28 Jul 2012 14:40:01 +0200 Subject: [PATCH 21/24] test: add failing fs_event test Watches the same file twice. Fails on Linux with a segmentation fault. --- test/test-fs-event.c | 32 ++++++++++++++++++++++++++++++++ test/test-list.h | 2 ++ 2 files changed, 34 insertions(+) diff --git a/test/test-fs-event.c b/test/test-fs-event.c index fe7bce2a..4d4cfbc3 100644 --- a/test/test-fs-event.c +++ b/test/test-fs-event.c @@ -85,6 +85,13 @@ static void close_cb(uv_handle_t* handle) { close_cb_called++; } +static void fail_cb(uv_fs_event_t* handle, + const char* path, + int events, + int status) { + ASSERT(0 && "fail_cb called"); +} + static void fs_event_cb_dir(uv_fs_event_t* handle, const char* filename, int events, int status) { ++fs_event_cb_called; @@ -159,6 +166,13 @@ static void timer_cb_touch(uv_timer_t* timer, int status) { timer_cb_touch_called++; } +static void timer_cb_watch_twice(uv_timer_t* handle, int status) { + uv_fs_event_t* handles = handle->data; + uv_close((uv_handle_t*) (handles + 0), NULL); + uv_close((uv_handle_t*) (handles + 1), NULL); + uv_close((uv_handle_t*) handle, NULL); +} + TEST_IMPL(fs_event_watch_dir) { uv_fs_t fs_req; uv_loop_t* loop = uv_default_loop(); @@ -225,6 +239,24 @@ TEST_IMPL(fs_event_watch_file) { return 0; } +TEST_IMPL(fs_event_watch_file_twice) { + const char path[] = "test/fixtures/empty_file"; + uv_fs_event_t watchers[2]; + uv_timer_t timer; + uv_loop_t* loop; + + loop = uv_default_loop(); + timer.data = watchers; + + ASSERT(0 == uv_fs_event_init(loop, watchers + 0, path, fail_cb, 0)); + ASSERT(0 == uv_fs_event_init(loop, watchers + 1, path, fail_cb, 0)); + ASSERT(0 == uv_timer_init(loop, &timer)); + ASSERT(0 == uv_timer_start(&timer, timer_cb_watch_twice, 10, 0)); + ASSERT(0 == uv_run(loop)); + + return 0; +} + TEST_IMPL(fs_event_watch_file_current_dir) { uv_timer_t timer; uv_loop_t* loop; diff --git a/test/test-list.h b/test/test-list.h index 76651499..19670e56 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -158,6 +158,7 @@ TEST_DECLARE (fs_stat_missing_path) TEST_DECLARE (fs_read_file_eof) TEST_DECLARE (fs_event_watch_dir) TEST_DECLARE (fs_event_watch_file) +TEST_DECLARE (fs_event_watch_file_twice) TEST_DECLARE (fs_event_watch_file_current_dir) TEST_DECLARE (fs_event_no_callback_on_close) TEST_DECLARE (fs_event_immediate_close) @@ -396,6 +397,7 @@ TASK_LIST_START TEST_ENTRY (fs_file_open_append) TEST_ENTRY (fs_event_watch_dir) TEST_ENTRY (fs_event_watch_file) + TEST_ENTRY (fs_event_watch_file_twice) TEST_ENTRY (fs_event_watch_file_current_dir) TEST_ENTRY (fs_event_no_callback_on_close) TEST_ENTRY (fs_event_immediate_close) From 4fe369b179024f539637dc0255e454c405310444 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 28 Jul 2012 14:47:20 +0200 Subject: [PATCH 22/24] test: add uv_fs_event_t to benchmark-sizes.c --- test/benchmark-sizes.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/benchmark-sizes.c b/test/benchmark-sizes.c index 6d16e3ca..605a3e6f 100644 --- a/test/benchmark-sizes.c +++ b/test/benchmark-sizes.c @@ -36,6 +36,7 @@ BENCHMARK_IMPL(sizes) { LOGF("uv_idle_t: %u bytes\n", (unsigned int) sizeof(uv_idle_t)); LOGF("uv_async_t: %u bytes\n", (unsigned int) sizeof(uv_async_t)); LOGF("uv_timer_t: %u bytes\n", (unsigned int) sizeof(uv_timer_t)); + LOGF("uv_fs_event_t: %u bytes\n", (unsigned int) sizeof(uv_fs_event_t)); LOGF("uv_process_t: %u bytes\n", (unsigned int) sizeof(uv_process_t)); LOGF("uv_poll_t: %u bytes\n", (unsigned int) sizeof(uv_poll_t)); return 0; From ec76a42515156364f9dc57d4b85acca47fedc2ca Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 28 Jul 2012 14:56:36 +0200 Subject: [PATCH 23/24] test: add uv_loop_t to benchmark-sizes.c --- test/benchmark-sizes.c | 1 + 1 file changed, 1 insertion(+) diff --git a/test/benchmark-sizes.c b/test/benchmark-sizes.c index 605a3e6f..b9cf74f2 100644 --- a/test/benchmark-sizes.c +++ b/test/benchmark-sizes.c @@ -39,5 +39,6 @@ BENCHMARK_IMPL(sizes) { LOGF("uv_fs_event_t: %u bytes\n", (unsigned int) sizeof(uv_fs_event_t)); LOGF("uv_process_t: %u bytes\n", (unsigned int) sizeof(uv_process_t)); LOGF("uv_poll_t: %u bytes\n", (unsigned int) sizeof(uv_poll_t)); + LOGF("uv_loop_t: %u bytes\n", (unsigned int) sizeof(uv_loop_t)); return 0; } From 4fe1916926f9e0deb904179e55501da426cd434f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Sat, 28 Jul 2012 16:11:59 +0200 Subject: [PATCH 24/24] linux: fix 'two watchers, one path' segfault Problem: registering two uv_fs_event_t watchers for the same path, then closing them, caused a segmentation fault. While active, the watchers didn't work right either, only one would receive events. Cause: each watcher has a wd (watch descriptor) that's used as its key in a binary tree. When you call inotify_watch_add() twice with the same path, the second call doesn't return a new wd - it returns the existing one. That in turn resulted in the first handle getting ousted from the binary tree, leaving dangling pointers. This commit addresses that by storing the watchers in a queue and storing the queue in the binary tree instead of storing the watchers directly in the tree. Fixes joyent/node#3789. --- include/uv-private/uv-unix.h | 17 ++---- src/unix/linux/inotify.c | 110 ++++++++++++++++++++++------------- src/unix/loop.c | 2 +- 3 files changed, 77 insertions(+), 52 deletions(-) diff --git a/include/uv-private/uv-unix.h b/include/uv-private/uv-unix.h index 7b86c0ea..40bbdf6e 100644 --- a/include/uv-private/uv-unix.h +++ b/include/uv-private/uv-unix.h @@ -105,11 +105,8 @@ struct uv__io_s { #if __linux__ # define UV_LOOP_PRIVATE_PLATFORM_FIELDS \ - /* RB_HEAD(uv__inotify_watchers, uv_fs_event_s) */ \ - struct uv__inotify_watchers { \ - struct uv_fs_event_s* rbh_root; \ - } inotify_watchers; \ uv__io_t inotify_read_watcher; \ + void* inotify_watchers; \ int inotify_fd; #elif defined(PORT_SOURCE_FILE) # define UV_LOOP_PRIVATE_PLATFORM_FIELDS \ @@ -277,15 +274,11 @@ struct uv__io_s { #if defined(__linux__) #define UV_FS_EVENT_PRIVATE_FIELDS \ - /* RB_ENTRY(fs_event_s) node; */ \ - struct { \ - struct uv_fs_event_s* rbe_left; \ - struct uv_fs_event_s* rbe_right; \ - struct uv_fs_event_s* rbe_parent; \ - int rbe_color; \ - } node; \ + ngx_queue_t watchers; \ uv_fs_event_cb cb; \ - int fd; \ + int wd; \ + void* pad0; \ + void* pad1; \ #elif defined(__APPLE__) \ || defined(__FreeBSD__) \ diff --git a/src/unix/linux/inotify.c b/src/unix/linux/inotify.c index 54525bc7..51ea996a 100644 --- a/src/unix/linux/inotify.c +++ b/src/unix/linux/inotify.c @@ -33,6 +33,18 @@ #include #include +struct watcher_list { + RB_ENTRY(watcher_list) entry; + ngx_queue_t watchers; + char* path; + int wd; +}; + +struct watcher_root { + struct watcher_list* rbh_root; +}; +#define CAST(p) ((struct watcher_root*)(p)) + /* Don't look aghast, this is exactly how glibc's basename() works. */ static char* basename_r(const char* path) { @@ -41,14 +53,15 @@ static char* basename_r(const char* path) { } -static int compare_watchers(const uv_fs_event_t* a, const uv_fs_event_t* b) { - if (a->fd < b->fd) return -1; - if (a->fd > b->fd) return 1; +static int compare_watchers(const struct watcher_list* a, + const struct watcher_list* b) { + if (a->wd < b->wd) return -1; + if (a->wd > b->wd) return 1; return 0; } -RB_GENERATE_STATIC(uv__inotify_watchers, uv_fs_event_s, node, compare_watchers) +RB_GENERATE_STATIC(watcher_root, watcher_list, entry, compare_watchers) static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int revents); @@ -95,36 +108,27 @@ static int init_inotify(uv_loop_t* loop) { } -static void add_watcher(uv_fs_event_t* handle) { - RB_INSERT(uv__inotify_watchers, &handle->loop->inotify_watchers, handle); +static struct watcher_list* find_watcher(uv_loop_t* loop, int wd) { + struct watcher_list w; + w.wd = wd; + return RB_FIND(watcher_root, CAST(&loop->inotify_watchers), &w); } -static uv_fs_event_t* find_watcher(uv_loop_t* loop, int wd) { - uv_fs_event_t handle; - handle.fd = wd; - return RB_FIND(uv__inotify_watchers, &loop->inotify_watchers, &handle); -} - - -static void remove_watcher(uv_fs_event_t* handle) { - RB_REMOVE(uv__inotify_watchers, &handle->loop->inotify_watchers, handle); -} - - -static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int events) { +static void uv__inotify_read(uv_loop_t* loop, uv__io_t* dummy, int events) { const struct uv__inotify_event* e; - uv_fs_event_t* handle; - const char* filename; + struct watcher_list* w; + uv_fs_event_t* h; + ngx_queue_t* q; + const char* path; ssize_t size; const char *p; /* needs to be large enough for sizeof(inotify_event) + strlen(filename) */ char buf[4096]; while (1) { - do { - size = read(loop->inotify_fd, buf, sizeof buf); - } + do + size = read(loop->inotify_fd, buf, sizeof(buf)); while (size == -1 && errno == EINTR); if (size == -1) { @@ -144,17 +148,20 @@ static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int events) { if (e->mask & ~(UV__IN_ATTRIB|UV__IN_MODIFY)) events |= UV_RENAME; - handle = find_watcher(loop, e->wd); - if (handle == NULL) - continue; /* Handle has already been closed. */ + w = find_watcher(loop, e->wd); + if (w == NULL) + continue; /* Stale event, no watchers left. */ /* inotify does not return the filename when monitoring a single file * for modifications. Repurpose the filename for API compatibility. * I'm not convinced this is a good thing, maybe it should go. */ - filename = e->len ? (const char*) (e + 1) : basename_r(handle->filename); + path = e->len ? (const char*) (e + 1) : basename_r(w->path); - handle->cb(handle, filename, events, 0); + ngx_queue_foreach(q, &w->watchers) { + h = ngx_queue_data(q, uv_fs_event_t, watchers); + h->cb(h, path, events, 0); + } } } } @@ -162,9 +169,10 @@ static void uv__inotify_read(uv_loop_t* loop, uv__io_t* w, int events) { int uv_fs_event_init(uv_loop_t* loop, uv_fs_event_t* handle, - const char* filename, + const char* path, uv_fs_event_cb cb, int flags) { + struct watcher_list* w; int events; int wd; @@ -184,26 +192,50 @@ int uv_fs_event_init(uv_loop_t* loop, | UV__IN_MOVED_FROM | UV__IN_MOVED_TO; - wd = uv__inotify_add_watch(loop->inotify_fd, filename, events); - if (wd == -1) return uv__set_sys_error(loop, errno); + wd = uv__inotify_add_watch(loop->inotify_fd, path, events); + if (wd == -1) + return uv__set_sys_error(loop, errno); + w = find_watcher(loop, wd); + if (w) + goto no_insert; + + w = malloc(sizeof(*w) + strlen(path) + 1); + if (w == NULL) + return uv__set_sys_error(loop, ENOMEM); + + w->wd = wd; + w->path = strcpy((char*)(w + 1), path); + ngx_queue_init(&w->watchers); + RB_INSERT(watcher_root, CAST(&loop->inotify_watchers), w); + +no_insert: uv__handle_init(loop, (uv_handle_t*)handle, UV_FS_EVENT); uv__handle_start(handle); /* FIXME shouldn't start automatically */ - handle->filename = strdup(filename); + ngx_queue_insert_tail(&w->watchers, &handle->watchers); + handle->filename = w->path; handle->cb = cb; - handle->fd = wd; - add_watcher(handle); + handle->wd = wd; return 0; } void uv__fs_event_close(uv_fs_event_t* handle) { - uv__inotify_rm_watch(handle->loop->inotify_fd, handle->fd); - remove_watcher(handle); - handle->fd = -1; + struct watcher_list* w; - free(handle->filename); + w = find_watcher(handle->loop, handle->wd); + assert(w != NULL); + + handle->wd = -1; handle->filename = NULL; uv__handle_stop(handle); + ngx_queue_remove(&handle->watchers); + + if (ngx_queue_empty(&w->watchers)) { + /* No watchers left for this path. Clean up. */ + RB_REMOVE(watcher_root, CAST(&handle->loop->inotify_watchers), w); + uv__inotify_rm_watch(handle->loop->inotify_fd, w->wd); + free(w); + } } diff --git a/src/unix/loop.c b/src/unix/loop.c index 75fad436..fcf9faf3 100644 --- a/src/unix/loop.c +++ b/src/unix/loop.c @@ -54,7 +54,7 @@ int uv__loop_init(uv_loop_t* loop, int default_loop) { eio_channel_init(&loop->uv_eio_channel, loop); #if __linux__ - RB_INIT(&loop->inotify_watchers); + loop->inotify_watchers = NULL; loop->inotify_fd = -1; #endif #if HAVE_PORTS_FS