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=ShH2dzWd; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 82B985A0269 for ; Thu, 16 Jul 2026 09:49:33 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202606; t=1784188169; bh=w3GRQJLzNuSQuQ4vhhnyTi4DT4WeB1DyTAGzG77UGlk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=ShH2dzWd/PrB6Yc58LbpHxT/38jqwOSJT594fEYJ1lW0MSK8dyZxXmZOrxh0Cbj8v Gimmq+9GnuBHCyBVi7hEXYFE2hu5JnHTmYHtjf5/bgS1GRGTAwxaUDDGFtTffseFm3 ET5N10B/7+Qru7EkLhqbBuZSk4qbv97iP+LLO8ulS6tkeCa19Ae64mhgKanlT7HFBh Ns4IR2nyewEDczPHCftsaK0muCftOnGY7rH9/WUqfiXyffykPD/w1ni0chK+bbgBpJ W83e4APqZ6PxeMWsAnHcbz5nEEREWy5L54sGUqizO88O17yz3CiP6WXxouFAxHuWF2 ISEtgO2YTf3uA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4h14tx3j6Nz4wBD; Thu, 16 Jul 2026 17:49:29 +1000 (AEST) Date: Thu, 16 Jul 2026 17:49:16 +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> <20260716092228.698aa80e@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="Wbysi/muWEsbqsCw" Content-Disposition: inline In-Reply-To: <20260716092228.698aa80e@elisabeth> Message-ID-Hash: SCYLCCCM2J7VQCW4YHPPAAZBXSPCJYSF X-Message-ID-Hash: SCYLCCCM2J7VQCW4YHPPAAZBXSPCJYSF 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: --Wbysi/muWEsbqsCw Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: >=20 > > 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 usu= al, > > > > I found a bunch of adjacent things to clean up. =20 > > >=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()?), =20 > >=20 > > Hm, I'm not entirely sure of the distinction you're drawing between > > the two cases. > >=20 > > > 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"). =20 > >=20 > > Ah, good point, I'll do that for the next spin. > >=20 > > > > 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 standa= rd > > > > streams in fds 0-2 was an accident waiting to happen. =20 > > >=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 =20 > >=20 > > That... really seems like taking bug for bug compatibility too far to > > me. > >=20 > > >=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, = 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 =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); =20 > >=20 > > Nit: needs to have a >=3D 0, F_GETFD returns flags, not 0 on success. > >=20 > > > + > > > 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 flag= s) > > > =20 > > > /** > > > * __daemon() - daemon()-like function writing PID file before paren= t 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 exi= ts. > > > * 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 d= evnull_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 d= evnull_fd); > > > int fls(unsigned long x); > > > int ilog2(unsigned long x); > > > int write_file(const char *path, const char *buf); > > > --- =20 > >=20 > > Yes, that will do the job. I still think avoiding anything but the > > standard streams in 0-2 is worthwhile. > >=20 > > > 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 polici= es but I > > > didn't get the chance yet (it would be nice if you could...) =20 > >=20 > > 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. >=20 > 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. >=20 > For example, pasta's SELinux policy currently has: >=20 > allow pasta_t console_device_t:chr_file { open write getattr ioctl }; > allow pasta_t user_devpts_t:chr_file { getattr read write ioctl }; >=20 > 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 pr= event > > > direct usage anyway with, say: > > >=20 > > > #define printf(x) @ "Don't call printf() directly, use err() / warn()= / debug() 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 su= re > > > we would have any remaining concern about risks of using standard > > > streams by mistake =20 > >=20 > > I guess. I feel like that's even uglier than what I've proposed. >=20 > 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 s= ockets > > > will never be numbered 0 and save a lot of initialisations to -1 a= nd > > > related checks, at that point? If we can achieve that as side effe= ct, > > > that would be another argument in favour of it in my eyes =20 > >=20 > > Huh, interesting, I guess it would allow that. > >=20 > > > =20 > > > > Much of the > > > > rest of the series is, for example, dealing with the possibility of > > > > --fd [012]. =20 > > >=20 > > > ...well, yes, but the possibility of --fd [12] was introduced by the > > > series itself. :) =20 > >=20 > > Well, true, but dealing with -fd [12] was basically free while dealing > > with --fd 0. > >=20 > > > 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).= =20 > >=20 > > 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 > Sure. >=20 > --=20 > Stefano >=20 --=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 --Wbysi/muWEsbqsCw Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmpYjPcACgkQzQJF27ox 2GfGjA/6A67r1pTdQDFW1CLS5E4b730155L5NSRym6EqeQhfi5sfkWzjWThY7Kel kEcONMxK7VKLDNaCs6ct3ifFtjTdnRhgywM6Prre09ufQFn5QnoeJKkWdg6nZFCS 0/niMfXKXxHmAHvSY/YYKPKEg21RPhr3yXDZ+1lNENA8RgUKU6fzd4XasFGHRPlU m5c4cox3I1hGF1S83SnpcJcvuznS7DiID3kmJIuprrD0FCfnQa6a5sxhakLf+N8S TK2DBjWcY1DSBjlDQhUutDTM7b+LZV1xnsze3qyEmvHHiW0TklJMIZWc/66kCMsp 4x4+TDFCOITxoYCAR41tifZjnierc04Y16MfWPhWLIREWTCIXolOtqQoqD/ioq5p TEhoWs2BFFhUd38EuvEFJdH3m7y9udXKLekLUOsBuZIBQfPvTCW60JbEE3cdgBM6 8ZEiIuSLgdgdjs5WQ0q+xfEBmruLL3S0Vqs58JkMkBSSEjF2fSlfvJlIFwH3/4F7 jUAAN6mevhiRIqMER3yJNB1oTHnEfGfMm5p/IQnLkh10WsXCam1/FPoSe9+bcBcK gHzUURHU8iZzOkL7Mkc0agajAsS7sBqCp3mJAhEwqkqY02nRfYFqtB2wHF2E7vE1 3jn/2CElKYqS8FL48IQCbAU76zjk+FpgtKl7/6s8kbhB7ZQlPzI= =CuJ9 -----END PGP SIGNATURE----- --Wbysi/muWEsbqsCw--