[PATCH 0/6] Fix bug 215 and some related issues with fd handling

[PATCH 0/6] Fix bug 215 and some related issues with fd handling

[David Gibson]

Stefano, you called it correctly.  While working on bug 215, as usual,
I found a bunch of adjacent things to clean up.

I did start by attempting the appoach you suggested for bug 215 -
remembering which of the low fds were standard streams and avoiding
closing them in __daemon().  It is indeed shorter, but only by 1-2
lines.  Looking at possible interactions with other things, I became
more and more convinced that leaving anything other than the standard
streams in fds 0-2 was an accident waiting to happen.  Much of the
rest of the series is, for example, dealing with the possibility of
--fd [012].

David Gibson (6):
  isolation, util: Fold close_open_files() into isolate_initial()
  isolation, conf: Set c->fd_tap from eary parse of --fd
  conf: Make conf_tap_fd() operate more like conf_mode()
  isolation: Move --fd descriptor to a number of our choosing
  main: Ensure fds 0-2 are populated
  passt: *Always* close pidfile_fd, not just when daemonizing

 conf.c      | 31 +++++++++++++++++++++++--------
 conf.h      |  2 +-
 isolation.c | 32 ++++++++++++++++++++++++++++++--
 isolation.h |  2 +-
 passt.c     | 27 +++++++++++++++++----------
 util.c      | 52 +---------------------------------------------------
 util.h      |  1 -
 7 files changed, 73 insertions(+), 74 deletions(-)

-- 
2.55.0

[PATCH 1/6] isolation, util: Fold close_open_files() into isolate_initial()

[David Gibson]

Most functions in util.c are, well, utilities, that are useful in a bunch
of places.  close_open_files(), however, is very specific, it's only called
from isolate_initial(), and really only can be called from there - it's
logic encodes a very specific action we want to take at that point.

So, remove it from util.c and fold into isolate_initial().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 isolation.c | 37 ++++++++++++++++++++++++++++++++++++-
 util.c      | 49 -------------------------------------------------
 util.h      |  1 -
 3 files changed, 36 insertions(+), 51 deletions(-)

diff --git a/isolation.c b/isolation.c
index c868e668..07b281bb 100644
--- a/isolation.c
+++ b/isolation.c
@@ -58,6 +58,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <getopt.h>
 #include <grp.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -88,6 +89,7 @@
 #include "passt.h"
 #include "log.h"
 #include "isolation.h"
+#include "conf.h"
 
 #define CAP_VERSION	_LINUX_CAPABILITY_VERSION_3
 #define CAP_WORDS	_LINUX_CAPABILITY_U32S_3
@@ -204,6 +206,9 @@ static int move_root(void)
  */
 void isolate_initial(int argc, char **argv)
 {
+	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
+					{ 0 }, };
+	int name, rc, fd_tap = -1;
 	uint64_t keep;
 
 	/* We want to keep CAP_NET_BIND_SERVICE in the initial
@@ -244,7 +249,37 @@ void isolate_initial(int argc, char **argv)
 
 	drop_caps_ep_except(keep);
 
-	close_open_files(argc, argv);
+	do {
+		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
+
+		if (name == 'F')
+			fd_tap = conf_tap_fd(optarg);
+	} while (name != -1);
+
+	if (fd_tap == -1) {
+		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	} else if (fd_tap == STDERR_FILENO + 1) { /* Still a single range */
+		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
+	} else {
+		rc = close_range(STDERR_FILENO + 1, fd_tap - 1,
+				 CLOSE_RANGE_UNSHARE);
+		if (!rc)
+			rc = close_range(fd_tap + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	}
+
+	if (rc) {
+		if (errno == ENOSYS || errno == EINVAL) {
+			/* This probably means close_range() or the
+			 * CLOSE_RANGE_UNSHARE flag is not supported by the
+			 * kernel.  Not much we can do here except carry on and
+			 * hope for the best.
+			 */
+			warn(
+"Can't use close_range() to ensure no files leaked by parent");
+		} else {
+			die_perror("Failed to close files leaked by parent");
+		}
+	}
 }
 
 /**
diff --git a/util.c b/util.c
index 4bc5d6f8..ce5021a9 100644
--- a/util.c
+++ b/util.c
@@ -26,7 +26,6 @@
 #include <stdbool.h>
 #include <linux/errqueue.h>
 #include <linux/in6.h>
-#include <getopt.h>
 
 #include "linux_dep.h"
 #include "util.h"
@@ -38,7 +37,6 @@
 #include "epoll_ctl.h"
 #include "pasta.h"
 #include "serialise.h"
-#include "conf.h"
 #ifdef HAS_GETRANDOM
 #include <sys/random.h>
 #endif
@@ -924,53 +922,6 @@ const char *str_ee_origin(const struct sock_extended_err *ee)
 	return "<invalid>";
 }
 
-/**
- * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr
- * @argc:	Argument count
- * @argv:	Command line options, as we need to skip any file given via --fd
- */
-void close_open_files(int argc, char **argv)
-{
-	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
-					{ 0 },
-				      };
-	long fd = -1;
-	int name, rc;
-
-	do {
-		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
-
-		if (name == 'F')
-			fd = conf_tap_fd(optarg);
-	} while (name != -1);
-
-	if (fd == -1) {
-		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	} else if (fd == STDERR_FILENO + 1) { /* Still a single range */
-		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
-	} else {
-		rc = close_range(STDERR_FILENO + 1, fd - 1,
-				 CLOSE_RANGE_UNSHARE);
-		if (!rc)
-			rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	}
-
-	if (rc) {
-		if (errno == ENOSYS || errno == EINVAL) {
-			/* This probably means close_range() or the
-			 * CLOSE_RANGE_UNSHARE flag is not supported by the
-			 * kernel.  Not much we can do here except carry on and
-			 * hope for the best.
-			 */
-			warn(
-"Can't use close_range() to ensure no files leaked by parent");
-		} else {
-			die_perror("Failed to close files leaked by parent");
-		}
-	}
-
-}
-
 /**
  * snprintf_check() - snprintf() wrapper, checking for truncation and errors
  * @str:	Output buffer
diff --git a/util.h b/util.h
index 90e8a20d..2435f536 100644
--- a/util.h
+++ b/util.h
@@ -168,7 +168,6 @@ intmax_t read_file_integer(const char *path, intmax_t fallback);
 int write_remainder(int fd, const struct iovec *iov, size_t iovcnt,
 		    size_t skip, size_t length);
 int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip);
-void close_open_files(int argc, char **argv);
 bool snprintf_check(char *str, size_t size, const char *format, ...);
 long clamped_scale(long x, long y, long lo, long hi, long f);
 
-- 
2.55.0

[PATCH 2/6] isolation, conf: Set c->fd_tap from eary parse of --fd

[David Gibson]

We parse --fd twice: once in isolate_initial() just to avoid clobbering
the passed in fd.  Then we parse it "for real" in conf(), to set c->fd_tap
and other configuration variables.

Change this, so that we return the value parsed early from
isolate_initial() and set c->fd_tap from that.  This doesn't accomplish
much immediately, but will make some further cleanups possible.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c      | 2 +-
 isolation.c | 6 +++++-
 isolation.h | 2 +-
 passt.c     | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/conf.c b/conf.c
index 6d83daef..41c9d557 100644
--- a/conf.c
+++ b/conf.c
@@ -1593,7 +1593,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->fd_control_listen = c->fd_control = -1;
 			break;
 		case 'F':
-			c->fd_tap = conf_tap_fd(optarg);
+			/* --fd was parsed early and c->fd_tap set in main() */
 			c->one_off = true;
 			*c->sock_path = 0;
 			break;
