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

[David Gibson <david@gibson.dropbear.id.au>]

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