From 5356267f158bbcf3d6b83c287a434bb3b32d6c8e Mon Sep 17 00:00:00 2001 From: ZmnSCPxj jxPCSnmZ Date: Tue, 19 Oct 2021 11:35:44 +0800 Subject: [PATCH] *: Use new closefrom module from ccan. This also inadvertently fixes a latent bug: before this patch, in the `subd` function in `lightningd/subd.c`, we would close `execfail[1]` *before* doing an `exec`. We use an EOF on `execfail[1]` as a signal that `exec` succeeded (the fd is marked CLOEXEC), and otherwise use it to pump `errno` to the parent. The intent is that this fd should be kept open until `exec`, at which point CLOEXEC triggers and close that fd and sends the EOF, *or* if `exec` fails we can send the `errno` to the parent process vua that pipe-end. However, in the previous version, we end up closing that fd *before* reaching `exec`, either in the loop which `dup2`s passed-in fds (by overwriting `execfail[1]` with a `dup2`) or in the "close everything" loop, which does not guard against `execfail[1]`, only `dev_disconnect_fd`. --- common/dev_disconnect.c | 20 +++++++++- lightningd/lightningd.c | 7 +--- lightningd/subd.c | 88 +++++++++++++++++++++++++++++++++++------ 3 files changed, 95 insertions(+), 20 deletions(-) diff --git a/common/dev_disconnect.c b/common/dev_disconnect.c index 045005474..bb2453c30 100644 --- a/common/dev_disconnect.c +++ b/common/dev_disconnect.c @@ -1,4 +1,5 @@ #include "config.h" +#include #include #include #include @@ -122,6 +123,8 @@ void dev_blackhole_fd(int fd) int i; struct stat st; + int maxfd; + if (socketpair(AF_UNIX, SOCK_STREAM, 0, fds) != 0) err(1, "dev_blackhole_fd: creating socketpair"); @@ -130,12 +133,25 @@ void dev_blackhole_fd(int fd) err(1, "dev_blackhole_fd: forking"); case 0: /* Close everything but the dev_disconnect_fd, the socket - * which is pretending to be the peer, and stderr. */ - for (i = 0; i < sysconf(_SC_OPEN_MAX); i++) + * which is pretending to be the peer, and stderr. + * The "correct" way to do this would be to move the + * fds we want to preserve to the low end (0, 1, 2...) + * of the fd space and then just do a single closefrom + * call, but dup2 could fail with ENFILE (which is a + * *system*-level error, i.e. the entire system has too + * many processes with open files) and we have no + * convenient way to inform the parent of the error. + * So loop until we reach whichever is higher of fds[0] + * or dev_disconnect_fd, and *then* closefrom after that. + */ + maxfd = (fds[0] > dev_disconnect_fd) ? fds[0] : + dev_disconnect_fd ; + for (i = 0; i < maxfd; i++) if (i != fds[0] && i != dev_disconnect_fd && i != STDERR_FILENO) close(i); + closefrom(maxfd + 1); /* Close once dev_disconnect file is truncated. */ for (;;) { diff --git a/lightningd/lightningd.c b/lightningd/lightningd.c index 878856c94..67f35133c 100644 --- a/lightningd/lightningd.c +++ b/lightningd/lightningd.c @@ -39,6 +39,7 @@ * in detail below. */ #include +#include #include #include #include @@ -1182,15 +1183,11 @@ int main(int argc, char *argv[]) /* Were we supposed to restart ourselves? */ if (try_reexec) { - long max_fd; - /* Give a reasonable chance for the install to finish. */ sleep(5); /* Close all filedescriptors except stdin/stdout/stderr */ - max_fd = sysconf(_SC_OPEN_MAX); - for (int i = STDERR_FILENO+1; i < max_fd; i++) - close(i); + closefrom(STDERR_FILENO + 1); execv(orig_argv[0], orig_argv); err(1, "Failed to re-exec ourselves after version change"); } diff --git a/lightningd/subd.c b/lightningd/subd.c index 765367bdb..517ad5b70 100644 --- a/lightningd/subd.c +++ b/lightningd/subd.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -21,12 +22,53 @@ static bool move_fd(int from, int to) { assert(from >= 0); + + /* dup2 with same arguments may be a no-op, but + * the later close would make the fd invalid. + * Handle this edge case. + */ + if (from == to) + return true; + if (dup2(from, to) == -1) return false; + + /* dup2 does not duplicate flags, copy it here. + * This should be benign; the only POSIX-defined + * flag is FD_CLOEXEC, and we only use it rarely. + */ + if (fcntl(to, F_SETFD, fcntl(from, F_GETFD)) < 0) + return false; + close(from); return true; } +/* Like the above, but move the fd from whatever it currently has + * to any other unused fd number that is *not* its current value. + */ +static bool move_fd_any(int *fd) +{ + int old_fd = *fd; + int new_fd; + + assert(old_fd >= 0); + + if ((new_fd = dup(old_fd)) == -1) + return false; + + /* dup does not duplicate flags. */ + if (fcntl(new_fd, F_SETFD, fcntl(old_fd, F_GETFD)) < 0) + return false; + + close(old_fd); + + *fd = new_fd; + + assert(old_fd != *fd); + return true; +} + struct subd_req { struct list_node list; @@ -149,8 +191,7 @@ static int subd(const char *path, const char *name, goto close_execfail_fail; if (childpid == 0) { - int fdnum = 3, i, stdin_is_now = STDIN_FILENO; - long max; + int fdnum = 3, stdin_is_now = STDIN_FILENO; size_t num_args; char *args[] = { NULL, NULL, NULL, NULL, NULL }; @@ -165,29 +206,50 @@ static int subd(const char *path, const char *name, goto child_errno_fail; } - // Move dev_disconnect_fd out the way. - if (dev_disconnect_fd != -1) { - if (!move_fd(dev_disconnect_fd, 101)) - goto child_errno_fail; - dev_disconnect_fd = 101; - } - /* Dup any extra fds up first. */ while ((fd = va_arg(*ap, int *)) != NULL) { int actual_fd = *fd; /* If this were stdin, we moved it above! */ if (actual_fd == STDIN_FILENO) actual_fd = stdin_is_now; + + /* If we would overwrite important fds, move those. */ + if (fdnum == dev_disconnect_fd) { + if (!move_fd_any(&dev_disconnect_fd)) + goto child_errno_fail; + } + if (fdnum == execfail[1]) { + if (!move_fd_any(&execfail[1])) + goto child_errno_fail; + } + if (!move_fd(actual_fd, fdnum)) goto child_errno_fail; fdnum++; } + /* Move dev_disconnect_fd *after* the extra fds above. */ + if (dev_disconnect_fd != -1) { + /* Do not overwrite execfail[1]. */ + if (fdnum == execfail[1]) { + if (!move_fd_any(&execfail[1])) + goto child_errno_fail; + } + if (!move_fd(dev_disconnect_fd, fdnum)) + goto child_errno_fail; + dev_disconnect_fd = fdnum; + fdnum++; + } + + /* Move execfail[1] *after* the fds we will pass + * to the subdaemon. */ + if (!move_fd(execfail[1], fdnum)) + goto child_errno_fail; + execfail[1] = fdnum; + fdnum++; + /* Make (fairly!) sure all other fds are closed. */ - max = sysconf(_SC_OPEN_MAX); - for (i = fdnum; i < max; i++) - if (i != dev_disconnect_fd) - close(i); + closefrom(fdnum); num_args = 0; args[num_args++] = tal_strdup(NULL, path);