dhcpcd-discuss

On 11/09/2020 20:28, Rob Newberry wrote:
> OK, I've at least PARTIALLY figured this out.
>
> There's a bug in my "embedded system/network manager" code that is somehow closing STDIN_FILENO before forking and running dhcpcd.
>
> I haven't yet figured out exactly where that bug is, but I will :-).
>
> It's a really cool bug, in that it's the kind of nastiness that few programs ever expect to deal with.  In this particular case, what was happening was that, because STDIN_FILENO was closed, the pidfile lock was getting back STDIN_FILENO for the lock file.
>
> And then when dhcpcd called "freopen" on stdin, it actually closed the pidfile and re-opened it for read-only (appropriate for stdin), and the future pidfile_lock failed because the descriptor was no longer open for writing.  Very weird repercussion from such an unexpected situation (I kinda like bugs like this, as it makes me learn more about other parts of the system in more detail).
>
> Anyway, very sorry to have bothered folks here, since it did indeed turn out to be a bug elsewhere (and completely my own), but really, really appreciate the help debugging.

WOW! That is a nice find!
Well, there could be other situations like this - although file descriptors 0, 1 
and 2 should always be open, no actual guarantee is made that this is the case.

Attached is a patch which should address this. Can you test it and let me know 
if it fixes it for you?

Roy

diff --git a/src/dhcpcd.c b/src/dhcpcd.c
index f80d293a..1c863b5e 100644
--- a/src/dhcpcd.c
+++ b/src/dhcpcd.c
@@ -360,7 +360,7 @@ dhcpcd_daemonise(struct dhcpcd_ctx *ctx)
 		return;
 
 	/* Don't use loginfo because this makes no sense in a log. */
-	if (!(logopts & LOGERR_QUIET))
+	if (!(logopts & LOGERR_QUIET) && ctx->stderr_valid)
 		(void)fprintf(stderr,
 		    "forked to background, child pid %d\n", getpid());
 	i = EXIT_SUCCESS;
@@ -1893,9 +1893,16 @@ main(int argc, char **argv)
 	ctx.ps_inet_fd = ctx.ps_control_fd = -1;
 	TAILQ_INIT(&ctx.ps_processes);
 #endif
-	rt_init(&ctx);
 
