From 8e8af8fd348152da27abb080f9a8643ad4a2f295 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Fri, 29 Jul 2011 02:45:38 +0200 Subject: [PATCH] uv-unix: use lock file to detect stale UNIX sockets --- include/uv-unix.h | 1 + src/uv-unix.c | 101 ++++++++++++++++++++++++++++++++++------------ 2 files changed, 77 insertions(+), 25 deletions(-) diff --git a/include/uv-unix.h b/include/uv-unix.h index 72c66a9f..2f084b27 100644 --- a/include/uv-unix.h +++ b/include/uv-unix.h @@ -88,6 +88,7 @@ typedef struct { #define UV_PIPE_PRIVATE_FIELDS \ UV_TCP_PRIVATE_FIELDS \ const char* pipe_fname; /* strdup'ed */ \ + void* pipe_flock; /* UV_PREPARE */ \ diff --git a/src/uv-unix.c b/src/uv-unix.c index 8348fabd..0f6433d0 100644 --- a/src/uv-unix.c +++ b/src/uv-unix.c @@ -92,8 +92,11 @@ int uv_flock_init(uv_flock_t* lock, const char* filename); /* Try to acquire the file lock. Returns 0 on success, -1 on error. * Does not wait for the lock to be released if it is held by another process. + * + * If `locked` is not NULL, the memory pointed it points to is set to 1 if + * the file is locked by another process. Only relevant in error scenarios. */ -int uv_flock_acquire(uv_flock_t* lock); +int uv_flock_acquire(uv_flock_t* lock, int* locked); /* Release the file lock. Returns 0 on success, -1 on error. */ @@ -111,6 +114,7 @@ static uv_err_t uv_err_new(uv_handle_t* handle, int sys_error); static int uv_tcp_listen(uv_tcp_t* tcp, int backlog, uv_connection_cb cb); static int uv_pipe_listen(uv_pipe_t* handle, int backlog, uv_connection_cb cb); +static int uv_pipe_cleanup(uv_pipe_t* handle); static uv_write_t* uv__write(uv_stream_t* stream); static void uv__read(uv_stream_t* stream); static void uv__stream_connect(uv_stream_t*); @@ -270,17 +274,8 @@ void uv_close(uv_handle_t* handle, uv_close_cb close_cb) { case UV_NAMED_PIPE: pipe = (uv_pipe_t*)handle; - if (pipe->pipe_fname) { - /* - * Unlink the file system entity before closing the file descriptor. - * Doing it the other way around introduces a race where our process - * unlinks a socket with the same name that's just been created by - * another thread or process. - */ - unlink(pipe->pipe_fname); - free((void*)pipe->pipe_fname); - } - uv_read_stop((uv_stream_t*)pipe); + uv_pipe_cleanup(pipe); + uv_read_stop((uv_stream_t*)handle); ev_io_stop(EV_DEFAULT_ &pipe->write_watcher); break; @@ -1836,6 +1831,7 @@ int uv_pipe_init(uv_pipe_t* handle) { uv_counters()->pipe_init++; handle->type = UV_NAMED_PIPE; + handle->pipe_flock = NULL; /* Only set by listener. */ handle->pipe_fname = NULL; /* Only set by listener. */ ev_init(&handle->write_watcher, uv__stream_io); @@ -1854,13 +1850,16 @@ int uv_pipe_init(uv_pipe_t* handle) { int uv_pipe_bind(uv_pipe_t* handle, const char* name) { struct sockaddr_un sun; const char* pipe_fname; + uv_flock_t* pipe_flock; int saved_errno; + int locked; int sockfd; int status; int bound; saved_errno = errno; pipe_fname = NULL; + pipe_flock = NULL; sockfd = -1; status = -1; bound = 0; @@ -1880,6 +1879,20 @@ int uv_pipe_bind(uv_pipe_t* handle, const char* name) { /* We've got a copy, don't touch the original any more. */ name = NULL; + /* Create and acquire a file lock for this UNIX socket. */ + if ((pipe_flock = malloc(sizeof *pipe_flock)) == NULL + || uv_flock_init(pipe_flock, pipe_fname) == -1) { + uv_err_new((uv_handle_t*)handle, ENOMEM); + goto out; + } + + if (uv_flock_acquire(pipe_flock, &locked) == -1) { + /* Another process holds the lock so the socket is in use. */ + uv_err_new_artificial((uv_handle_t*)handle, + locked ? UV_EADDRINUSE : UV_EACCESS); + goto out; + } + if ((sockfd = uv__socket(AF_UNIX, SOCK_STREAM, 0)) == -1) { uv_err_new((uv_handle_t*)handle, errno); goto out; @@ -1890,17 +1903,13 @@ int uv_pipe_bind(uv_pipe_t* handle, const char* name) { sun.sun_family = AF_UNIX; if (bind(sockfd, (struct sockaddr*)&sun, sizeof sun) == -1) { -#ifdef DONT_RACE_ME_BRO - /* - * Try to bind the socket. Note that we explicitly don't act - * on EADDRINUSE. Unlinking and trying to bind again opens - * a window for races with other threads and processes. - */ - uv_err_new((uv_handle_t*)handle, (errno == ENOENT) ? EACCES : errno); - goto out; -#else - /* - * Try to re-purpose the socket. This is a potential race window. + /* On EADDRINUSE: + * + * We hold the file lock so there is no other process listening + * on the socket. Ergo, it's stale - remove it. + * + * This assumes that the other process uses locking too + * but that's a good enough assumption for now. */ if (errno != EADDRINUSE || unlink(pipe_fname) == -1 @@ -1909,7 +1918,6 @@ int uv_pipe_bind(uv_pipe_t* handle, const char* name) { uv_err_new((uv_handle_t*)handle, (errno == ENOENT) ? EACCES : errno); goto out; } -#endif } bound = 1; @@ -1927,6 +1935,11 @@ out: unlink(pipe_fname); } uv__close(sockfd); + + if (pipe_flock) { + uv_flock_destroy(pipe_flock); + } + free((void*)pipe_fname); } @@ -1962,6 +1975,37 @@ out: } +static int uv_pipe_cleanup(uv_pipe_t* handle) { + int saved_errno; + int status; + + saved_errno = errno; + status = -1; + + if (handle->pipe_fname) { + /* + * Unlink the file system entity before closing the file descriptor. + * Doing it the other way around introduces a race where our process + * unlinks a socket with the same name that's just been created by + * another thread or process. + * + * This is less of an issue now that we attach a file lock + * to the socket but it's still a best practice. + */ + unlink(handle->pipe_fname); + free((void*)handle->pipe_fname); + } + + if (handle->pipe_flock) { + uv_flock_destroy((uv_flock_t*)handle->pipe_flock); + free(handle->pipe_flock); + } + + errno = saved_errno; + return status; +} + + int uv_pipe_connect(uv_connect_t* req, uv_pipe_t* handle, const char* name, @@ -2367,15 +2411,17 @@ out: #undef LOCKFILE_SUFFIX -int uv_flock_acquire(uv_flock_t* lock) { +int uv_flock_acquire(uv_flock_t* lock, int* locked_p) { char buf[32]; int saved_errno; int status; int lockfd; + int locked; saved_errno = errno; status = -1; lockfd = -1; + locked = 0; do { lockfd = open(lock->lockfile, O_WRONLY | O_CREAT, 0666); @@ -2392,6 +2438,7 @@ int uv_flock_acquire(uv_flock_t* lock) { while (status == -1 && errno == EINTR); if (status == -1) { + locked = (errno == EAGAIN); /* Lock is held by another process. */ goto out; } @@ -2409,6 +2456,10 @@ out: uv__close(lockfd); } + if (locked_p) { + *locked_p = locked; + } + errno = saved_errno; return status; }