[PATCH v2 0/6] Fix bug 215 and some related issues with fd handling
Stefano, you called it correctly. While working on bug 215, as usual, I found a bunch of adjacent things to clean up. 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. Much of the rest of the series is, for example, dealing with the possibility of --fd [012]. v2: * No longer fold fd closing into isolate_initial() * Some minor polish * Various improved comments and commit messages David Gibson (6): passt: Always close pidfile_fd, not just when daemonizing isolation: Move close_open_files() to isolate_fds() isolation, conf: Set c->fd_tap from early parse of --fd conf: Make conf_tap_fd() operate more like conf_mode() isolation: Move --fd descriptor to a number of our choosing main: Ensure fds 0-2 are populated conf.c | 31 ++++++++++++++++++++------- conf.h | 2 +- isolation.c | 60 ++++++++++++++++++++++++++++++++++++++++++++++------- isolation.h | 3 ++- passt.c | 28 +++++++++++++++++-------- util.c | 52 +--------------------------------------------- util.h | 1 - 7 files changed, 98 insertions(+), 79 deletions(-) -- 2.55.0
We parse --fd twice: once in isolate_initial() just to avoid clobbering
the passed in fd. Then we parse it "for real" in conf(), to set c->fd_tap
and other configuration variables.
Change this, so that we return the value parsed early from
isolate_initial() and set c->fd_tap from that. This doesn't accomplish
much immediately, but will make some further cleanups possible.
Signed-off-by: David Gibson
Most functions in util.c are, well, utilities, that are useful in a bunch
of places. close_open_files(), however, is very specific, it's only called
from isolate_initial(), and performs a very specific step of our self
isolation. So, it makes more sense as a function in isolate.c - move it
there and rename it to isolate_fds().
In addition, call it directly from main() rather than from
isolate_initial(). That's pretty arbitrary now, but will make some
subsequent changes easier.
Signed-off-by: David Gibson
We have two cases where we need to parse specific options early:
conf_tap_fd() and conf_mode(). conf_tap_fd() has a slightly odd interface,
requiring the caller to use getopt_long() to find the right option, then
pass it in. Alter it to work like conf_mode() instead, where all the
command line parsing logic is contained within the conf.c function.
This is slightly more lines, but has a clearer division of responsibility.
Signed-off-by: David Gibson
Usually fds 0-2 are stdin, stdout and stderr. However, there are certain
use cases where passt can be invoked with one or more of those standard fds
closed. In those cases, anything we open might be placed in one of the
standard slots. For the handful of things we open early enough, this can
be a problem because we close fds 0-2 in __daemon(), replacing them with
dupes of /dev/null.
We could avoid closing those fds in __daemon() if they're not standard
streams. However, leaving things other than the standard streams in fds
0-2 is a footgun: a stray printf() that occurs in a circumstance it
shouldn't could send harmful garbage to a device or socket. It's also
likely to be confusing if debugging with strace or similar.
To avoid this, fill any missing standard streams with a dupe of /dev/null,
right after isolate_fds(). Since open()ing /dev/null itself could land in
one of those fd 0-2 slots, we need to be careful when we close it not to
leave a new gap.
Cc: jirib79@gmail.com
Link: https://bugs.passt.top/show_bug.cgi?id=215
Signed-off-by: David Gibson
Some users of passt pass an fd for the tap interface in, with the --fd
parameter, rather than having passt open it itself. This requires some
slightly fiddly logic in isolate_fds() so we don't close() it along with
any other file descriptors leaked into us by the parent.
More importantly, this is broken if the passed fd is 0, 1 or 2, since in
that case we will assume it's a standard stream and close it in __daemon().
We explicitly disallow 1 or 2 in conf_tap_fd(), but 0 has been permitted
since aa1cc8922 ("conf: allow --fd 0"). It looks like the use case of the
contributor of that patch didn't involve daemonizing passt.
To fix this more robustly, use dup2() to move to the passed fd to 3. This
removes the possibility of mixing it up with a standard stream, and as a
bonus makes the close_range() logic much simpler. With isolate_fds() made
safe for --fd 1 and --fd 2, we can remove the logic excluding those from
conf_fd_tap().
Cc: Alyssa Ross
On Fri, 17 Jul 2026 15:46:28 +1000
David Gibson
Stefano, you called it correctly. While working on bug 215, as usual, I found a bunch of adjacent things to clean up.
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. Much of the rest of the series is, for example, dealing with the possibility of --fd [012].
v2: * No longer fold fd closing into isolate_initial() * Some minor polish * Various improved comments and commit messages
Applied. -- Stefano
participants (2)
-
David Gibson
-
Stefano Brivio