From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: passt.top; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=PaAM9Osc; dkim-atps=neutral Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTPS id F00495A026E for ; Wed, 15 Jul 2026 18:39:53 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1784133592; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xrb2QWjT3ruoTfQzEInMrzN3zp2IW4ot/IhVsAVQ5bo=; b=PaAM9OscZeX5vXJo6RT3fVFnG6QkylbTa0Hy38sG3liUaiik7aIE3uZOLeH95QCL91IIn+ PB2/sEREgvum/X0pkSMNs82CSEpGOAJzRrYymS4UnfXeetI1MPtWp6hcHOYbA80TdAZvve lz+J/St9BMEPomQcax46yiD98R+sJY0= Received: from mail-wr1-f71.google.com (mail-wr1-f71.google.com [209.85.221.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-265-Zx5IzMDgOdqy3iwT7XfAMQ-1; Wed, 15 Jul 2026 12:39:51 -0400 X-MC-Unique: Zx5IzMDgOdqy3iwT7XfAMQ-1 X-Mimecast-MFC-AGG-ID: Zx5IzMDgOdqy3iwT7XfAMQ_1784133590 Received: by mail-wr1-f71.google.com with SMTP id ffacd0b85a97d-4744b72f90bso3068693f8f.0 for ; Wed, 15 Jul 2026 09:39:50 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1784133589; x=1784738389; h=date:content-transfer-encoding:content-type:mime-version :organization:references:in-reply-to:message-id:subject:cc:to:from :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to:content-type; bh=xrb2QWjT3ruoTfQzEInMrzN3zp2IW4ot/IhVsAVQ5bo=; b=IUkHWxEsWRDvQkSw7vY6FuxiS7uagMFq8c2s27qw3nLyrV6+AWfuU0UiGB+wRNe9xD lh7iR9m6ml9sVsa9CawoM8hH2fPJ4vztTqvL2Smd/E8H80icpbmnR54LxtiM/X7HmmZ/ iZ1FXtgd9Zhamgxf/Ss8HE1aVmtJM8X8mIdscqnrI9VZXA3aYIpbB9zKPwcmEeQxCYkA WojcIjFYIlV6NkzArz2NL6njPi4jfr7Hbp/3byGpi9H4/H+VCyvV/Be079dVwy0w+Ziv mySanJJfF7kcCPAYm4t+hpB16uZXv1k/ecdM7WQ2E1NVlZvO45aADpudwKAckAaGoG5D RKlw== X-Gm-Message-State: AOJu0YwC//LKIE0XuduJYxIP2Oy0zXtJGtfiEMKWcKxMF/P4EPo5DUww fCYT+G0MpWc4rlxusBs5nps1OelBNV498byhT9eN+JQw0RR/ORnLmOVk3nxSwVpRI1wvTh96ful VMH/JUuUP66LcbZ92cmQbZ+i2u2O4R3kT/Z/PT2ZMN/QoTlE2WjUWOoB9Gfk5OA== X-Gm-Gg: AfdE7cmulWetMtj5678dTM3bOgQeCkJVtUdgSahZYtD/lP1a8iYPUO6mEP0gPPO0MQS Je9f9f2bYtWn9XXGqx8d6px8UV5VoAtVD9vNeHECyhESs0MjsMwwl1oa1vY8v7Jk+uLEldpR3Q2 XgPRbPretORli9IOafaMY0W0flbfdaIoC3Uw94qPQmr/oLpk/fZjs6mlO1oDOk6YzY/VKIFxcnr 5i6YIcj+0SpGX60p4xM73ZgceRYO7pvEGdnp+9ghwqqzT8Vd2LnuXRfKG8+t+daNqJOlAG8J/pb ZaMkD9O6IOGDjzdmEKr4FmvzTBzHaXBdo4XDePZ8ekk1QwpTcUqwAzfSIALwswrWplLSUOrMs4L KP0QIH5gBpjpSrmCHgXqg3w== X-Received: by 2002:a5d:59a4:0:b0:472:67de:27ca with SMTP id ffacd0b85a97d-47f488bf7afmr9139115f8f.41.1784133589368; Wed, 15 Jul 2026 09:39:49 -0700 (PDT) X-Received: by 2002:a5d:59a4:0:b0:472:67de:27ca with SMTP id ffacd0b85a97d-47f488bf7afmr9139074f8f.41.1784133588762; Wed, 15 Jul 2026 09:39:48 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [2a10:fc81:a806:d6a9::1]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47f4635ac2esm18085645f8f.13.2026.07.15.09.39.47 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jul 2026 09:39:47 -0700 (PDT) From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 0/6] Fix bug 215 and some related issues with fd handling Message-ID: <20260715183946.6121580a@elisabeth> In-Reply-To: <20260714092926.2881848-1-david@gibson.dropbear.id.au> References: <20260714092926.2881848-1-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.49; x86_64-pc-linux-gnu) MIME-Version: 1.0 Date: Wed, 15 Jul 2026 18:39:47 +0200 (CEST) X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: pB9Otiw7KJflR-aMSzOx94tTqunZyPK8HrcLBuNckWk_1784133590 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: NT7GLWE2XM35FCKFI5QURPTKDRFZ6DHA X-Message-ID-Hash: NT7GLWE2XM35FCKFI5QURPTKDRFZ6DHA X-MailFrom: sbrivio@redhat.com 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: 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()?), 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"). > 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 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); + 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); --- 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...) 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 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 > 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. :) 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). -- Stefano