dhcpcd: Remove stdio callback and detach on daemonise

For some reason, the stdio callback is extremely flaky on
*some* Linux based distributions making it very hard to debug some
things.
Removing it is fine because we now enforce that we have file descriptors
for stdin, stdout and stdrr on launch and dup them to /dev/null on daemonise.

It's also interesting to see behavioural differences between
some socketpair implementations that emit a HANGUP and some don't.

As such, we now close the fork socket on daemonise once more AND
in the fork_cb depending on if we hangup or read zero first.

Fixes #262

diff --git a/src/dhcpcd.c b/src/dhcpcd.c
index 25ce57c10d6a6766e9cb867871ba74ce05236916..17af1a05e46735c07364a34b97f1a2c5905d94ac 100644 (file)
--- a/src/dhcpcd.c
+++ b/src/dhcpcd.c
@@ -364,7 +364,7 @@ dhcpcd_daemonise(struct dhcpcd_ctx *ctx)
        errno = ENOSYS;
        return;
 #else
-       int i;
+       int exit_code;
 
        if (ctx->options & DHCPCD_DAEMONISE &&
            !(ctx->options & (DHCPCD_DAEMONISED | DHCPCD_NOWAITIP)))
@@ -385,16 +385,19 @@ dhcpcd_daemonise(struct dhcpcd_ctx *ctx)
                return;
 
 #ifdef PRIVSEP
-       ps_daemonised(ctx);
+       if (IN_PRIVSEP(ctx))
+               ps_daemonised(ctx);
+       else
 #else
-       dhcpcd_daemonised(ctx);
+               dhcpcd_daemonised(ctx);
 #endif
 
-       i = EXIT_SUCCESS;
-       if (write(ctx->fork_fd, &i, sizeof(i)) == -1)
-               logerr("write");
-       ctx->options |= DHCPCD_DAEMONISED;
-       // dhcpcd_fork_cb will close the socket
+       eloop_event_delete(ctx->eloop, ctx->fork_fd);
+       exit_code = EXIT_SUCCESS;
+       if (write(ctx->fork_fd, &exit_code, sizeof(exit_code)) == -1)
+               logerr(__func__);
+       close(ctx->fork_fd);
+       ctx->fork_fd = -1;
 #endif
 }
 
@@ -1814,30 +1817,6 @@ dhcpcd_readdump(struct dhcpcd_ctx *ctx)
            dhcpcd_readdump0, ctx);
 }
 
-static void
-dhcpcd_stderr_cb(void *arg, unsigned short events)
-{
-       struct dhcpcd_ctx *ctx = arg;
-       char log[BUFSIZ];
-       ssize_t len;
-
-       if (events & ELE_HANGUP)
-               eloop_exit(ctx->eloop, EXIT_SUCCESS);
-
-       if (!(events & ELE_READ))
-               return;
-
-       len = read(ctx->stderr_fd, log, sizeof(log) - 1);
-       if (len == -1) {
-               if (errno != ECONNRESET)
-                       logerr(__func__);
-               return;
-       }
-
-       log[len] = '\0';
-       fprintf(stderr, "%s", log);
-}
-
 static void
 dhcpcd_fork_cb(void *arg, unsigned short events)
 {
@@ -1928,7 +1907,7 @@ main(int argc, char **argv, char **envp)
        ssize_t len;
 #if defined(USE_SIGNALS) || !defined(THERE_IS_NO_FORK)
        pid_t pid;
-       int fork_fd[2], stderr_fd[2];
+       int fork_fd[2];
 #endif
 #ifdef USE_SIGNALS
        int sig = 0;
@@ -2013,22 +1992,17 @@ main(int argc, char **argv, char **envp)
        TAILQ_INIT(&ctx.ps_processes);
 #endif
 
-       /* 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;
 
-       /* Even we if we don't have input/outputs, we need to
-        * ensure they are setup for shells. */
-       if (!ctx.stdin_valid)
+       /* Ensure we have stdin, stdout and stderr file descriptors.
+        * This is important as we do run scripts which expect these. */
+       if (fcntl(STDIN_FILENO,  F_GETFD) == -1)
                dup_null(STDIN_FILENO);
-       if (!ctx.stdout_valid)
+       if (fcntl(STDOUT_FILENO,  F_GETFD) == -1)
                dup_null(STDOUT_FILENO);
-       if (!ctx.stderr_valid)
+       if (fcntl(STDERR_FILENO,  F_GETFD) == -1)
                dup_null(STDERR_FILENO);
-
-       logopts = LOGERR_LOG | LOGERR_LOG_DATE | LOGERR_LOG_PID;
-       if (ctx.stderr_valid)
+       else
                logopts |= LOGERR_ERR;
 
        i = 0;
@@ -2398,17 +2372,13 @@ printpidfile:
        loginfox(PACKAGE "-" VERSION " starting");
 
        // We don't need stdin past this point
-       if (ctx.stdin_valid)
-               dup_null(STDIN_FILENO);
+       dup_null(STDIN_FILENO);
 
 #if defined(USE_SIGNALS) && !defined(THERE_IS_NO_FORK)
        if (!(ctx.options & DHCPCD_DAEMONISE))
                goto start_manager;
 
-       if (xsocketpair(AF_UNIX, SOCK_SEQPACKET|SOCK_CXNB, 0, fork_fd) == -1 ||
-           (ctx.stderr_valid &&
-           xsocketpair(AF_UNIX, SOCK_SEQPACKET|SOCK_CXNB, 0, stderr_fd) == -1))
-       {
+       if (xsocketpair(AF_UNIX, SOCK_SEQPACKET|SOCK_CXNB, 0, fork_fd) == -1) {
                logerr("socketpair");
                goto exit_failure;
        }
@@ -2429,22 +2399,6 @@ printpidfile:
                    dhcpcd_fork_cb, &ctx) == -1)
                        logerr("%s: eloop_event_add", __func__);
 
