From fb66e2e24943018961321d13e46ee2ab66de882a Mon Sep 17 00:00:00 2001 From: Andy Pan Date: Wed, 20 Jan 2021 04:57:30 +0800 Subject: [PATCH] Use FD_CLOEXEC in Sentinel, so that FDs don't leak to the scripts it runs (#8242) Sentinel uses execve to run scripts, so it needs to use FD_CLOEXEC on all file descriptors, so that they're not accessible by the script it runs. This commit includes a change to the sentinel tests, which verifies no FDs are left opened when the script is executed. --- src/ae.c | 1 + src/ae_epoll.c | 1 + src/ae_evport.c | 1 + src/ae_kqueue.c | 1 + src/anet.c | 23 ++++++++++++++++++++ src/anet.h | 1 + src/cluster.c | 2 +- src/module.c | 5 +++++ src/networking.c | 3 +++ src/sentinel.c | 3 ++- src/server.c | 6 +++-- tests/instances.tcl | 14 +++++++++++- tests/sentinel/run.tcl | 2 +- tests/sentinel/tests/includes/init-tests.tcl | 2 ++ tests/sentinel/tests/includes/notify.sh | 20 +++++++++++++++++ 15 files changed, 79 insertions(+), 6 deletions(-) create mode 100755 tests/sentinel/tests/includes/notify.sh diff --git a/src/ae.c b/src/ae.c index 1c3a4e091..283f51438 100644 --- a/src/ae.c +++ b/src/ae.c @@ -31,6 +31,7 @@ */ #include "ae.h" +#include "anet.h" #include #include diff --git a/src/ae_epoll.c b/src/ae_epoll.c index fa197297e..07ca8ca41 100644 --- a/src/ae_epoll.c +++ b/src/ae_epoll.c @@ -51,6 +51,7 @@ static int aeApiCreate(aeEventLoop *eventLoop) { zfree(state); return -1; } + anetCloexec(state->epfd); eventLoop->apidata = state; return 0; } diff --git a/src/ae_evport.c b/src/ae_evport.c index 4e254b602..7a0b03aea 100644 --- a/src/ae_evport.c +++ b/src/ae_evport.c @@ -82,6 +82,7 @@ static int aeApiCreate(aeEventLoop *eventLoop) { zfree(state); return -1; } + anetCloexec(state->portfd); state->npending = 0; diff --git a/src/ae_kqueue.c b/src/ae_kqueue.c index 6796f4ceb..b146f2519 100644 --- a/src/ae_kqueue.c +++ b/src/ae_kqueue.c @@ -53,6 +53,7 @@ static int aeApiCreate(aeEventLoop *eventLoop) { zfree(state); return -1; } + anetCloexec(state->kqfd); eventLoop->apidata = state; return 0; } diff --git a/src/anet.c b/src/anet.c index 4e2e6be88..f2c39b200 100644 --- a/src/anet.c +++ b/src/anet.c @@ -94,6 +94,29 @@ int anetBlock(char *err, int fd) { return anetSetBlock(err,fd,0); } +/* Enable the FD_CLOEXEC on the given fd to avoid fd leaks. + * This function should be invoked for fd's on specific places + * where fork + execve system calls are called. */ +int anetCloexec(int fd) { + int r; + int flags; + + do { + r = fcntl(fd, F_GETFD); + } while (r == -1 && errno == EINTR); + + if (r == -1 || (r & FD_CLOEXEC)) + return r; + + flags = r | FD_CLOEXEC; + + do { + r = fcntl(fd, F_SETFD, flags); + } while (r == -1 && errno == EINTR); + + return r; +} + /* Set TCP keep alive option to detect dead peers. The interval option * is only used for Linux as we are using Linux-specific APIs to set * the probe send time, interval, and count. */ diff --git a/src/anet.h b/src/anet.h index fbf41cd17..dc3cbeb32 100644 --- a/src/anet.h +++ b/src/anet.h @@ -70,6 +70,7 @@ int anetUnixAccept(char *err, int serversock); int anetWrite(int fd, char *buf, int count); int anetNonBlock(char *err, int fd); int anetBlock(char *err, int fd); +int anetCloexec(int fd); int anetEnableTcpNoDelay(char *err, int fd); int anetDisableTcpNoDelay(char *err, int fd); int anetTcpKeepAlive(char *err, int fd); diff --git a/src/cluster.c b/src/cluster.c index 2a06ef6dc..2b5acec90 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -398,7 +398,7 @@ int clusterLockConfig(char *filename) { /* To lock it, we need to open the file in a way it is created if * it does not exist, otherwise there is a race condition with other * processes. */ - int fd = open(filename,O_WRONLY|O_CREAT,0644); + int fd = open(filename,O_WRONLY|O_CREAT|O_CLOEXEC,0644); if (fd == -1) { serverLog(LL_WARNING, "Can't open %s in order to acquire a lock: %s", diff --git a/src/module.c b/src/module.c index fd3f90d84..a6a1c1df7 100644 --- a/src/module.c +++ b/src/module.c @@ -7670,6 +7670,11 @@ void moduleInitModulesSystem(void) { anetNonBlock(NULL,server.module_blocked_pipe[0]); anetNonBlock(NULL,server.module_blocked_pipe[1]); + /* Enable close-on-exec flag on pipes in case of the fork-exec system calls in + * sentinels or redis servers. */ + anetCloexec(server.module_blocked_pipe[0]); + anetCloexec(server.module_blocked_pipe[1]); + /* Create the timers radix tree. */ Timers = raxNew(); diff --git a/src/networking.c b/src/networking.c index 6d5973563..26e4017c5 100644 --- a/src/networking.c +++ b/src/networking.c @@ -1104,6 +1104,7 @@ void acceptTcpHandler(aeEventLoop *el, int fd, void *privdata, int mask) { "Accepting client connection: %s", server.neterr); return; } + anetCloexec(cfd); serverLog(LL_VERBOSE,"Accepted %s:%d", cip, cport); acceptCommonHandler(connCreateAcceptedSocket(cfd),0,cip); } @@ -1124,6 +1125,7 @@ void acceptTLSHandler(aeEventLoop *el, int fd, void *privdata, int mask) { "Accepting client connection: %s", server.neterr); return; } + anetCloexec(cfd); serverLog(LL_VERBOSE,"Accepted %s:%d", cip, cport); acceptCommonHandler(connCreateAcceptedTLS(cfd, server.tls_auth_clients),0,cip); } @@ -1143,6 +1145,7 @@ void acceptUnixHandler(aeEventLoop *el, int fd, void *privdata, int mask) { "Accepting client connection: %s", server.neterr); return; } + anetCloexec(cfd); serverLog(LL_VERBOSE,"Accepted connection to %s", server.unixsocket); acceptCommonHandler(connCreateAcceptedSocket(cfd),CLIENT_UNIX_SOCKET,NULL); } diff --git a/src/sentinel.c b/src/sentinel.c index 089282b71..26c129385 100644 --- a/src/sentinel.c +++ b/src/sentinel.c @@ -2135,6 +2135,7 @@ void sentinelReconnectInstance(sentinelRedisInstance *ri) { link->cc->errstr); instanceLinkCloseConnection(link,link->cc); } else { + anetCloexec(link->cc->c.fd); link->pending_commands = 0; link->cc_conn_time = mstime(); link->cc->data = link; @@ -2162,7 +2163,7 @@ void sentinelReconnectInstance(sentinelRedisInstance *ri) { instanceLinkCloseConnection(link,link->pc); } else { int retval; - + anetCloexec(link->pc->c.fd); link->pc_conn_time = mstime(); link->pc->data = link; redisAeAttach(server.el,link->pc); diff --git a/src/server.c b/src/server.c index 90b9045ea..f3d75ce04 100644 --- a/src/server.c +++ b/src/server.c @@ -2957,6 +2957,7 @@ int listenToPort(int port, int *fds, int *count) { return C_ERR; } anetNonBlock(NULL,fds[*count]); + anetCloexec(fds[*count]); (*count)++; } return C_OK; @@ -3095,6 +3096,7 @@ void initServer(void) { exit(1); } anetNonBlock(NULL,server.sofd); + anetCloexec(server.sofd); } /* Abort if there are no listening sockets at all. */ @@ -5470,7 +5472,7 @@ void setupChildSignalHandlers(void) { * of the parent process, e.g. fd(socket or flock) etc. * should close the resources not used by the child process, so that if the * parent restarts it can bind/lock despite the child possibly still running. */ -void closeClildUnusedResourceAfterFork() { +void closeChildUnusedResourceAfterFork() { closeListeningSockets(0); if (server.cluster_enabled && server.cluster_config_file_lock_fd != -1) close(server.cluster_config_file_lock_fd); /* don't care if this fails */ @@ -5497,7 +5499,7 @@ int redisFork(int purpose) { server.in_fork_child = purpose; setOOMScoreAdj(CONFIG_OOM_BGCHILD); setupChildSignalHandlers(); - closeClildUnusedResourceAfterFork(); + closeChildUnusedResourceAfterFork(); } else { /* Parent */ server.stat_total_forks++; diff --git a/tests/instances.tcl b/tests/instances.tcl index 18eb1a402..5b25bcc97 100644 --- a/tests/instances.tcl +++ b/tests/instances.tcl @@ -400,6 +400,11 @@ proc check_leaks instance_types { # Execute all the units inside the 'tests' directory. proc run_tests {} { + set sentinel_fd_leaks_file "sentinel_fd_leaks" + if { [file exists $sentinel_fd_leaks_file] } { + file delete $sentinel_fd_leaks_file + } + set tests [lsort [glob ../tests/*]] foreach test $tests { if {$::run_matching ne {} && [string match $::run_matching $test] == 0} { @@ -414,7 +419,14 @@ proc run_tests {} { # Print a message and exists with 0 / 1 according to zero or more failures. proc end_tests {} { - if {$::failed == 0} { + set sentinel_fd_leaks_file "sentinel_fd_leaks" + if { [file exists $sentinel_fd_leaks_file] } { + puts [colorstr red "WARNING: sentinel test(s) failed, there are leaked fds in sentinel:"] + exec cat $sentinel_fd_leaks_file + exit 1 + } + + if {$::failed == 0 } { puts "GOOD! No errors." exit 0 } else { diff --git a/tests/sentinel/run.tcl b/tests/sentinel/run.tcl index 996af906a..99362c193 100644 --- a/tests/sentinel/run.tcl +++ b/tests/sentinel/run.tcl @@ -10,7 +10,7 @@ set ::tlsdir "../../tls" proc main {} { parse_options - spawn_instance sentinel $::sentinel_base_port $::instances_count + spawn_instance sentinel $::sentinel_base_port $::instances_count [list "sentinel deny-scripts-reconfig no"] spawn_instance redis $::redis_base_port $::instances_count run_tests cleanup diff --git a/tests/sentinel/tests/includes/init-tests.tcl b/tests/sentinel/tests/includes/init-tests.tcl index 234f9c589..d6796fda6 100644 --- a/tests/sentinel/tests/includes/init-tests.tcl +++ b/tests/sentinel/tests/includes/init-tests.tcl @@ -37,6 +37,8 @@ test "(init) Sentinels can start monitoring a master" { S $id SENTINEL SET mymaster down-after-milliseconds 2000 S $id SENTINEL SET mymaster failover-timeout 20000 S $id SENTINEL SET mymaster parallel-syncs 10 + S $id SENTINEL SET mymaster notification-script ../../tests/includes/notify.sh + S $id SENTINEL SET mymaster client-reconfig-script ../../tests/includes/notify.sh } } diff --git a/tests/sentinel/tests/includes/notify.sh b/tests/sentinel/tests/includes/notify.sh new file mode 100755 index 000000000..09a8addca --- /dev/null +++ b/tests/sentinel/tests/includes/notify.sh @@ -0,0 +1,20 @@ +#!/usr/bin/env bash + +OS=`uname -s` +if [ ${OS} == "Linux" ] +then + exit 0 +fi + +# fd 3 is meant to catch the actual access to /proc/pid/fd, +# in case there's an fd leak by the sentinel, +# it can take 3, but then the access to /proc will take another fd, and we'll catch that. +leaked_fd_count=`ls /proc/self/fd | grep -vE '^[0|1|2|3]$' | wc -l` +if [ $leaked_fd_count -gt 0 ] +then + sentinel_fd_leaks_file="../sentinel_fd_leaks" + if [ ! -f $sentinel_fd_leaks_file ] + then + ls -l /proc/self/fd | cat >> $sentinel_fd_leaks_file + fi +fi