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

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 16 Jul 2026 17:49:16 +1000
David Gibson <david@gibson.dropbear.id.au> 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 <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?  
> 
> 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