diff --git a/isolation.c b/isolation.c
index 07b281bb..4bb79e34 100644
--- a/isolation.c
+++ b/isolation.c
@@ -203,8 +203,10 @@ static int move_root(void)
  *  - close all open files except for standard streams and the one from --fd
  * Mustn't:
  *  - remove filesystem access (we need to access files during setup)
+ *
+ * Return: fd number from --fd, or -1 if not specified
  */
-void isolate_initial(int argc, char **argv)
+int isolate_initial(int argc, char **argv)
 {
 	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
 					{ 0 }, };
@@ -280,6 +282,8 @@ void isolate_initial(int argc, char **argv)
 			die_perror("Failed to close files leaked by parent");
 		}
 	}
+
+	return fd_tap;
 }
 
 /**
diff --git a/isolation.h b/isolation.h
index 66b6968d..ef27c7d5 100644
--- a/isolation.h
+++ b/isolation.h
@@ -10,7 +10,7 @@
 #include <stdbool.h>
 #include <unistd.h>
 
-void isolate_initial(int argc, char **argv);
+int isolate_initial(int argc, char **argv);
 void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns,
 		  const char *userns);
 int isolate_prefork(const struct ctx *c);
diff --git a/passt.c b/passt.c
index 65a07d72..b99e5998 100644
--- a/passt.c
+++ b/passt.c
@@ -341,7 +341,7 @@ int main(int argc, char **argv)
 
 	arch_avx2_exec(argv);
 
-	isolate_initial(argc, argv);
+	c->fd_tap = isolate_initial(argc, argv);
 
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
-- 
2.55.0

[PATCH 3/6] conf: Make conf_tap_fd() operate more like conf_mode()

[David Gibson]

We have two cases where we need to parse specific options early:
conf_tap_fd() and conf_mode().  conf_tap_fd() has a slightly odd interface,
requiring the caller to use getopt_long() to find the right option, then
pass it in.  Alter it to work like conf_mode() instead, where all the
command line parsing logic is contained within the conf.c function.

This is slightly more lines, but has a clearer division of responsibility.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c      | 25 +++++++++++++++++++++----
 conf.h      |  2 +-
 isolation.c | 12 ++----------
 3 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/conf.c b/conf.c
index 41c9d557..f0bce41b 100644
--- a/conf.c
+++ b/conf.c
@@ -1153,17 +1153,34 @@ static void conf_sock_listen(const struct ctx *c)
 
 /**
  * conf_tap_fd() - Read tap fd as supplied by -F command line option
- * @arg:	Argument to -F command line option
+ * @argc:	Argument count
+ * @argv:	Command line options
+ *
+ * Return: fd number from --fd option, or -1 if not supplied
  */
-int conf_tap_fd(const char *arg)
+int conf_tap_fd(int argc, char **argv)
 {
-	const char *p = arg;
+	const struct option optfd[] = {	{ "fd", required_argument, NULL, 'F' },
+					{ 0 }, };
+	const char *fdarg = NULL, *p;
 	unsigned long val;
+	int name;
+
+	optind = 0;
+	do {
+		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
+		if (name == 'F')
+			fdarg = optarg;
+	} while (name != -1);
+
+	if (!fdarg)
+		return -1;
 
+	p = fdarg;
 	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p)	||
 	    val > INT_MAX					||
 	    (val != STDIN_FILENO && val <= STDERR_FILENO))
-		die("Invalid --fd: %s", arg);
+		die("Invalid --fd: %s", fdarg);
 
 	return val;
 }
diff --git a/conf.h b/conf.h
index 1fa1280e..19bf9bc5 100644
--- a/conf.h
+++ b/conf.h
@@ -7,7 +7,7 @@
 #define CONF_H
 
 enum passt_modes conf_mode(int argc, char *argv[]);
-int conf_tap_fd(const char *arg);
+int conf_tap_fd(int argc, char **argv);
 void conf(struct ctx *c, int argc, char **argv);
 void conf_listen_handler(struct ctx *c, uint32_t events);
 void conf_handler(struct ctx *c, uint32_t events);
diff --git a/isolation.c b/isolation.c
index 4bb79e34..c4d476d6 100644
--- a/isolation.c
+++ b/isolation.c
@@ -58,7 +58,6 @@
 
 #include <errno.h>
 #include <fcntl.h>
