*: 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`.

Author: ZmnSCPxj

common/dev_disconnect.c

@@ -1,4 +1,5 @@
 #include "config.h"
+#include <ccan/closefrom/closefrom.h>
 #include <ccan/err/err.h>
 #include <common/dev_disconnect.h>
 #include <common/status.h>
@@ -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 (;;) {

lightningd/lightningd.c

@@ -39,6 +39,7 @@
  * in detail below.
  */
 #include <ccan/array_size/array_size.h>
+#include <ccan/closefrom/closefrom.h>
 #include <ccan/opt/opt.h>
 #include <ccan/pipecmd/pipecmd.h>
 #include <ccan/read_write_all/read_write_all.h>
@@ -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");
 	}

lightningd/subd.c

@@ -1,3 +1,4 @@
+#include <ccan/closefrom/closefrom.h>
 #include <ccan/err/err.h>
 #include <ccan/io/fdpass/fdpass.h>
 #include <ccan/mem/mem.h>
@@ -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);