Prevent FD leakage when calling fork(2)+exec(2)

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;
 

hiredis.h

@@ -37,6 +37,7 @@
 #include <stdarg.h> /* for va_list */
 #include <sys/time.h> /* for struct timeval */
 #include <stdint.h> /* uintXX_t, etc */
+#include <string.h> /* for strerror_r */
 #include "sds.h" /* for sds */
 
 #define HIREDIS_MAJOR 0
@@ -74,6 +75,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 +171,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);

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;

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);
     }
@@ -647,6 +650,86 @@ 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);
+
+    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;
 // static void __test_callback(redisContext *c, void *privdata) {
 //     ((void)c);
@@ -798,6 +881,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;