From 8ba8a6da281a422dc54df59b6da394bd784cf9ea Mon Sep 17 00:00:00 2001 From: Felipe Ruiz Date: Mon, 6 Nov 2017 08:40:10 -0200 Subject: [PATCH 1/4] Add test for current socket behavior on exec --- test.c | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/test.c b/test.c index a23d60676..b86899c71 100644 --- a/test.c +++ b/test.c @@ -647,6 +647,48 @@ static void test_throughput(struct config config) { disconnect(c, 0); } +static void test_close_on_exec(struct config config) { + + redisContext *c; + + c = connect(config); + { + FILE *pipe_fp; + char readbuf[16]; + + { + char command_fmt[] = "stat /proc/self/fd/%d 2>&1"; + int command_size; + char *command; + + command_size = snprintf(NULL, 0, command_fmt, c->fd); + command = malloc((command_size + 1) * sizeof(command)); + if (snprintf(command, command_size + 1, command_fmt, c->fd) < command_size) { + fprintf(stderr, "Failed to allocate test command\n"); + exit(1); + } + + if ((pipe_fp = popen(command, "r")) == NULL) { + fprintf(stderr, "Failed to popen\n"); + free(command); + exit(1); + } + free(command); + } + + if (fgets(readbuf, 16, pipe_fp) == NULL) { + fprintf(stderr, "Error reading from pipe"); + pclose(pipe_fp); + exit(1); + } + pclose(pipe_fp); + + test("Keep socket on fork+exec without setting close-on-exec: "); + test_cond(strncmp(readbuf, "stat: cannot", strlen("stat: cannot")) != 0); + } + disconnect(c, 0); +} + // static long __test_callback_flags = 0; // static void __test_callback(redisContext *c, void *privdata) { // ((void)c); @@ -798,6 +840,7 @@ int main(int argc, char **argv) { test_invalid_timeout_errors(cfg); test_append_formatted_commands(cfg); if (throughput) test_throughput(cfg); + test_close_on_exec(cfg); printf("\nTesting against Unix socket connection (%s):\n", cfg.unix_sock.path); cfg.type = CONN_UNIX; From 38ff0fa754376b9215999c40720bb055235ba58c Mon Sep 17 00:00:00 2001 From: Felipe Ruiz Date: Mon, 6 Nov 2017 08:49:52 -0200 Subject: [PATCH 2/4] Allow setting CLOEXEC on socket creation In order to prevent file descriptor leakage when calling fork(2)+exec(2), the FD_CLOEXEC flag needs to be set. Just setting the flag using fcntl(2) after socket creation is, however, not enough, since, in a multi-threaded application, another thread might fork after we created the socket but before we had the chance of setting the flag. To remedy this, Linux introduced the SOCK_CLOEXEC flag in 2.6.27, to be set on socket creation. At the time of this commit, it has been ported to other systems, such as FreeBSD. Should the flag not be available, as is the case in macOS or older versions of the Linux kernel, we do the next best thing, which is trying to call fcntl(2) right after socket creation. --- hiredis.c | 41 +++++++++++++++++++++++++++++++++++++++++ hiredis.h | 6 ++++++ net.c | 43 ++++++++++++++++++++++++++++++++++++++++++- test.c | 43 ++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 131 insertions(+), 2 deletions(-) diff --git a/hiredis.c b/hiredis.c index 3a7ab11ae..dc741826a 100644 --- a/hiredis.c +++ b/hiredis.c @@ -724,6 +724,47 @@ redisContext *redisConnectBindNonBlockWithReuse(const char *ip, int port, return c; } +redisContext *redisConnectCloseOnExec(const char *ip, int port) { + redisContext *c; + + c = redisContextInit(); + if (c == NULL) { + return NULL; + } + + c->flags |= REDIS_BLOCK; + c->flags |= REDIS_CLOEXEC; + redisContextConnectTcp(c,ip,port,NULL); + return c; +} + +redisContext *redisConnectWithTimeoutCloseOnExec(const char *ip, int port, const struct timeval tv) { + redisContext *c; + + c = redisContextInit(); + if (c == NULL) + return NULL; + + c->flags |= REDIS_BLOCK; + c->flags |= REDIS_CLOEXEC; + redisContextConnectTcp(c,ip,port,&tv); + return c; +} + +redisContext *redisConnectNonBlockCloseOnExec(const char *ip, int port) { + redisContext *c; + + c = redisContextInit(); + if (c == NULL) + return NULL; + + c->flags &= ~REDIS_BLOCK; + c->flags |= REDIS_CLOEXEC; + redisContextConnectTcp(c,ip,port,NULL); + return c; +} + + redisContext *redisConnectUnix(const char *path) { redisContext *c; diff --git a/hiredis.h b/hiredis.h index 77d5797eb..4eea231b8 100644 --- a/hiredis.h +++ b/hiredis.h @@ -74,6 +74,9 @@ /* Flag that is set when we should set SO_REUSEADDR before calling bind() */ #define REDIS_REUSEADDR 0x80 +/* Flag that is set when we should set FD_CLOEXEC when opening the socket */ +#define REDIS_CLOEXEC 0x100 + #define REDIS_KEEPALIVE_INTERVAL 15 /* seconds */ /* number of times we retry to connect in the case of EADDRNOTAVAIL and @@ -167,6 +170,9 @@ redisContext *redisConnectBindNonBlock(const char *ip, int port, const char *source_addr); redisContext *redisConnectBindNonBlockWithReuse(const char *ip, int port, const char *source_addr); +redisContext *redisConnectCloseOnExec(const char *ip, int port); +redisContext *redisConnectWithTimeoutCloseOnExec(const char *ip, int port, const struct timeval tv); +redisContext *redisConnectNonBlockCloseOnExec(const char *ip, int port); redisContext *redisConnectUnix(const char *path); redisContext *redisConnectUnixWithTimeout(const char *path, const struct timeval tv); redisContext *redisConnectUnixNonBlock(const char *path); diff --git a/net.c b/net.c index 33d9a8297..5086fadec 100644 --- a/net.c +++ b/net.c @@ -54,6 +54,14 @@ #include "net.h" #include "sds.h" +#ifdef SOCK_CLOEXEC +/* sock_cloexec is initialized to SOCK_CLOEXEC and cleared to zero if + * a socket() call ever fails with EINVAL. */ +static int sock_cloexec = SOCK_CLOEXEC; +#else +#define sock_cloexec 0 +#endif + /* Defined in hiredis.c */ void __redisSetError(redisContext *c, int type, const char *str); @@ -270,8 +278,10 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port, struct addrinfo hints, *servinfo, *bservinfo, *p, *b; int blocking = (c->flags & REDIS_BLOCK); int reuseaddr = (c->flags & REDIS_REUSEADDR); + int set_cloexec = (c->flags & REDIS_CLOEXEC); int reuses = 0; long timeout_msec = -1; + int socket_type; servinfo = NULL; c->connection_type = REDIS_CONN_TCP; @@ -336,9 +346,40 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port, } for (p = servinfo; p != NULL; p = p->ai_next) { addrretry: - if ((s = socket(p->ai_family,p->ai_socktype,p->ai_protocol)) == -1) + socket_type = p->ai_socktype; + if (set_cloexec) { + socket_type |= sock_cloexec; + } + + s = socket(p->ai_family, socket_type, p->ai_protocol); + /* If we attempted to open the socket with SOCK_CLOEXEC and + * errno == EINVAL, that means setting it on socket creation + * is not supported. We clear sock_cloexec so it doesn't do + * anything the next time we use it, and try to create it + * without SOCK_CLOEXEC. */ + if (s == -1 && (socket_type & sock_cloexec) && errno == EINVAL) { + socket_type &= ~sock_cloexec; + sock_cloexec = 0; + s = socket(p->ai_family, socket_type, p->ai_protocol); + } + + if (s == -1) continue; + /* If creating the socket with SOCK_CLOEXEC failed, we need to set + * FD_CLOEXEC on it ASAP in order to minimize the possibility that + * another thread in the running program will fork and inherit the + * file descriptor, so we do this right after checking if the socket + * has been created. Note that we're being optmistic here and there's + * no guarantees that we will be able to set it in time. */ + if (set_cloexec && !sock_cloexec) { + if (fcntl(s, F_SETFD, FD_CLOEXEC) == -1) { + __redisSetErrorFromErrno(c, REDIS_ERR_IO, "fcntl(F_SETFD)"); + redisContextCloseFd(c); + return REDIS_ERR; + } + } + c->fd = s; if (redisSetBlocking(c,0) != REDIS_OK) goto error; diff --git a/test.c b/test.c index b86899c71..5ec66ae73 100644 --- a/test.c +++ b/test.c @@ -16,7 +16,8 @@ enum connection_type { CONN_TCP, CONN_UNIX, - CONN_FD + CONN_FD, + CONN_TCP_CLOEXEC }; struct config { @@ -106,6 +107,8 @@ static redisContext *connect(struct config config) { printf("Connecting to inherited fd %d\n", fd); c = redisConnectFd(fd); } + } else if (config.type == CONN_TCP_CLOEXEC) { + c = redisConnectCloseOnExec(config.tcp.host, config.tcp.port); } else { assert(NULL); } @@ -687,6 +690,44 @@ static void test_close_on_exec(struct config config) { test_cond(strncmp(readbuf, "stat: cannot", strlen("stat: cannot")) != 0); } disconnect(c, 0); + + config.type = CONN_TCP_CLOEXEC; + c = connect(config); + { + FILE *pipe_fp; + char readbuf[16]; + + { + char command_fmt[] = "stat /proc/self/fd/%d 2>&1"; + int command_size; + char *command; + + command_size = snprintf(NULL, 0, command_fmt, c->fd); + command = malloc((command_size + 1) * sizeof(command)); + if (snprintf(command, command_size + 1, command_fmt, c->fd) < command_size) { + fprintf(stderr, "Failed to allocate test command\n"); + exit(1); + } + + if ((pipe_fp = popen(command, "r")) == NULL) { + fprintf(stderr, "Failed to popen\n"); + free(command); + exit(1); + } + free(command); + } + + if (fgets(readbuf, 16, pipe_fp) == NULL) { + fprintf(stderr, "Error reading from pipe"); + pclose(pipe_fp); + exit(1); + } + pclose(pipe_fp); + + test("Don't keep socket on fork+exec when setting close-on-exec: "); + test_cond(strncmp(readbuf, "stat: cannot", strlen("stat: cannot")) == 0); + } + disconnect(c, 0); } // static long __test_callback_flags = 0; From a4439da5c67cb06284c3661b86893a2d0eca5ca2 Mon Sep 17 00:00:00 2001 From: Felipe Ruiz Date: Mon, 6 Nov 2017 11:03:44 -0200 Subject: [PATCH 3/4] Fix compilation error on macOS --- net.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/net.c b/net.c index 5086fadec..0352119fa 100644 --- a/net.c +++ b/net.c @@ -56,10 +56,10 @@ #ifdef SOCK_CLOEXEC /* sock_cloexec is initialized to SOCK_CLOEXEC and cleared to zero if - * a socket() call ever fails with EINVAL. */ + * socket(2) ever fails with EINVAL when SOCK_CLOEXEC is set. */ static int sock_cloexec = SOCK_CLOEXEC; #else -#define sock_cloexec 0 +static int sock_cloexec = 0; #endif /* Defined in hiredis.c */ From c512b7da497e99d3d8032c61ffb17a6bfefa16c6 Mon Sep 17 00:00:00 2001 From: Felipe Ruiz Date: Mon, 6 Nov 2017 15:28:27 -0200 Subject: [PATCH 4/4] Refactor tests using lsof MacOS doesn't have procfs, which is what we were using for testing. --- test.c | 33 +++++++++++---------------------- 1 file changed, 11 insertions(+), 22 deletions(-) diff --git a/test.c b/test.c index 5ec66ae73..9b696e00b 100644 --- a/test.c +++ b/test.c @@ -653,20 +653,20 @@ static void test_throughput(struct config config) { static void test_close_on_exec(struct config config) { redisContext *c; + char command_fmt[] = "echo $$ > /dev/null; lsof -t -R -a -i @%s:%d -p $$"; c = connect(config); { FILE *pipe_fp; - char readbuf[16]; + int status; { - char command_fmt[] = "stat /proc/self/fd/%d 2>&1"; int command_size; char *command; - command_size = snprintf(NULL, 0, command_fmt, c->fd); + command_size = snprintf(NULL, 0, command_fmt, config.tcp.host, config.tcp.port); command = malloc((command_size + 1) * sizeof(command)); - if (snprintf(command, command_size + 1, command_fmt, c->fd) < command_size) { + if (snprintf(command, command_size + 1, command_fmt, config.tcp.host, config.tcp.port) < command_size) { fprintf(stderr, "Failed to allocate test command\n"); exit(1); } @@ -679,15 +679,10 @@ static void test_close_on_exec(struct config config) { free(command); } - if (fgets(readbuf, 16, pipe_fp) == NULL) { - fprintf(stderr, "Error reading from pipe"); - pclose(pipe_fp); - exit(1); - } - pclose(pipe_fp); + status = pclose(pipe_fp); test("Keep socket on fork+exec without setting close-on-exec: "); - test_cond(strncmp(readbuf, "stat: cannot", strlen("stat: cannot")) != 0); + test_cond(WIFEXITED(status) && WEXITSTATUS(status) == 0); } disconnect(c, 0); @@ -695,16 +690,15 @@ static void test_close_on_exec(struct config config) { c = connect(config); { FILE *pipe_fp; - char readbuf[16]; + int status; { - char command_fmt[] = "stat /proc/self/fd/%d 2>&1"; int command_size; char *command; - command_size = snprintf(NULL, 0, command_fmt, c->fd); + command_size = snprintf(NULL, 0, command_fmt, config.tcp.host, config.tcp.port); command = malloc((command_size + 1) * sizeof(command)); - if (snprintf(command, command_size + 1, command_fmt, c->fd) < command_size) { + if (snprintf(command, command_size + 1, command_fmt, config.tcp.host, config.tcp.port) < command_size) { fprintf(stderr, "Failed to allocate test command\n"); exit(1); } @@ -717,15 +711,10 @@ static void test_close_on_exec(struct config config) { free(command); } - if (fgets(readbuf, 16, pipe_fp) == NULL) { - fprintf(stderr, "Error reading from pipe"); - pclose(pipe_fp); - exit(1); - } - pclose(pipe_fp); + status = pclose(pipe_fp); test("Don't keep socket on fork+exec when setting close-on-exec: "); - test_cond(strncmp(readbuf, "stat: cannot", strlen("stat: cannot")) == 0); + test_cond(WIFEXITED(status) && WEXITSTATUS(status) == 1); } disconnect(c, 0); }