-	logopts = LOGERR_ERR|LOGERR_LOG|LOGERR_LOG_DATE|LOGERR_LOG_PID;
+	/* Check our streams for validity */
+	ctx.stdin_valid =  fcntl(STDIN_FILENO,  F_GETFD) != -1;
+	ctx.stdout_valid = fcntl(STDOUT_FILENO, F_GETFD) != -1;
+	ctx.stderr_valid = fcntl(STDERR_FILENO, F_GETFD) != -1;
+
+	logopts = LOGERR_LOG | LOGERR_LOG_DATE | LOGERR_LOG_PID;
+	if (ctx.stderr_valid)
+		logopts |= LOGERR_ERR;
+
 	i = 0;
 	while ((opt = getopt_long(argc, argv,
 	    ctx.options & DHCPCD_PRINT_PIDFILE ? NOERR_IF_OPTS : IF_OPTS,
@@ -1979,6 +1986,8 @@ main(int argc, char **argv)
 	ctx.ifc = argc - optind;
 	ctx.ifv = argv + optind;
 
+	rt_init(&ctx);
+
 	ifo = read_config(&ctx, NULL, NULL, NULL);
 	if (ifo == NULL) {
 		if (ctx.options & DHCPCD_PRINT_PIDFILE)
@@ -2256,12 +2265,13 @@ printpidfile:
 	}
 
 	loginfox(PACKAGE "-" VERSION " starting");
-	if (freopen(_PATH_DEVNULL, "r", stdin) == NULL)
-		logerr("%s: freopen stdin", __func__);
+	if (ctx.stdin_valid && freopen(_PATH_DEVNULL, "w", stdin) == NULL)
+		logwarn("freopen stdin");
 
 #if defined(USE_SIGNALS) && !defined(THERE_IS_NO_FORK)
 	if (xsocketpair(AF_UNIX, SOCK_DGRAM | SOCK_CXNB, 0, fork_fd) == -1 ||
-	    xsocketpair(AF_UNIX, SOCK_DGRAM | SOCK_CXNB, 0, stderr_fd) == -1)
+	    (ctx.stderr_valid &&
+	    xsocketpair(AF_UNIX, SOCK_DGRAM | SOCK_CXNB, 0, stderr_fd) == -1))
 	{
 		logerr("socketpair");
 		goto exit_failure;
@@ -2284,15 +2294,18 @@ printpidfile:
 		 * Redirect stdout as well.
 		 * dhcpcd doesn't output via stdout, but something in
 		 * a called script might.
-		 *
-		 * Do NOT rights limit this fd as it will affect scripts.
-		 * For example, cmp reports insufficient caps on FreeBSD.
 		 */
-		if (dup2(stderr_fd[1], STDERR_FILENO) == -1 ||
-		    dup2(stderr_fd[1], STDOUT_FILENO) == -1)
-			logerr("dup2");
-		close(stderr_fd[0]);
-		close(stderr_fd[1]);
+		if (ctx.stderr_valid) {
+			if (dup2(stderr_fd[1], STDERR_FILENO) == -1 ||
+			    (ctx.stdout_valid &&
+			    dup2(stderr_fd[1], STDOUT_FILENO) == -1))
+				logerr("dup2");
+			close(stderr_fd[0]);
+			close(stderr_fd[1]);
+		} else if (ctx.stdout_valid) {
+			if (freopen(_PATH_DEVNULL, "w", stdout) == NULL)
+				logerr("freopen stdout");
+		}
 		if (setsid() == -1) {
 			logerr("%s: setsid", __func__);
 			goto exit_failure;
@@ -2312,10 +2325,9 @@ printpidfile:
 		break;
 	default:
 		ctx.options |= DHCPCD_FORKED; /* A lie */
+		setproctitle("[launcher]");
 		ctx.fork_fd = fork_fd[0];
 		close(fork_fd[1]);
-		ctx.stderr_fd = stderr_fd[0];
-		close(stderr_fd[1]);
 #ifdef PRIVSEP_RIGHTS
 		if (ps_rights_limit_fd(fork_fd[0]) == -1 ||
 		    ps_rights_limit_fd(stderr_fd[0]) == 1)
@@ -2324,10 +2336,21 @@ printpidfile:
 			goto exit_failure;
 		}
 #endif
-		setproctitle("[launcher]");
 		eloop_event_add(ctx.eloop, ctx.fork_fd, dhcpcd_fork_cb, &ctx);
-		eloop_event_add(ctx.eloop, ctx.stderr_fd, dhcpcd_stderr_cb,
-		    &ctx);
+
+		if (ctx.stderr_valid) {
+			ctx.stderr_fd = stderr_fd[0];
+			close(stderr_fd[1]);
+#ifdef PRIVSEP_RIGHTS
+			if (ps_rights_limit_fd(stderr_fd[0]) == 1) {
+				logerr("ps_rights_limit_fdpair");
+				goto exit_failure;
+			}
+#endif
+			if (ctx.stderr_valid)
+				eloop_event_add(ctx.eloop, ctx.stderr_fd,
+				    dhcpcd_stderr_cb, &ctx);
+		}
 		goto run_loop;
 	}
 
diff --git a/src/dhcpcd.h b/src/dhcpcd.h
index 95b7e073..d45bd6a6 100644
--- a/src/dhcpcd.h
+++ b/src/dhcpcd.h
@@ -117,8 +117,11 @@ struct passwd;
 struct dhcpcd_ctx {
 	char pidfile[sizeof(PIDFILE) + IF_NAMESIZE + 1];
 	char vendor[256];
+	bool stdin_valid;	/* It's possible stdin, stdout and stderr */
+	bool stdout_valid;	/* could be closed when dhcpcd starts. */
+	bool stderr_valid;
+	int stderr_fd;	/* FD for reading stderr */
 	int fork_fd;	/* FD for the fork init signal pipe */
-	int stderr_fd;	/* FD for logging to stderr */
 	const char *cffile;
 	unsigned long long options;
 	char *logfile;
diff --git a/src/privsep.c b/src/privsep.c
index 3f1dc2d6..f92ef45b 100644
--- a/src/privsep.c
+++ b/src/privsep.c
@@ -76,7 +76,6 @@
 #ifdef HAVE_CAPSICUM
 #include <sys/capsicum.h>
 #include <capsicum_helpers.h>
-#define ps_rights_limit_stdio caph_limit_stdio
 #endif
 #ifdef HAVE_UTIL_H
 #include <util.h>
@@ -278,6 +277,25 @@ ps_rights_limit_fdpair(int fd[])
 		return -1;
 	return 0;
 }
+
+static int
+ps_rights_limit_stdio(struct dhcpcd_ctx *ctx)
+{
+	const int iebadf = CAPH_IGNORE_EBADF;
+	int error = 0;
+
+	if (ctx->stdin_valid &&
+	    caph_limit_stream(STDIN_FILENO, CAPH_READ | iebadf) == -1)
+		error = -1;
+	if (ctx->stdout_valid &&
+	    caph_limit_stream(STDOUT_FILENO, CAPH_WRITE | iebadf) == -1)
+		error = -1;
+	if (ctx->stderr_valid &&
+	    caph_limit_stream(STDERR_FILENO, CAPH_WRITE | iebadf) == -1)
+		error = -1;
+
+	return error;
+}
 #endif
 
 pid_t
@@ -346,7 +364,7 @@ ps_dostart(struct dhcpcd_ctx *ctx,
 
 #ifdef PRIVSEP_RIGHTS
 		/* We cannot limit the root process in any way. */
-		if (ps_rights_limit_stdio() == -1) {
+		if (ps_rights_limit_stdio(ctx) == -1) {
 			logerr("ps_rights_limit_stdio");
 			goto errexit;
 		}
@@ -484,7 +502,7 @@ ps_mastersandbox(struct dhcpcd_ctx *ctx)
 	if ((ctx->pf_inet_fd != -1 &&
 	    ps_rights_limit_ioctl(ctx->pf_inet_fd) == -1) ||
 	    (ctx->link_fd != -1 && ps_rights_limit_fd(ctx->link_fd) == -1) ||
-	     ps_rights_limit_stdio() == -1)
+	     ps_rights_limit_stdio(ctx) == -1)
 	{
 		logerr("%s: cap_rights_limit", __func__);
 		return -1;