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

David Gibson

14 Jul 2026 14 Jul '26

11:29 a.m.

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

Show replies by date

David Gibson

14 Jul 14 Jul

11:29 a.m.

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

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 --- 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 #include -#include #include #include #include @@ -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

David Gibson

11:29 a.m.

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

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 --- 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 #include -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

David Gibson

11:29 a.m.

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

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 --- 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 #include +#include #include #include #include @@ -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 #include #include -#include #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 #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

David Gibson

11:29 a.m.

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

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 --- 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

David Gibson

11:29 a.m.

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

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 --- 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

Stefano Brivio

15 Jul 15 Jul

9:12 a.m.

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

On Tue, 14 Jul 2026 19:29:21 +1000 David Gibson 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

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

Stefano Brivio

9:12 a.m.

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

On Tue, 14 Jul 2026 19:29:22 +1000 David Gibson 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 --- 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

Stefano Brivio

6:39 p.m.

On Tue, 14 Jul 2026 19:29:20 +1000 David Gibson 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

David Gibson

16 Jul 16 Jul

7:25 a.m.

On Wed, Jul 15, 2026 at 06:39:47PM +0200, Stefano Brivio wrote:

...

On Tue, 14 Jul 2026 19:29:20 +1000 David Gibson 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

David Gibson

7:06 a.m.

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

On Wed, Jul 15, 2026 at 09:12:23AM +0200, Stefano Brivio wrote:

...

On Tue, 14 Jul 2026 19:29:21 +1000 David Gibson 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

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

David Gibson

7:12 a.m.

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

On Wed, Jul 15, 2026 at 09:12:35AM +0200, Stefano Brivio wrote:

...

On Tue, 14 Jul 2026 19:29:22 +1000 David Gibson 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 --- 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

Stefano Brivio

9:22 a.m.

On Thu, 16 Jul 2026 15:25:08 +1000 David Gibson 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 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

David Gibson

9:49 a.m.

On Thu, Jul 16, 2026 at 09:22:30AM +0200, Stefano Brivio wrote:

...

On Thu, 16 Jul 2026 15:25:08 +1000 David Gibson 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 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?

Oh, I see. Am I correct in thinking that those lavels are attached based on what the descriptor actually refers to, not based on the fd number? If so, we don't call dup2() on console files. We use it for 1) Moving the --fd descriptor out of the 0-2 range, so called on the --fd socket. 2) For filling up any empty slots in 0-2, so targeting a /dev/null handle. In short, we never move the standard streams, we just move things around them. Unless someone passes in an actual console descriptor with --fd, in which case SELinux blowing us up sounds like the correct outcome.

...

...
...
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).

Yeah, ok, let me revise that: I think it's a nice idea to catch an oops by a developer. I'm much less comfortable relying on only that to ensure we never access 0-2 assuming they're standard streams.

...

...
...
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

-- 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

Stefano Brivio

10:02 a.m.

On Thu, 16 Jul 2026 17:49:16 +1000 David Gibson wrote:

...

On Thu, Jul 16, 2026 at 09:22:30AM +0200, Stefano Brivio wrote:

...
On Thu, 16 Jul 2026 15:25:08 +1000 David Gibson 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 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?

Oh, I see. Am I correct in thinking that those lavels are attached based on what the descriptor actually refers to, not based on the fd number?

Yes.

...

If so, we don't call dup2() on console files.

Ah, oops, right, I mixed things up.

...

We use it for 1) Moving the --fd descriptor out of the 0-2 range, so called on the --fd socket. 2) For filling up any empty slots in 0-2, so targeting a /dev/null handle. In short, we never move the standard streams, we just move things around them.

Unless someone passes in an actual console descriptor with --fd, in which case SELinux blowing us up sounds like the correct outcome.

Right, good. I'll give things a quick try anyway once you re-spin. -- Stefano

Age (days ago)

Last active (days ago)

List overview

Download

14 comments

2 participants

participants (2)

David Gibson
Stefano Brivio

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

tags

participants (2)