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

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

[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

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

[PATCH v2 1/6] passt: Always close pidfile_fd, not just when daemonizing

[David Gibson]

In order to satisfy static checkers that we don't have an fd leak,
a9c61ffaf153 added a close() of c->pidfile_fd, amongst others.  However,
it only close()s it in the case where we daemonize into the background.
While less universally useful in the foreground / non-daemon case, it's
perfectly reasonable to still have a pidfile.  We'll still write it, and
we should still close it.

Cc: Jon Maloy <jmaloy@redhat.com>
Fixes: a9c61ffaf153 ("util, passt: Close daemon-lifetime fds on exit to avoid Coverity warning")
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 passt.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/passt.c b/passt.c
index 65a07d72..cc847ec2 100644
--- a/passt.c
+++ b/passt.c
@@ -420,11 +420,13 @@ int main(int argc, char **argv)
 
 	if (!c->foreground) {
 		__daemon(c->pidfile_fd, devnull_fd);
-		close(c->pidfile_fd);
-		c->pidfile_fd = -1;
 		log_stderr = false;
 	} else {
 		pidfile_write(c->pidfile_fd, getpid());
+	}
+
+	if (c->pidfile_fd >= 0) {
+		close(c->pidfile_fd);
 		c->pidfile_fd = -1;
 	}
 
-- 
2.55.0

[PATCH v2 2/6] isolation: Move close_open_files() to isolate_fds()

[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 <david@gibson.dropbear.id.au>
---
 isolation.c | 67 ++++++++++++++++++++++++++++++++++++++++++++++-------
 isolation.h |  3 ++-
 passt.c     |  3 ++-
 util.c      | 49 ---------------------------------------
 util.h      |  1 -
 5 files changed, 63 insertions(+), 60 deletions(-)

diff --git a/isolation.c b/isolation.c
index c868e668..ea85fdba 100644
--- a/isolation.c
+++ b/isolation.c
@@ -24,13 +24,18 @@
  * done anything we need to do with those resources, so we have
  * multiple stages of self-isolation.  In order these are:
  *
- * 1. isolate_initial()
+ * 1a. isolate_initial()
  * ====================
  *
  * Executed immediately after startup, drops capabilities we don't
  * need at any point during execution (or which we gain back when we
- * need by joining other namespaces), and closes any leaked file we
- * might have inherited from the parent process.
+ * need by joining other namespaces).
+ *
+ * 1b. isolate_fds()
+ * ================
+ *
+ * Executed immediately after isolate_initial().  Closes any leaked
+ * files we might have inherited from the parent process.
  *
  * 2. isolate_user()
  * =================
@@ -58,6 +63,7 @@
 
 #include <errno.h>
 #include <fcntl.h>
+#include <getopt.h>
 #include <grp.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -88,6 +94,7 @@
 #include "passt.h"
 #include "log.h"
 #include "isolation.h"
+#include "conf.h"
 
 #define CAP_VERSION	_LINUX_CAPABILITY_VERSION_3
 #define CAP_WORDS	_LINUX_CAPABILITY_U32S_3
@@ -193,16 +200,13 @@ static int move_root(void)
 
 /**
  * isolate_initial() - Early, mostly config independent self isolation
- * @argc:	Argument count
- * @argv:	Command line options: only --fd (if present) is relevant here
  *
  * Should:
  *  - drop unneeded capabilities
- *  - close all open files except for standard streams and the one from --fd
  * Mustn't:
  *  - remove filesystem access (we need to access files during setup)
  */
-void isolate_initial(int argc, char **argv)
+void isolate_initial(void)
 {
 	uint64_t keep;
 
@@ -243,8 +247,55 @@ void isolate_initial(int argc, char **argv)
 		keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE);
 
 	drop_caps_ep_except(keep);
+}
+
+
+/*
+ * isolate_fds() - Close leaked files, but not --fd, stdin, stdout, stderr
+ * @argc:	Argument count
+ * @argv:	Command line options, as we need to skip any file given via --fd
+ *
+ * Should:
+ *  - close all open files except for standard streams and the one from --fd
+ */
+void isolate_fds(int argc, char **argv)
+{
+	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
+					{ 0 }, };
+	long fd = -1;
+	int name, rc;
+
+	do {
+		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
+
+		if (name == 'F')
+			fd = conf_tap_fd(optarg);
+	} while (name != -1);
+
+	if (fd == -1) {
+		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	} else if (fd == STDERR_FILENO + 1) { /* Still a single range */
+		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
+	} else {
+		rc = close_range(STDERR_FILENO + 1, fd - 1,
+				 CLOSE_RANGE_UNSHARE);
+		if (!rc)
+			rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	}
 