-#include <getopt.h>
 #include <grp.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -208,9 +207,7 @@ static int move_root(void)
  */
 int isolate_initial(int argc, char **argv)
 {
-	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
-					{ 0 }, };
-	int name, rc, fd_tap = -1;
+	int rc, fd_tap = -1;
 	uint64_t keep;
 
 	/* We want to keep CAP_NET_BIND_SERVICE in the initial
@@ -251,12 +248,7 @@ int isolate_initial(int argc, char **argv)
 
 	drop_caps_ep_except(keep);
 
-	do {
-		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
-
-		if (name == 'F')
-			fd_tap = conf_tap_fd(optarg);
-	} while (name != -1);
+	fd_tap = conf_tap_fd(argc, argv);
 
 	if (fd_tap == -1) {
 		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-- 
2.55.0

[PATCH 4/6] isolation: Move --fd descriptor to a number of our choosing

[David Gibson]

Some users of passt pass an fd for the tap interface in, with the --fd
parameter, rather than having passt open it itself.  This requires some
slightly fiddly logic in isolate_initial() so we don't close() it along
with any other file descriptors leaked into us by hte parent.

More importantly, however, this is broken if the passed fd is 0, 1 or 2,
since in that case we will assume it's a standard stream and close it in
__daemon().  We explicitly disallow 1 or 2 in conf_tap_fd(), but 0 has been
permitted since aa1cc8922 ("conf: allow --fd 0").  It looks like the use
case of the contributor of that patch didn't involve daemonizing passt.

To fix this, use dup2() to move to the passed fd to 3.  This removes the
possibility of mixing it up with a standard stream, and as a bonus makes
the close_range() logic much simpler.  With isolate_initial() made safe
for --fd 1 and --fd 2, we can remove the logic excluding those from
conf_fd_tap().

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c      |  4 +---
 isolation.c | 21 +++++++++------------
 2 files changed, 10 insertions(+), 15 deletions(-)

diff --git a/conf.c b/conf.c
index f0bce41b..67f0db6e 100644
--- a/conf.c
+++ b/conf.c
@@ -1177,9 +1177,7 @@ int conf_tap_fd(int argc, char **argv)
 		return -1;
 
 	p = fdarg;
-	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p)	||
-	    val > INT_MAX					||
-	    (val != STDIN_FILENO && val <= STDERR_FILENO))
+	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) || val > INT_MAX)
 		die("Invalid --fd: %s", fdarg);
 
 	return val;
diff --git a/isolation.c b/isolation.c
index c4d476d6..1a7b8527 100644
--- a/isolation.c
+++ b/isolation.c
@@ -207,7 +207,7 @@ static int move_root(void)
  */
 int isolate_initial(int argc, char **argv)
 {
-	int rc, fd_tap = -1;
+	int fd_tap = -1, close_from = STDERR_FILENO + 1;
 	uint64_t keep;
 
 	/* We want to keep CAP_NET_BIND_SERVICE in the initial
@@ -249,19 +249,16 @@ int isolate_initial(int argc, char **argv)
 	drop_caps_ep_except(keep);
 
 	fd_tap = conf_tap_fd(argc, argv);
-
-	if (fd_tap == -1) {
-		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	} else if (fd_tap == STDERR_FILENO + 1) { /* Still a single range */
-		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
-	} else {
-		rc = close_range(STDERR_FILENO + 1, fd_tap - 1,
-				 CLOSE_RANGE_UNSHARE);
-		if (!rc)
-			rc = close_range(fd_tap + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	if (fd_tap >= 0) {
+		/* Move the passed fd to a more convenient location */
+		if (fd_tap != close_from			&&
+		    (dup2(fd_tap, close_from) != close_from	||
+		     close(fd_tap)))
+			die_perror("Could not move --fd descriptor");
+		fd_tap = close_from++;
 	}
 
-	if (rc) {
+	if (close_range(close_from, ~0U, CLOSE_RANGE_UNSHARE)) {
 		if (errno == ENOSYS || errno == EINVAL) {
 			/* This probably means close_range() or the
 			 * CLOSE_RANGE_UNSHARE flag is not supported by the
-- 
2.55.0

[PATCH 5/6] main: Ensure fds 0-2 are populated

[David Gibson]

Usually fds 0-2 are stdin, stdout and stderr.  However, certain things
which might invoke passt can close some of those standard fds.  If so,
anything we open might be placed in one of the standard slots.

For the handful of things we open early enough, this can be a problem
because we close fds 0-2 in __daemon(), replacing them with dupes of
/dev/null.

We could avoid closing those fds in __daemon() if they're not standard
streams.  However, leaving things other than the standard streams in fds
0-2 is a footgun: a stray printf() that occurs in a circumstance it
shouldn't could send harmful garbage to a device or socket.  It's also
likely to be confusing if debugging with strace or similar.

To avoid this, fill any missing standard streams with a dupe of /dev/null,
right after isolate_initial().  Since open()ing /dev/null itself could
land in one of those fd 0-2 slots, we need to be careful when we close it
not to leave a new gap.

Link: https://bugs.passt.top/show_bug.cgi?id=215
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt.c | 19 +++++++++++++------
 util.c  |  3 +--
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/passt.c b/passt.c
index b99e5998..663a1005 100644
--- a/passt.c
+++ b/passt.c
@@ -330,8 +330,8 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
 int main(int argc, char **argv)
 {
 	struct epoll_event events[NUM_EPOLL_EVENTS];
+	int nfds, devnull_fd = -1, fd;
 	struct ctx *c = &passt_ctx;
-	int nfds, devnull_fd = -1;
 	struct rlimit limit;
 	struct timespec now;
 	struct sigaction sa;
@@ -343,6 +343,15 @@ int main(int argc, char **argv)
 
 	c->fd_tap = isolate_initial(argc, argv);
 
+	if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0)
+		die_perror("Failed to open /dev/null");
+	/* Ensure fds 0-2 are populated */
+	for (fd = 0; fd <= STDERR_FILENO; fd++) {
+		if (fcntl(fd, F_GETFD) < 0 &&
+		    dup2(devnull_fd, fd) < 0)
+			die_perror("Failed to populate fd %d", fd);
+	}
+
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
 	sa.sa_handler = exit_handler;
@@ -410,11 +419,6 @@ int main(int argc, char **argv)
 	fwd_neigh_table_init(c);
 	nl_neigh_notify_init(c);
 
-	if (!c->foreground) {
-		if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0)
-			die_perror("Failed to open /dev/null");
-	}
-
 	if (isolate_prefork(c))
 		die("Failed to sandbox process, exiting");
 
@@ -428,6 +432,9 @@ int main(int argc, char **argv)
 		c->pidfile_fd = -1;
 	}
 
+	if (devnull_fd > STDERR_FILENO)
+		close(devnull_fd);
+
 	if (pasta_child_pid) {
 		kill(pasta_child_pid, SIGUSR1);
 		log_stderr = false;
diff --git a/util.c b/util.c
index ce5021a9..bd4c6caa 100644
--- a/util.c
+++ b/util.c
@@ -522,8 +522,7 @@ int __daemon(int pidfile_fd, int devnull_fd)
 	if (setsid()				< 0 ||
 	    dup2(devnull_fd, STDIN_FILENO)	< 0 ||
 	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
-	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
-	    close(devnull_fd))
+	    dup2(devnull_fd, STDERR_FILENO)	< 0)
 		passt_exit(EXIT_FAILURE);
 
 	return 0;
-- 
2.55.0

[PATCH 6/6] passt: *Always* close pidfile_fd, not just when daemonizing

[David Gibson]

In order to satisfy static checkers that we don't have an fd leak,
a9c61ffaf153 added a close() of c->pidfile_fd, amongst others.  However,
it only close()s it in the case where we daemonize into the background.
While less universally useful in the foreground / non-daemon case, it's
perfectly reasonable to still have a pidfile.  We'll still write it, and
we should still close it.

Fixes: a9c61ffaf153 ("util, passt: Close daemon-lifetime fds on exit to avoid Coverity warning")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/passt.c b/passt.c
index 663a1005..4b7c8434 100644
--- a/passt.c
+++ b/passt.c
@@ -424,14 +424,14 @@ int main(int argc, char **argv)
 
 	if (!c->foreground) {
 		__daemon(c->pidfile_fd, devnull_fd);
-		close(c->pidfile_fd);
-		c->pidfile_fd = -1;
 		log_stderr = false;
 	} else {
 		pidfile_write(c->pidfile_fd, getpid());
-		c->pidfile_fd = -1;
 	}
 
+	close(c->pidfile_fd);
+	c->pidfile_fd = -1;
+
 	if (devnull_fd > STDERR_FILENO)
 		close(devnull_fd);
 
-- 
2.55.0

Re: [PATCH 1/6] isolation, util: Fold close_open_files() into isolate_initial()

[Stefano Brivio]

On Tue, 14 Jul 2026 19:29:21 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Most functions in util.c are, well, utilities, that are useful in a bunch
> of places.  close_open_files(), however, is very specific, it's only called
> from isolate_initial(), and really only can be called from there - it's

Nit: its

> logic encodes a very specific action we want to take at that point.
> 
> So, remove it from util.c and fold into isolate_initial().

To me this doesn't actually look like an improvement (well, of course,
otherwise I would have structured commit 09603cab28f9 ("passt, util:
Close any open file that the parent might have leaked") differently)
exactly because close_open_files() is very specific and does exactly
that one thing, contributing to a shorter version of isolate_initial().

Now, I realise that we're pretty far from "fixing"
https://bugs.passt.top/show_bug.cgi?id=31, but still,
isolate_initial() in its current form looks much more digestible than
a version with thirty-three lines added on top. It currently has a
specific role and that's conceptually a separated operation,
regardless of the fact it has a single caller.

On top of that, the function comment to close_open_files() disappears
this way, because it has no natural place anymore. But I think that
was rather useful.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

By the way, should you respin, you could probably Cc: Ammar as he
volunteered to review this kind of series. I would also suggest Cc'ing
reporters (and marking them as such when doable).

-- 
Stefano

Re: [PATCH 2/6] isolation, conf: Set c->fd_tap from eary parse of --fd

[Stefano Brivio]

On Tue, 14 Jul 2026 19:29:22 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> We parse --fd twice: once in isolate_initial() just to avoid clobbering
> the passed in fd.  Then we parse it "for real" in conf(), to set c->fd_tap
> and other configuration variables.
> 
> Change this, so that we return the value parsed early from
> isolate_initial() and set c->fd_tap from that.  This doesn't accomplish
> much immediately, but will make some further cleanups possible.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  conf.c      | 2 +-
>  isolation.c | 6 +++++-
>  isolation.h | 2 +-
>  passt.c     | 2 +-
>  4 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index 6d83daef..41c9d557 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -1593,7 +1593,7 @@ void conf(struct ctx *c, int argc, char **argv)
>  			c->fd_control_listen = c->fd_control = -1;
>  			break;
>  		case 'F':
> -			c->fd_tap = conf_tap_fd(optarg);
> +			/* --fd was parsed early and c->fd_tap set in main() */
>  			c->one_off = true;
>  			*c->sock_path = 0;
>  			break;
> diff --git a/isolation.c b/isolation.c
> index 07b281bb..4bb79e34 100644
> --- a/isolation.c
> +++ b/isolation.c
> @@ -203,8 +203,10 @@ static int move_root(void)
>   *  - close all open files except for standard streams and the one from --fd
>   * Mustn't:
>   *  - remove filesystem access (we need to access files during setup)
> + *
> + * Return: fd number from --fd, or -1 if not specified

I think this is a rather weird value to return for a function that's
called isolate_initial(). Are patches 1/2 and 2/2 really needed for the
rest of the series? I don't quite see the connection.

I'm still reviewing the rest of the series.

-- 
Stefano

Re: [PATCH 0/6] Fix bug 215 and some related issues with fd handling

[Stefano Brivio]

On Tue, 14 Jul 2026 19:29:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Stefano, you called it correctly.  While working on bug 215, as usual,
> I found a bunch of adjacent things to clean up.

So, I finally finished reviewing the series.

Other than 1/6 and 2/6 on which I already commented (long story short,
I think we should avoid 1/6, and about 2/6, it would be nice to parse -F
just once in general but I think we shouldn't "force" it like that...
maybe just parse / get it outside conf()?), I don't see any substantial
issue with the other patches, but I have some general comments about the
approach.

As a detail, though, I would recommend Cc'ing everybody who might be
interested or affected by this (reporter of bug #215, Rich as he fixed
the original https://github.com/libguestfs/libguestfs/issues/360, and
Alyssa as author of aa1cc8922867 ("conf: allow --fd 0").

> I did start by attempting the appoach you suggested for bug 215 -
> remembering which of the low fds were standard streams and avoiding
> closing them in __daemon().  It is indeed shorter, but only by 1-2
> lines.  Looking at possible interactions with other things, I became
> more and more convinced that leaving anything other than the standard
> streams in fds 0-2 was an accident waiting to happen.

It did actually happen, see c66f0341d94d ("log: Don't report syslog
failures to stderr after initialisation"). I didn't consider that,
and it's indeed a strong argument in favour of this approach.

I still have some remarks and doubts about it though:

1. we might have users passing a given file descriptor with the
   expectation that it won't change its number as seen from procfs
   (and dup2() changes that, right?), even just for debugging, and 4/6
   breaks that. It's not a very legitimate expectation maybe but it
   might be one, we simply don't know

2. the reason behind my proposal (check if file descriptors are open
   when we start and avoid closing them) was simplicity and avoiding
   the risk of a number of side effects (more below).

   Yes, it's just a bit shorter (depending on how we count), but this
   diff (build tested only) should be equivalent to patches 4/6 and 5/6,
   which look considerably more complicated to me (even though the
   simplification in close_open_files() is significant... but we could
   get the same outcome also with just 4/6):

---
diff --git a/passt.c b/passt.c
index 65a07d7..e2ea613 100644
--- a/passt.c
+++ b/passt.c
@@ -330,8 +330,9 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
 int main(int argc, char **argv)
 {
 	struct epoll_event events[NUM_EPOLL_EVENTS];
+	bool close_low_fd[STDERR_FILENO + 1];
+	int nfds, devnull_fd = -1, i;
 	struct ctx *c = &passt_ctx;
-	int nfds, devnull_fd = -1;
 	struct rlimit limit;
 	struct timespec now;
 	struct sigaction sa;
@@ -339,6 +340,9 @@ int main(int argc, char **argv)
 	if (clock_gettime(CLOCK_MONOTONIC, &log_start))
 		die_perror("Failed to get CLOCK_MONOTONIC time");
 
+	for (i = STDIN_FILENO; i <= STDERR_FILENO; i++)
+		close_low_fd[i] = !fcntl(i, F_GETFD);
+
 	arch_avx2_exec(argv);
 
 	isolate_initial(argc, argv);
@@ -419,7 +423,10 @@ int main(int argc, char **argv)
 		die("Failed to sandbox process, exiting");
 
 	if (!c->foreground) {
-		__daemon(c->pidfile_fd, devnull_fd);
+		if (c->fd_tap != -1 && c->fd_tap < STDERR_FILENO)
+			close_low_fd[c->fd_tap] = false;
+
+		__daemon(close_low_fd, c->pidfile_fd, devnull_fd);
 		close(c->pidfile_fd);
 		c->pidfile_fd = -1;
 		log_stderr = false;
diff --git a/util.c b/util.c
index 4bc5d6f..57d42e1 100644
--- a/util.c
+++ b/util.c
@@ -501,13 +501,14 @@ int output_file_open(const char *path, int flags)
 
 /**
  * __daemon() - daemon()-like function writing PID file before parent exits
+ * @close_fd:	Standard stream descriptors numbers to close
  * @pidfile_fd:	Open PID file descriptor
  * @devnull_fd:	Open file descriptor for /dev/null
  *
  * Return: 0 in the child process on success. The parent process exits.
  * 	   Does not return in either process on failure (calls _exit).
  */
-int __daemon(int pidfile_fd, int devnull_fd)
+int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd)
 {
 	pid_t pid = fork();
 
@@ -522,9 +523,9 @@ int __daemon(int pidfile_fd, int devnull_fd)
 	}
 
 	if (setsid()				< 0 ||
-	    dup2(devnull_fd, STDIN_FILENO)	< 0 ||
-	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
-	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
+	    (close_fd[STDIN_FILENO]  && dup2(devnull_fd, STDIN_FILENO)  < 0) ||
+	    (close_fd[STDOUT_FILENO] && dup2(devnull_fd, STDOUT_FILENO)	< 0) ||
+	    (close_fd[STDERR_FILENO] && dup2(devnull_fd, STDERR_FILENO)	< 0) ||
 	    close(devnull_fd))
 		passt_exit(EXIT_FAILURE);
 
diff --git a/util.h b/util.h
index 90e8a20..246ac67 100644
--- a/util.h
+++ b/util.h
@@ -160,7 +160,7 @@ bool ns_is_init(void);
 int open_in_ns(const struct ctx *c, const char *path, int flags);
 int output_file_open(const char *path, int flags);
 void pidfile_write(int fd, pid_t pid);
-int __daemon(int pidfile_fd, int devnull_fd);
+int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd);
 int fls(unsigned long x);
 int ilog2(unsigned long x);
 int write_file(const char *path, const char *buf);
---

3. I have a generic worry that LSMs might get in the way. This would be solved
   by testing your series against current AppArmor and SELinux policies but I
   didn't get the chance yet (it would be nice if you could...)

4. if the concern is a misused fprintf() or printf(), shouldn't we prevent
   direct usage anyway with, say:

#define printf(x) @ "Don't call printf() directly, use err() / warn() / debug() etc."

   and similar for fprintf(), that could only be called directly from
   FPRINTF() and wherever we really need it? At that point I'm not sure
   we would have any remaining concern about risks of using standard
   streams by mistake

5. assuming we go with both 4/6 and 5/6: can we finally assume that sockets
   will never be numbered 0 and save a lot of initialisations to -1 and
   related checks, at that point? If we can achieve that as side effect,
   that would be another argument in favour of it in my eyes

> Much of the
> rest of the series is, for example, dealing with the possibility of
> --fd [012].

...well, yes, but the possibility of --fd [12] was introduced by the
series itself. :)

Anyway, summing up my feedback, *maybe* other advantages outweigh 1. (I
haven't checked what happens in procfs though), and once we check that
3. is not a problem, I'm fine with this approach (even though still a
bit reluctant because we're adding substantial changes for a problem
that doesn't even exist anymore as it's already fixed in libguestfs).

-- 
Stefano

Re: [PATCH 1/6] isolation, util: Fold close_open_files() into isolate_initial()

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2204 bytes --]

On Wed, Jul 15, 2026 at 09:12:23AM +0200, Stefano Brivio wrote:
> On Tue, 14 Jul 2026 19:29:21 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Most functions in util.c are, well, utilities, that are useful in a bunch
> > of places.  close_open_files(), however, is very specific, it's only called
> > from isolate_initial(), and really only can be called from there - it's
> 
> Nit: its

Fixed.

> > logic encodes a very specific action we want to take at that point.
> > 
> > So, remove it from util.c and fold into isolate_initial().
> 
> To me this doesn't actually look like an improvement (well, of course,
> otherwise I would have structured commit 09603cab28f9 ("passt, util:
> Close any open file that the parent might have leaked") differently)
> exactly because close_open_files() is very specific and does exactly
> that one thing, contributing to a shorter version of isolate_initial().
> 
> Now, I realise that we're pretty far from "fixing"
> https://bugs.passt.top/show_bug.cgi?id=31, but still,
> isolate_initial() in its current form looks much more digestible than
> a version with thirty-three lines added on top. It currently has a
> specific role and that's conceptually a separated operation,
> regardless of the fact it has a single caller.

> On top of that, the function comment to close_open_files() disappears
> this way, because it has no natural place anymore. But I think that
> was rather useful.

Fair points.  I still think the case for putting this code in
isolate.c rather than util.c is strong.  How would you feel about
moving close_open_files() to isolate.c, but not folding it into
isolate_initial().  Maybe 'isolate_fds()'?

> 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> 
> By the way, should you respin, you could probably Cc: Ammar as he
> volunteered to review this kind of series. I would also suggest Cc'ing
> reporters (and marking them as such when doable).

Oh, good point.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/6] isolation, conf: Set c->fd_tap from eary parse of --fd

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2828 bytes --]

On Wed, Jul 15, 2026 at 09:12:35AM +0200, Stefano Brivio wrote:
> On Tue, 14 Jul 2026 19:29:22 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > We parse --fd twice: once in isolate_initial() just to avoid clobbering
> > the passed in fd.  Then we parse it "for real" in conf(), to set c->fd_tap
> > and other configuration variables.
> > 
> > Change this, so that we return the value parsed early from
> > isolate_initial() and set c->fd_tap from that.  This doesn't accomplish
> > much immediately, but will make some further cleanups possible.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  conf.c      | 2 +-
> >  isolation.c | 6 +++++-
> >  isolation.h | 2 +-
> >  passt.c     | 2 +-
> >  4 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index 6d83daef..41c9d557 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -1593,7 +1593,7 @@ void conf(struct ctx *c, int argc, char **argv)
> >  			c->fd_control_listen = c->fd_control = -1;
> >  			break;
> >  		case 'F':
> > -			c->fd_tap = conf_tap_fd(optarg);
> > +			/* --fd was parsed early and c->fd_tap set in main() */
> >  			c->one_off = true;
> >  			*c->sock_path = 0;
> >  			break;
> > diff --git a/isolation.c b/isolation.c
> > index 07b281bb..4bb79e34 100644
> > --- a/isolation.c
> > +++ b/isolation.c
> > @@ -203,8 +203,10 @@ static int move_root(void)
> >   *  - close all open files except for standard streams and the one from --fd
> >   * Mustn't:
> >   *  - remove filesystem access (we need to access files during setup)
> > + *
> > + * Return: fd number from --fd, or -1 if not specified
> 
> I think this is a rather weird value to return for a function that's
> called isolate_initial().

Agreed, but there are reasons..

> Are patches 1/2 and 2/2 really needed for the
> rest of the series? I don't quite see the connection.

1) (minor) The new semantics of conf_tap_fd() introduced in 3/6 work
   better (IMO) for its use in close_open_files(), but don't work for
   its reuse during the "main" arg parse.  Removing that second parse
   avoids the problem.

2) (major) 4/6 significantly simplifies the close_range() logic, but
   it involves *moving* the tap descriptor given with --fd (using
   dup2(2).  That means if we parsed it again, it would now be wrong.

I think the weirdness would also be mitigated, if close_open_files()
became isolate_fds(), and was called directly from main() rather than
via isolate_initial().
> 
> I'm still reviewing the rest of the series.
> 
> -- 
> Stefano
> 

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] Fix bug 215 and some related issues with fd handling

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8420 bytes --]