-               /*
-                * Redirect stderr to the stderr socketpair.
-                * Redirect stdout as well.
-                * dhcpcd doesn't output via stdout, but something in
-                * a called script might.
-                */
-               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)
-                       dup_null(STDOUT_FILENO);
-
                if (setsid() == -1) {
                        logerr("%s: setsid", __func__);
                        goto exit_failure;
@@ -2478,19 +2432,6 @@ printpidfile:
                    dhcpcd_fork_cb, &ctx) == -1)
                        logerr("%s: eloop_event_add", __func__);
 
-               if (ctx.stderr_valid) {
-                       ctx.stderr_fd = stderr_fd[0];
-                       close(stderr_fd[1]);
-#ifdef PRIVSEP_RIGHTS
-                       if (ps_rights_limit_fd(ctx.stderr_fd) == 1) {
-                               logerr("ps_rights_limit_fd");
-                               goto exit_failure;
-                       }
-#endif
-                       if (eloop_event_add(ctx.eloop, ctx.stderr_fd, ELE_READ,
-                           dhcpcd_stderr_cb, &ctx) == -1)
-                               logerr("%s: eloop_event_add", __func__);
-               }
 #ifdef PRIVSEP
                if (IN_PRIVSEP(&ctx) && ps_managersandbox(&ctx, NULL) == -1)
                        goto exit_failure;
@@ -2602,6 +2543,7 @@ start_manager:
                if (ifp->active == IF_ACTIVE_USER)
                        break;
        }
+
        if (ifp == NULL) {
                if (ctx.ifc == 0) {
                        int loglevel;
@@ -2735,24 +2677,22 @@ exit1:
        if (ps_stopwait(&ctx) != EXIT_SUCCESS)
                i = EXIT_FAILURE;
 #endif
-       if (ctx.options & DHCPCD_STARTED && !(ctx.options & DHCPCD_FORKED)) {
+       if (ctx.options & DHCPCD_STARTED && !(ctx.options & DHCPCD_FORKED))
                loginfox(PACKAGE " exited");
-#ifdef USE_SIGNALS
-               /* Detach from the launch process.
-                * This *should* happen after we stop the root process,
-                * but for some reason non privsep builds get a zero length
-                * read in dhcpcd_fork_cb(). */
-               if (ctx.fork_fd != -1) {
-                       if (write(ctx.fork_fd, &i, sizeof(i)) == -1)
-                               logerr("%s: write", __func__);
-               }
-#endif
-       }
 #ifdef PRIVSEP
        if (ps_root_stop(&ctx) == -1)
                i = EXIT_FAILURE;
        eloop_free(ctx.ps_eloop);
 #endif
+
+#ifdef USE_SIGNALS
+       /* If still attached, detach from the launcher */
+       if (ctx.options & DHCPCD_STARTED && ctx.fork_fd != -1) {
+               if (write(ctx.fork_fd, &i, sizeof(i)) == -1)
+                       logerr("%s: write", __func__);
+       }
+#endif
+
        eloop_free(ctx.eloop);
        logclose();
        free(ctx.logfile);
@@ -2760,6 +2700,7 @@ exit1:
 #ifdef SETPROCTITLE_H
        setproctitle_fini();
 #endif
+
 #ifdef USE_SIGNALS
        if (ctx.options & (DHCPCD_FORKED | DHCPCD_PRIVSEP))
                _exit(i); /* so atexit won't remove our pidfile */

diff --git a/src/dhcpcd.h b/src/dhcpcd.h
index 918dc687e4513a2463632938adf6413d6b9e249e..7fee0604768f1d47093165d274bb80c5c5d651aa 100644 (file)
--- a/src/dhcpcd.h
+++ b/src/dhcpcd.h
@@ -116,10 +116,6 @@ 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 logging to stderr */
        int fork_fd;    /* FD for the fork init signal pipe */
        const char *cffile;
        unsigned long long options;

diff --git a/src/privsep.c b/src/privsep.c
index c3aeab8df021c4f7619b8d756b9fe9222dff162e..2decb8b8bc3ab84e76cf1df60adec0eee179894b 100644 (file)
--- a/src/privsep.c
+++ b/src/privsep.c
@@ -172,8 +172,7 @@ ps_dropprivs(struct dhcpcd_ctx *ctx)
         * Obviously this won't work if we are using a logfile
         * or redirecting stderr to a file. */
        if ((ctx->options & DHC_NOCHKIO) == DHC_NOCHKIO ||
-           (ctx->logfile == NULL &&
-           (!ctx->stderr_valid || isatty(STDERR_FILENO) == 1)))
+           (ctx->logfile == NULL && isatty(STDERR_FILENO) == 1))
        {
                if (setrlimit(RLIMIT_FSIZE, &rzero) == -1)
                        logerr("setrlimit RLIMIT_FSIZE");
@@ -305,14 +304,11 @@ 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)
+       if (caph_limit_stream(STDIN_FILENO, CAPH_READ | iebadf) == -1)
                error = -1;
-       if (ctx->stdout_valid &&
-           caph_limit_stream(STDOUT_FILENO, CAPH_WRITE | iebadf) == -1)
+       if (caph_limit_stream(STDOUT_FILENO, CAPH_WRITE | iebadf) == -1)
                error = -1;
-       if (ctx->stderr_valid &&
-           caph_limit_stream(STDERR_FILENO, CAPH_WRITE | iebadf) == -1)
+       if (caph_limit_stream(STDERR_FILENO, CAPH_WRITE | iebadf) == -1)
                error = -1;
 
        return error;