-	close_open_files(argc, argv);
+	if (rc) {
+		if (errno == ENOSYS || errno == EINVAL) {
+			/* This probably means close_range() or the
+			 * CLOSE_RANGE_UNSHARE flag is not supported by the
+			 * kernel.  Not much we can do here except carry on and
+			 * hope for the best.
+			 */
+			warn(
+"Can't use close_range() to ensure no files leaked by parent");
+		} else {
+			die_perror("Failed to close files leaked by parent");
+		}
+	}
 }
 
 /**
diff --git a/isolation.h b/isolation.h
index 66b6968d..e1b1bc5d 100644
--- a/isolation.h
+++ b/isolation.h
@@ -10,7 +10,8 @@
 #include <stdbool.h>
 #include <unistd.h>
 
-void isolate_initial(int argc, char **argv);
+void isolate_initial(void);
+void isolate_fds(int argc, char **argv);
 void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns,
 		  const char *userns);
 int isolate_prefork(const struct ctx *c);
diff --git a/passt.c b/passt.c
index cc847ec2..ce20cedb 100644
--- a/passt.c
+++ b/passt.c
@@ -341,7 +341,8 @@ int main(int argc, char **argv)
 
 	arch_avx2_exec(argv);
 
-	isolate_initial(argc, argv);
+	isolate_initial();
+	isolate_fds(argc, argv);
 
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
diff --git a/util.c b/util.c
index 4bc5d6f8..ce5021a9 100644
--- a/util.c
+++ b/util.c
@@ -26,7 +26,6 @@
 #include <stdbool.h>
 #include <linux/errqueue.h>
 #include <linux/in6.h>
-#include <getopt.h>
 
 #include "linux_dep.h"
 #include "util.h"
@@ -38,7 +37,6 @@
 #include "epoll_ctl.h"
 #include "pasta.h"
 #include "serialise.h"
-#include "conf.h"
 #ifdef HAS_GETRANDOM
 #include <sys/random.h>
 #endif
@@ -924,53 +922,6 @@ const char *str_ee_origin(const struct sock_extended_err *ee)
 	return "<invalid>";
 }
 
-/**
- * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr
- * @argc:	Argument count
- * @argv:	Command line options, as we need to skip any file given via --fd
- */
-void close_open_files(int argc, char **argv)
-{
-	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
-					{ 0 },
-				      };
-	long fd = -1;
-	int name, rc;
-
-	do {
-		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
-
-		if (name == 'F')
-			fd = conf_tap_fd(optarg);
-	} while (name != -1);
-
-	if (fd == -1) {
-		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	} else if (fd == STDERR_FILENO + 1) { /* Still a single range */
-		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
-	} else {
-		rc = close_range(STDERR_FILENO + 1, fd - 1,
-				 CLOSE_RANGE_UNSHARE);
-		if (!rc)
-			rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	}
-
-	if (rc) {
-		if (errno == ENOSYS || errno == EINVAL) {
-			/* This probably means close_range() or the
-			 * CLOSE_RANGE_UNSHARE flag is not supported by the
-			 * kernel.  Not much we can do here except carry on and
-			 * hope for the best.
-			 */
-			warn(
-"Can't use close_range() to ensure no files leaked by parent");
-		} else {
-			die_perror("Failed to close files leaked by parent");
-		}
-	}
-
-}
-
 /**
  * snprintf_check() - snprintf() wrapper, checking for truncation and errors
  * @str:	Output buffer
diff --git a/util.h b/util.h
index 90e8a20d..2435f536 100644
--- a/util.h
+++ b/util.h
@@ -168,7 +168,6 @@ intmax_t read_file_integer(const char *path, intmax_t fallback);
 int write_remainder(int fd, const struct iovec *iov, size_t iovcnt,
 		    size_t skip, size_t length);
 int read_remainder(int fd, const struct iovec *iov, size_t cnt, size_t skip);
-void close_open_files(int argc, char **argv);
 bool snprintf_check(char *str, size_t size, const char *format, ...);
 long clamped_scale(long x, long y, long lo, long hi, long f);
 
-- 
2.55.0

[PATCH v2 3/6] isolation, conf: Set c->fd_tap from early parse of --fd

[David Gibson]

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 <david@gibson.dropbear.id.au>
---
 conf.c      | 2 +-
 isolation.c | 6 +++++-
 isolation.h | 2 +-
 passt.c     | 2 +-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/conf.c b/conf.c
index 5b6cc2be..b3211e54 100644
--- a/conf.c
+++ b/conf.c
@@ -1593,7 +1593,7 @@ void conf(struct ctx *c, int argc, char **argv)
 			c->fd_control_listen = c->fd_control = -1;
 			break;
 		case 'F':
-			c->fd_tap = conf_tap_fd(optarg);
+			/* --fd was parsed early and c->fd_tap set in main() */
 			c->one_off = true;
 			*c->sock_path = 0;
 			break;