On Wed, Jul 15, 2026 at 06:39:47PM +0200, Stefano Brivio wrote:
> On Tue, 14 Jul 2026 19:29:20 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > Stefano, you called it correctly.  While working on bug 215, as usual,
> > I found a bunch of adjacent things to clean up.
> 
> So, I finally finished reviewing the series.
> 
> Other than 1/6 and 2/6 on which I already commented (long story short,
> I think we should avoid 1/6, and about 2/6, it would be nice to parse -F
> just once in general but I think we shouldn't "force" it like that...
> maybe just parse / get it outside conf()?),

Hm, I'm not entirely sure of the distinction you're drawing between
the two cases.

> I don't see any substantial
> issue with the other patches, but I have some general comments about the
> approach.
> 
> As a detail, though, I would recommend Cc'ing everybody who might be
> interested or affected by this (reporter of bug #215, Rich as he fixed
> the original https://github.com/libguestfs/libguestfs/issues/360, and
> Alyssa as author of aa1cc8922867 ("conf: allow --fd 0").

Ah, good point, I'll do that for the next spin.

> > I did start by attempting the appoach you suggested for bug 215 -
> > remembering which of the low fds were standard streams and avoiding
> > closing them in __daemon().  It is indeed shorter, but only by 1-2
> > lines.  Looking at possible interactions with other things, I became
> > more and more convinced that leaving anything other than the standard
> > streams in fds 0-2 was an accident waiting to happen.
> 
> It did actually happen, see c66f0341d94d ("log: Don't report syslog
> failures to stderr after initialisation"). I didn't consider that,
> and it's indeed a strong argument in favour of this approach.
> 
> I still have some remarks and doubts about it though:
> 
> 1. we might have users passing a given file descriptor with the
>    expectation that it won't change its number as seen from procfs
>    (and dup2() changes that, right?), even just for debugging, and 4/6
>    breaks that. It's not a very legitimate expectation maybe but it
>    might be one, we simply don't know

That... really seems like taking bug for bug compatibility too far to
me.

> 
> 2. the reason behind my proposal (check if file descriptors are open
>    when we start and avoid closing them) was simplicity and avoiding
>    the risk of a number of side effects (more below).
> 
>    Yes, it's just a bit shorter (depending on how we count), but this
>    diff (build tested only) should be equivalent to patches 4/6 and 5/6,
>    which look considerably more complicated to me (even though the
>    simplification in close_open_files() is significant... but we could
>    get the same outcome also with just 4/6):
> 
> ---
> diff --git a/passt.c b/passt.c
> index 65a07d7..e2ea613 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -330,8 +330,9 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
>  int main(int argc, char **argv)
>  {
>  	struct epoll_event events[NUM_EPOLL_EVENTS];
> +	bool close_low_fd[STDERR_FILENO + 1];
> +	int nfds, devnull_fd = -1, i;
>  	struct ctx *c = &passt_ctx;
> -	int nfds, devnull_fd = -1;
>  	struct rlimit limit;
>  	struct timespec now;
>  	struct sigaction sa;
> @@ -339,6 +340,9 @@ int main(int argc, char **argv)
>  	if (clock_gettime(CLOCK_MONOTONIC, &log_start))
>  		die_perror("Failed to get CLOCK_MONOTONIC time");
>  
> +	for (i = STDIN_FILENO; i <= STDERR_FILENO; i++)
> +		close_low_fd[i] = !fcntl(i, F_GETFD);

Nit: needs to have a >= 0, F_GETFD returns flags, not 0 on success.

> +
>  	arch_avx2_exec(argv);
>  
>  	isolate_initial(argc, argv);
> @@ -419,7 +423,10 @@ int main(int argc, char **argv)
>  		die("Failed to sandbox process, exiting");
>  
>  	if (!c->foreground) {
> -		__daemon(c->pidfile_fd, devnull_fd);
> +		if (c->fd_tap != -1 && c->fd_tap < STDERR_FILENO)
> +			close_low_fd[c->fd_tap] = false;
> +
> +		__daemon(close_low_fd, c->pidfile_fd, devnull_fd);
>  		close(c->pidfile_fd);
>  		c->pidfile_fd = -1;
>  		log_stderr = false;
> diff --git a/util.c b/util.c
> index 4bc5d6f..57d42e1 100644
> --- a/util.c
> +++ b/util.c
> @@ -501,13 +501,14 @@ int output_file_open(const char *path, int flags)
>  
>  /**
>   * __daemon() - daemon()-like function writing PID file before parent exits
> + * @close_fd:	Standard stream descriptors numbers to close
>   * @pidfile_fd:	Open PID file descriptor
>   * @devnull_fd:	Open file descriptor for /dev/null
>   *
>   * Return: 0 in the child process on success. The parent process exits.
>   * 	   Does not return in either process on failure (calls _exit).
>   */
> -int __daemon(int pidfile_fd, int devnull_fd)
> +int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd)
>  {
>  	pid_t pid = fork();
>  
> @@ -522,9 +523,9 @@ int __daemon(int pidfile_fd, int devnull_fd)
>  	}
>  
>  	if (setsid()				< 0 ||
> -	    dup2(devnull_fd, STDIN_FILENO)	< 0 ||
> -	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
> -	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
> +	    (close_fd[STDIN_FILENO]  && dup2(devnull_fd, STDIN_FILENO)  < 0) ||
> +	    (close_fd[STDOUT_FILENO] && dup2(devnull_fd, STDOUT_FILENO)	< 0) ||
> +	    (close_fd[STDERR_FILENO] && dup2(devnull_fd, STDERR_FILENO)	< 0) ||
>  	    close(devnull_fd))
>  		passt_exit(EXIT_FAILURE);
>  
> diff --git a/util.h b/util.h
> index 90e8a20..246ac67 100644
> --- a/util.h
> +++ b/util.h
> @@ -160,7 +160,7 @@ bool ns_is_init(void);
>  int open_in_ns(const struct ctx *c, const char *path, int flags);
>  int output_file_open(const char *path, int flags);
>  void pidfile_write(int fd, pid_t pid);
> -int __daemon(int pidfile_fd, int devnull_fd);
> +int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd);
>  int fls(unsigned long x);
>  int ilog2(unsigned long x);
>  int write_file(const char *path, const char *buf);
> ---

Yes, that will do the job.  I still think avoiding anything but the
standard streams in 0-2 is worthwhile.

> 3. I have a generic worry that LSMs might get in the way. This would be solved
>    by testing your series against current AppArmor and SELinux policies but I
>    didn't get the chance yet (it would be nice if you could...)

That seems really far-fetched to me.  I assume you're meaning they
would block the dup2()?  That would break nearly anything that spawns
child processes.

> 4. if the concern is a misused fprintf() or printf(), shouldn't we prevent
>    direct usage anyway with, say:
> 
> #define printf(x) @ "Don't call printf() directly, use err() / warn() / debug() etc."
> 
>    and similar for fprintf(), that could only be called directly from
>    FPRINTF() and wherever we really need it? At that point I'm not sure
>    we would have any remaining concern about risks of using standard
>    streams by mistake

I guess.  I feel like that's even uglier than what I've proposed.

> 5. assuming we go with both 4/6 and 5/6: can we finally assume that sockets
>    will never be numbered 0 and save a lot of initialisations to -1 and
>    related checks, at that point? If we can achieve that as side effect,
>    that would be another argument in favour of it in my eyes

Huh, interesting, I guess it would allow that.

> 
> > Much of the
> > rest of the series is, for example, dealing with the possibility of
> > --fd [012].
> 
> ...well, yes, but the possibility of --fd [12] was introduced by the
> series itself. :)

Well, true, but dealing with -fd [12] was basically free while dealing
with --fd 0.

> Anyway, summing up my feedback, *maybe* other advantages outweigh 1. (I
> haven't checked what happens in procfs though), and once we check that
> 3. is not a problem, I'm fine with this approach (even though still a
> bit reluctant because we're adding substantial changes for a problem
> that doesn't even exist anymore as it's already fixed in libguestfs).

Hm, ok.  Well I have some ideas that might mitigate at least some of
your concerns, so I'll apply those, respin and see what you think.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/6] Fix bug 215 and some related issues with fd handling

[Stefano Brivio]

On Thu, 16 Jul 2026 15:25:08 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 15, 2026 at 06:39:47PM +0200, Stefano Brivio wrote:
> > On Tue, 14 Jul 2026 19:29:20 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > Stefano, you called it correctly.  While working on bug 215, as usual,
> > > I found a bunch of adjacent things to clean up.  
> > 
> > So, I finally finished reviewing the series.
> > 
> > Other than 1/6 and 2/6 on which I already commented (long story short,
> > I think we should avoid 1/6, and about 2/6, it would be nice to parse -F
> > just once in general but I think we shouldn't "force" it like that...
> > maybe just parse / get it outside conf()?),  
> 
> Hm, I'm not entirely sure of the distinction you're drawing between
> the two cases.
> 
> > I don't see any substantial
> > issue with the other patches, but I have some general comments about the
> > approach.
> > 
> > As a detail, though, I would recommend Cc'ing everybody who might be
> > interested or affected by this (reporter of bug #215, Rich as he fixed
> > the original https://github.com/libguestfs/libguestfs/issues/360, and
> > Alyssa as author of aa1cc8922867 ("conf: allow --fd 0").  
> 
> Ah, good point, I'll do that for the next spin.
> 
> > > I did start by attempting the appoach you suggested for bug 215 -
> > > remembering which of the low fds were standard streams and avoiding
> > > closing them in __daemon().  It is indeed shorter, but only by 1-2
> > > lines.  Looking at possible interactions with other things, I became
> > > more and more convinced that leaving anything other than the standard
> > > streams in fds 0-2 was an accident waiting to happen.  
> > 
> > It did actually happen, see c66f0341d94d ("log: Don't report syslog
> > failures to stderr after initialisation"). I didn't consider that,
> > and it's indeed a strong argument in favour of this approach.
> > 
> > I still have some remarks and doubts about it though:
> > 
> > 1. we might have users passing a given file descriptor with the
> >    expectation that it won't change its number as seen from procfs
> >    (and dup2() changes that, right?), even just for debugging, and 4/6
> >    breaks that. It's not a very legitimate expectation maybe but it
> >    might be one, we simply don't know  
> 
> That... really seems like taking bug for bug compatibility too far to
> me.
> 
> > 
> > 2. the reason behind my proposal (check if file descriptors are open
> >    when we start and avoid closing them) was simplicity and avoiding
> >    the risk of a number of side effects (more below).
> > 
> >    Yes, it's just a bit shorter (depending on how we count), but this
> >    diff (build tested only) should be equivalent to patches 4/6 and 5/6,
> >    which look considerably more complicated to me (even though the
> >    simplification in close_open_files() is significant... but we could
> >    get the same outcome also with just 4/6):
> > 
> > ---
> > diff --git a/passt.c b/passt.c
> > index 65a07d7..e2ea613 100644
> > --- a/passt.c
> > +++ b/passt.c
> > @@ -330,8 +330,9 @@ static void passt_worker(void *opaque, int nfds, struct epoll_event *events)
> >  int main(int argc, char **argv)
> >  {
> >  	struct epoll_event events[NUM_EPOLL_EVENTS];
> > +	bool close_low_fd[STDERR_FILENO + 1];
> > +	int nfds, devnull_fd = -1, i;
> >  	struct ctx *c = &passt_ctx;
> > -	int nfds, devnull_fd = -1;
> >  	struct rlimit limit;
> >  	struct timespec now;
> >  	struct sigaction sa;
> > @@ -339,6 +340,9 @@ int main(int argc, char **argv)
> >  	if (clock_gettime(CLOCK_MONOTONIC, &log_start))
> >  		die_perror("Failed to get CLOCK_MONOTONIC time");
> >  
> > +	for (i = STDIN_FILENO; i <= STDERR_FILENO; i++)
> > +		close_low_fd[i] = !fcntl(i, F_GETFD);  
> 
> Nit: needs to have a >= 0, F_GETFD returns flags, not 0 on success.
> 
> > +
> >  	arch_avx2_exec(argv);
> >  
> >  	isolate_initial(argc, argv);
> > @@ -419,7 +423,10 @@ int main(int argc, char **argv)
> >  		die("Failed to sandbox process, exiting");
> >  
> >  	if (!c->foreground) {
> > -		__daemon(c->pidfile_fd, devnull_fd);
> > +		if (c->fd_tap != -1 && c->fd_tap < STDERR_FILENO)
> > +			close_low_fd[c->fd_tap] = false;
> > +
> > +		__daemon(close_low_fd, c->pidfile_fd, devnull_fd);
> >  		close(c->pidfile_fd);
> >  		c->pidfile_fd = -1;
> >  		log_stderr = false;
> > diff --git a/util.c b/util.c
> > index 4bc5d6f..57d42e1 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -501,13 +501,14 @@ int output_file_open(const char *path, int flags)
> >  
> >  /**
> >   * __daemon() - daemon()-like function writing PID file before parent exits
> > + * @close_fd:	Standard stream descriptors numbers to close
> >   * @pidfile_fd:	Open PID file descriptor
> >   * @devnull_fd:	Open file descriptor for /dev/null
> >   *
> >   * Return: 0 in the child process on success. The parent process exits.
> >   * 	   Does not return in either process on failure (calls _exit).
> >   */
> > -int __daemon(int pidfile_fd, int devnull_fd)
> > +int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd)
> >  {
> >  	pid_t pid = fork();
> >  
> > @@ -522,9 +523,9 @@ int __daemon(int pidfile_fd, int devnull_fd)
> >  	}
> >  
> >  	if (setsid()				< 0 ||
> > -	    dup2(devnull_fd, STDIN_FILENO)	< 0 ||
> > -	    dup2(devnull_fd, STDOUT_FILENO)	< 0 ||
> > -	    dup2(devnull_fd, STDERR_FILENO)	< 0 ||
> > +	    (close_fd[STDIN_FILENO]  && dup2(devnull_fd, STDIN_FILENO)  < 0) ||
> > +	    (close_fd[STDOUT_FILENO] && dup2(devnull_fd, STDOUT_FILENO)	< 0) ||
> > +	    (close_fd[STDERR_FILENO] && dup2(devnull_fd, STDERR_FILENO)	< 0) ||
> >  	    close(devnull_fd))
> >  		passt_exit(EXIT_FAILURE);
> >  
> > diff --git a/util.h b/util.h
> > index 90e8a20..246ac67 100644
> > --- a/util.h
> > +++ b/util.h
> > @@ -160,7 +160,7 @@ bool ns_is_init(void);
> >  int open_in_ns(const struct ctx *c, const char *path, int flags);
> >  int output_file_open(const char *path, int flags);
> >  void pidfile_write(int fd, pid_t pid);
> > -int __daemon(int pidfile_fd, int devnull_fd);
> > +int __daemon(bool close_fd[STDERR_FILENO + 1], int pidfile_fd, int devnull_fd);
> >  int fls(unsigned long x);
> >  int ilog2(unsigned long x);
> >  int write_file(const char *path, const char *buf);
> > ---  
> 
> Yes, that will do the job.  I still think avoiding anything but the
> standard streams in 0-2 is worthwhile.
> 
> > 3. I have a generic worry that LSMs might get in the way. This would be solved
> >    by testing your series against current AppArmor and SELinux policies but I
> >    didn't get the chance yet (it would be nice if you could...)  
> 
> That seems really far-fetched to me.  I assume you're meaning they
> would block the dup2()?  That would break nearly anything that spawns
> child processes.

No, not dup2() specifically. AppArmor and SELinux don't operate (in
general) on specific system calls. I'm rather thinking of some issue
manipulating file descriptors that correspond to console devices.

For example, pasta's SELinux policy currently has:

  allow pasta_t console_device_t:chr_file { open write getattr ioctl };
  allow pasta_t user_devpts_t:chr_file { getattr read write ioctl };

are those sufficient if we call dup2() on those?

> > 4. if the concern is a misused fprintf() or printf(), shouldn't we prevent
> >    direct usage anyway with, say:
> > 
> > #define printf(x) @ "Don't call printf() directly, use err() / warn() / debug() etc."
> > 
> >    and similar for fprintf(), that could only be called directly from
> >    FPRINTF() and wherever we really need it? At that point I'm not sure
> >    we would have any remaining concern about risks of using standard
> >    streams by mistake  
> 
> I guess.  I feel like that's even uglier than what I've proposed.

I was actually proposing to do that regardless of this series, it
didn't really look ugly to me (we already do that for strerror() and I
think it's quite convenient).

> > 5. assuming we go with both 4/6 and 5/6: can we finally assume that sockets
> >    will never be numbered 0 and save a lot of initialisations to -1 and
> >    related checks, at that point? If we can achieve that as side effect,
> >    that would be another argument in favour of it in my eyes  
> 
> Huh, interesting, I guess it would allow that.
> 
> >   
> > > Much of the
> > > rest of the series is, for example, dealing with the possibility of
> > > --fd [012].  
> > 
> > ...well, yes, but the possibility of --fd [12] was introduced by the
> > series itself. :)  
> 
> Well, true, but dealing with -fd [12] was basically free while dealing
> with --fd 0.
> 
> > Anyway, summing up my feedback, *maybe* other advantages outweigh 1. (I
> > haven't checked what happens in procfs though), and once we check that
> > 3. is not a problem, I'm fine with this approach (even though still a
> > bit reluctant because we're adding substantial changes for a problem
> > that doesn't even exist anymore as it's already fixed in libguestfs).  
> 
> Hm, ok.  Well I have some ideas that might mitigate at least some of
> your concerns, so I'll apply those, respin and see what you think.

Sure.

-- 
Stefano