From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202606 header.b=oBb+ZSTz; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 631B65A0269 for ; Thu, 16 Jul 2026 07:45:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1784180743; bh=MtpcOh+k3E+5bEzavdwHVvT26mp2UGHkV/DFKEKg0IA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=oBb+ZSTztrEzwkz+rbL34lmtzPPGbZaIcCmsCxSf3v/QTBILt6cFtgFww/pieGtsq X0bBZW2M9iqoG/PskRJnuD4n6RbIaFABgB/pBSDj3vxnuMSchqaEtd3eS8kYs5Xb/Q jugyEazLB+YK3M7tjwNqIYo3DYyiEOrgH1QeJWFLEDQL30ItKNwSelNdkkXHczBFfm 2JXFDrqYwuJmc+ybiry1EgNLXUTAI761A7h3WaTJeAlkp9EltM4GdJqkMUR0k/QWjN aheGJzNeIAO2kN1FgKVuw8t33X2kWBitU7jUnlylLwc1/nSvcWGqsWGAxPFuHjgvKP VOv3p8kXuwUhQ== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4h128675Dwz4w11; Thu, 16 Jul 2026 15:45:42 +1000 (AEST) Date: Thu, 16 Jul 2026 15:25:08 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 0/6] Fix bug 215 and some related issues with fd handling Message-ID: References: <20260714092926.2881848-1-david@gibson.dropbear.id.au> <20260715183946.6121580a@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="j40u2F4X+iA8Q01b" Content-Disposition: inline In-Reply-To: <20260715183946.6121580a@elisabeth> Message-ID-Hash: J2COYRS2NA7BRWH2E633XSNJVANGOFVG X-Message-ID-Hash: J2COYRS2NA7BRWH2E633XSNJVANGOFVG X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --j40u2F4X+iA8Q01b Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > Stefano, you called it correctly. While working on bug 215, as usual, > > I found a bunch of adjacent things to clean up. >=20 > So, I finally finished reviewing the series. >=20 > 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. >=20 > 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. >=20 > 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. >=20 > I still have some remarks and doubts about it though: >=20 > 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. >=20 > 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). >=20 > 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): >=20 > --- > 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, stru= ct 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 =3D -1, i; > struct ctx *c =3D &passt_ctx; > - int nfds, devnull_fd =3D -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"); > =20 > + for (i =3D STDIN_FILENO; i <=3D STDERR_FILENO; i++) > + close_low_fd[i] =3D !fcntl(i, F_GETFD); Nit: needs to have a >=3D 0, F_GETFD returns flags, not 0 on success. > + > arch_avx2_exec(argv); > =20 > isolate_initial(argc, argv); > @@ -419,7 +423,10 @@ int main(int argc, char **argv) > die("Failed to sandbox process, exiting"); > =20 > if (!c->foreground) { > - __daemon(c->pidfile_fd, devnull_fd); > + if (c->fd_tap !=3D -1 && c->fd_tap < STDERR_FILENO) > + close_low_fd[c->fd_tap] =3D false; > + > + __daemon(close_low_fd, c->pidfile_fd, devnull_fd); > close(c->pidfile_fd); > c->pidfile_fd =3D -1; > log_stderr =3D 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) > =20 > /** > * __daemon() - daemon()-like function writing PID file before parent ex= its > + * @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 devnu= ll_fd) > { > pid_t pid =3D fork(); > =20 > @@ -522,9 +523,9 @@ int __daemon(int pidfile_fd, int devnull_fd) > } > =20 > 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); > =20 > 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 devnu= ll_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 s= olved > by testing your series against current AppArmor and SELinux policies b= ut 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: >=20 > #define printf(x) @ "Don't call printf() directly, use err() / warn() / d= ebug() etc." >=20 > 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 socke= ts > 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. >=20 > > Much of the > > rest of the series is, for example, dealing with the possibility of > > --fd [012]. >=20 > ...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. --=20 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 --j40u2F4X+iA8Q01b Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpYayoACgkQzQJF27ox 2Gf34w/+Kk0Qg+AYcyWqDUVUNs8jmHCU7AEgRLdjqMZNMvd912NsOxRfE9O4OzA4 VUuXX84OxpHtz4JEFIH+tkmh/tKUTwhLrrmOLlL+sRzgDq/wHWVKb3cTx0yHmD+R evTYD7quROfg0C4ALr5W15mtmPJFSqwQlaebKkRk4AUx+HKl/p+ta1xo6sWFEJKH Jub0WAiJkxmVZdGga7FMasbeejQtgN0Tbkl0+Qn+oCStsmWrBsJhPukJqf9GGNm6 ukV7+SrNZDQiajvcA1UHML/sWCS6VDNUUFqGAO0DIK0KWJeSvTNhwJzd++h2tWPO AXeRWny45xhGaG42eLZEPsiKsbXfYMz9CevHk79lWdaFFDudPfyI9ng95sdm3h5y HOxMBz4Btt9+oeGYMNR8tz/DmISonEu+BxOFyFUYdRPsPOkQjJJho2t8eAxaJYnR 2QYzJtm7H878P/N76DBXkgzQPpj75yPaYDNXT+k/qOvbfa6A5ZgevErHfXdWcZ+E i1x6Bn0mJqKuBZoG/XlGPg663tBWTlhmT8kCYnaSh8nzaq/7Glb+x3Gpf9/OwBLq J9VkxO34asXxkYSbWE88T2SWLfALGxH3LrI7yqNyF9dwnyZp/py4WJ9vd2fEdxSa 1BdCWMSk1EcB1dDlO2P7GjqkXeYe3g442cH+A06Qub6C8R88ayA= =d8aD -----END PGP SIGNATURE----- --j40u2F4X+iA8Q01b--