diff --git a/isolation.c b/isolation.c
index ea85fdba..c9dfefa8 100644
--- a/isolation.c
+++ b/isolation.c
@@ -257,8 +257,10 @@ void isolate_initial(void)
  *
  * Should:
  *  - close all open files except for standard streams and the one from --fd
+ *
+ * Return: fd number from --fd, or -1 if not specified
  */
-void isolate_fds(int argc, char **argv)
+int isolate_fds(int argc, char **argv)
 {
 	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
 					{ 0 }, };
@@ -296,6 +298,8 @@ void isolate_fds(int argc, char **argv)
 			die_perror("Failed to close files leaked by parent");
 		}
 	}
+
+	return fd;
 }
 
 /**
diff --git a/isolation.h b/isolation.h
index e1b1bc5d..ec470388 100644
--- a/isolation.h
+++ b/isolation.h
@@ -11,7 +11,7 @@
 #include <unistd.h>
 
 void isolate_initial(void);
-void isolate_fds(int argc, char **argv);
+int isolate_fds(int argc, char **argv);
 void isolate_user(const struct ctx *c, uid_t uid, gid_t gid, bool use_userns,
 		  const char *userns);
 int isolate_prefork(const struct ctx *c);
diff --git a/passt.c b/passt.c
index ce20cedb..e930df43 100644
--- a/passt.c
+++ b/passt.c
@@ -342,7 +342,7 @@ int main(int argc, char **argv)
 	arch_avx2_exec(argv);
 
 	isolate_initial();
-	isolate_fds(argc, argv);
+	c->fd_tap = isolate_fds(argc, argv);
 
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
-- 
2.55.0

[PATCH v2 4/6] conf: Make conf_tap_fd() operate more like conf_mode()

[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 <david@gibson.dropbear.id.au>
---
 conf.c      | 25 +++++++++++++++++++++----
 conf.h      |  2 +-
 isolation.c | 13 ++-----------
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/conf.c b/conf.c
index b3211e54..df204d13 100644
--- a/conf.c
+++ b/conf.c
@@ -1153,17 +1153,34 @@ static void conf_sock_listen(const struct ctx *c)
 
 /**
  * conf_tap_fd() - Read tap fd as supplied by -F command line option
- * @arg:	Argument to -F command line option
+ * @argc:	Argument count
+ * @argv:	Command line options
+ *
+ * Return: fd number from --fd option, or -1 if not supplied
  */
