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