-int conf_tap_fd(const char *arg)
+int conf_tap_fd(int argc, char **argv)
 {
-	const char *p = arg;
+	const struct option optfd[] = {	{ "fd", required_argument, NULL, 'F' },
+					{ 0 }, };
+	const char *fdarg = NULL, *p;
 	unsigned long val;
+	int name;
+
+	optind = 0;
+	do {
+		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
+		if (name == 'F')
+			fdarg = optarg;
+	} while (name != -1);
+
+	if (!fdarg)
+		return -1;
 
+	p = fdarg;
 	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p)	||
 	    val > INT_MAX					||
 	    (val != STDIN_FILENO && val <= STDERR_FILENO))
-		die("Invalid --fd: %s", arg);
+		die("Invalid --fd: %s", fdarg);
 
 	return val;
 }
diff --git a/conf.h b/conf.h
index 1fa1280e..19bf9bc5 100644
--- a/conf.h
+++ b/conf.h
@@ -7,7 +7,7 @@
 #define CONF_H
 
 enum passt_modes conf_mode(int argc, char *argv[]);
-int conf_tap_fd(const char *arg);
+int conf_tap_fd(int argc, char **argv);
 void conf(struct ctx *c, int argc, char **argv);
 void conf_listen_handler(struct ctx *c, uint32_t events);
 void conf_handler(struct ctx *c, uint32_t events);
diff --git a/isolation.c b/isolation.c
index c9dfefa8..725a72bb 100644
--- a/isolation.c
+++ b/isolation.c
@@ -63,7 +63,6 @@
 
 #include <errno.h>
 #include <fcntl.h>
-#include <getopt.h>
 #include <grp.h>
 #include <inttypes.h>
 #include <limits.h>
@@ -262,17 +261,9 @@ void isolate_initial(void)
  */
 int isolate_fds(int argc, char **argv)
 {
-	const struct option optfd[] = { { "fd", required_argument, NULL, 'F' },
-					{ 0 }, };
-	long fd = -1;
-	int name, rc;
+	int fd, rc;
 
-	do {
-		name = getopt_long(argc, argv, "-:F:", optfd, NULL);
-
-		if (name == 'F')
-			fd = conf_tap_fd(optarg);
-	} while (name != -1);
+	fd = conf_tap_fd(argc, argv);
 
 	if (fd == -1) {
 		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-- 
2.55.0

[PATCH v2 5/6] isolation: Move --fd descriptor to a number of our choosing

[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 <hi@alyssa.is>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 conf.c      |  4 +---
 isolation.c | 24 +++++++++++-------------
 2 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/conf.c b/conf.c
index df204d13..0fcba5cf 100644
--- a/conf.c
+++ b/conf.c
@@ -1177,9 +1177,7 @@ int conf_tap_fd(int argc, char **argv)
 		return -1;
 
 	p = fdarg;
-	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p)	||
-	    val > INT_MAX					||
-	    (val != STDIN_FILENO && val <= STDERR_FILENO))
+	if (!parse_unsigned(&p, 0, &val) || !parse_eoi(p) || val > INT_MAX)
 		die("Invalid --fd: %s", fdarg);
 
 	return val;
diff --git a/isolation.c b/isolation.c
index 725a72bb..94cbe7f8 100644
--- a/isolation.c
+++ b/isolation.c
@@ -248,7 +248,6 @@ void isolate_initial(void)
 	drop_caps_ep_except(keep);
 }
 
-
 /*
  * isolate_fds() - Close leaked files, but not --fd, stdin, stdout, stderr
  * @argc:	Argument count
@@ -256,27 +255,26 @@ void isolate_initial(void)
  *
  * Should:
  *  - close all open files except for standard streams and the one from --fd
+ *  - move the --fd descriptor out of the range 0-2
  *
- * Return: fd number from --fd, or -1 if not specified
+ * Return: new fd number for descriptor from --fd, or -1 if not specified
  */
 int isolate_fds(int argc, char **argv)
 {
-	int fd, rc;
+	int fd, close_from = STDERR_FILENO + 1;
 
 	fd = conf_tap_fd(argc, argv);
 
-	if (fd == -1) {
-		rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE);
-	} else if (fd == STDERR_FILENO + 1) { /* Still a single range */
-		rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE);
-	} else {
-		rc = close_range(STDERR_FILENO + 1, fd - 1,
-				 CLOSE_RANGE_UNSHARE);
-		if (!rc)
-			rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE);
+	if (fd >= 0) {
+		/* Move the passed fd to a more convenient location */
+		if (fd != close_from			&&
+		    (dup2(fd, close_from) != close_from	||
+		     close(fd)))
+			die_perror("Could not move --fd descriptor");
+		fd = close_from++;
 	}
 
-	if (rc) {
+	if (close_range(close_from, ~0U, CLOSE_RANGE_UNSHARE)) {
 		if (errno == ENOSYS || errno == EINVAL) {
 			/* This probably means close_range() or the
 			 * CLOSE_RANGE_UNSHARE flag is not supported by the
-- 
2.55.0

[PATCH v2 6/6] main: Ensure fds 0-2 are populated

[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 <david@gibson.dropbear.id.au>
---
 passt.c | 19 +++++++++++++------
 util.c  |  3 +--
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/passt.c b/passt.c
index e930df43..a2e7bf1f 100644
--- a/passt.c
+++ b/passt.c
@@ -330,8 +330,8 @@ 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];
+	int nfds, devnull_fd = -1, fd;
 	struct ctx *c = &passt_ctx;
-	int nfds, devnull_fd = -1;
 	struct rlimit limit;
 	struct timespec now;
 	struct sigaction sa;
@@ -344,6 +344,15 @@ int main(int argc, char **argv)
 	isolate_initial();
 	c->fd_tap = isolate_fds(argc, argv);
 
+	if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0)
+		die_perror("Failed to open /dev/null");
+	/* Ensure fds 0-2 are populated */
+	for (fd = 0; fd <= STDERR_FILENO; fd++) {
+		if (fcntl(fd, F_GETFD) < 0 &&
+		    dup2(devnull_fd, fd) < 0)
+			die_perror("Failed to populate fd %d", fd);
+	}
+
 	sigemptyset(&sa.sa_mask);
 	sa.sa_flags = 0;
 	sa.sa_handler = exit_handler;
@@ -411,11 +420,6 @@ int main(int argc, char **argv)
 	fwd_neigh_table_init(c);
 	nl_neigh_notify_init(c);
 
-	if (!c->foreground) {
-		if ((devnull_fd = open("/dev/null", O_RDWR | O_CLOEXEC)) < 0)
-			die_perror("Failed to open /dev/null");
-	}
-
 	if (isolate_prefork(c))
 		die("Failed to sandbox process, exiting");
 
@@ -431,6 +435,9 @@ int main(int argc, char **argv)
 		c->pidfile_fd = -1;
 	}
 
+	if (devnull_fd > STDERR_FILENO)
+		close(devnull_fd);
+
 	if (pasta_child_pid) {
 		kill(pasta_child_pid, SIGUSR1);
 		log_stderr = false;
diff --git a/util.c b/util.c
index ce5021a9..bd4c6caa 100644
--- a/util.c
+++ b/util.c
@@ -522,8 +522,7 @@ 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(devnull_fd))
+	    dup2(devnull_fd, STDERR_FILENO)	< 0)
 		passt_exit(EXIT_FAILURE);
 
 	return 0;
-- 
2.55.0