Hey,

I think setjmping from a signal handler to put a time limit on
pclose is too magical, especially when the alternative doesn't
require much more code.

But I do think putting a time limit on our wait for wall to do its
job is a reasonable precaution, though, so:

  * Replace popen with pipe/fork/execle,

  * fprintf -> dprintf, fwrite -> write, and

  * Strip SA_RESTART from SIGALRM's sigaction so we EINTR out
    of our wait(2) after 30 seconds.

We wait (instead of waitpid) to pick up any stragglers from prior
calls to timewarn() that we had to leave behind.

ok?

--
Scott Cheloha

Index: sbin/shutdown/shutdown.c
===================================================================
RCS file: /cvs/src/sbin/shutdown/shutdown.c,v
retrieving revision 1.47
diff -u -p -r1.47 shutdown.c
--- sbin/shutdown/shutdown.c    4 Feb 2018 04:28:41 -0000       1.47
+++ sbin/shutdown/shutdown.c    22 Feb 2018 19:26:27 -0000
@@ -40,7 +40,6 @@
 #include <fcntl.h>
 #include <sys/termios.h>
 #include <pwd.h>
-#include <setjmp.h>
 #include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
@@ -86,7 +85,7 @@ struct interval {
 
 static time_t offset, shuttime;
 static int dofast, dohalt, doreboot, dopower, dodump, mbuflen, nosync;
-static sig_atomic_t killflg;
+static sig_atomic_t killflg, timed_out;
 static char *whom, mbuf[BUFSIZ];
 
 void badtime(void);
@@ -268,8 +267,6 @@ loop(void)
        die_you_gravy_sucking_pig_dog();
 }
 
-static jmp_buf alarmbuf;
-
 static char *restricted_environ[] = {
        "PATH=" _PATH_STDPATH,
        NULL
@@ -279,59 +276,83 @@ void
 timewarn(int timeleft)
 {
        static char hostname[HOST_NAME_MAX+1];
-       char wcmd[PATH_MAX + 4];
-       extern char **environ;
        static int first;
-       FILE *pf;
+       int fd[2];
+       pid_t pid, wpid;
 
        if (!first++)
                (void)gethostname(hostname, sizeof(hostname));
 
-       /* undoc -n option to wall suppresses normal wall banner */
-       (void)snprintf(wcmd, sizeof(wcmd), "%s -n", _PATH_WALL);
-       environ = restricted_environ;
-       if (!(pf = popen(wcmd, "w"))) {
-               syslog(LOG_ERR, "shutdown: can't find %s: %m", _PATH_WALL);
+       if (pipe(fd) == -1) {
+               syslog(LOG_ERR, "pipe: %m");
+               return;
+       }
+       switch (pid = fork()) {
+       case -1:
+               syslog(LOG_ERR, "fork: %m");
+               close(fd[0]);
+               close(fd[1]);
                return;
+       case 0:
+               if (dup2(fd[0], STDIN_FILENO) == -1) {
+                       syslog(LOG_ERR, "dup2: %m");
+                       _exit(1);
+               }
+               close(fd[0]);
+               close(fd[1]);
+               /* wall(1)'s undocumented '-n' flag suppresses its banner. */
+               execle(_PATH_WALL, _PATH_WALL, "-n", (char *)NULL,
+                   restricted_environ);
+               syslog(LOG_ERR, "%s: %m", _PATH_WALL);
+               _exit(1);
+       default:
+               close(fd[0]);
        }
 
-       (void)fprintf(pf,
+       dprintf(fd[1],
            "\007*** %sSystem shutdown message from %s@%s ***\007\n",
            timeleft ? "": "FINAL ", whom, hostname);
 
        if (timeleft > 10*60) {
                struct tm *tm = localtime(&shuttime);
 
-               fprintf(pf, "System going down at %d:%02d\n\n",
+               dprintf(fd[1], "System going down at %d:%02d\n\n",
                    tm->tm_hour, tm->tm_min);
        } else if (timeleft > 59)
-               (void)fprintf(pf, "System going down in %d minute%s\n\n",
+               dprintf(fd[1], "System going down in %d minute%s\n\n",
                    timeleft / 60, (timeleft > 60) ? "s" : "");
        else if (timeleft)
-               (void)fprintf(pf, "System going down in 30 seconds\n\n");
+               dprintf(fd[1], "System going down in 30 seconds\n\n");
        else
-               (void)fprintf(pf, "System going down IMMEDIATELY\n\n");
+               dprintf(fd[1], "System going down IMMEDIATELY\n\n");
 
        if (mbuflen)
-               (void)fwrite(mbuf, sizeof(*mbuf), mbuflen, pf);
+               (void)write(fd[1], mbuf, mbuflen);
+       close(fd[1]);
 
        /*
-        * play some games, just in case wall doesn't come back
-        * probably unnecessary, given that wall is careful.
+        * If we wait longer than 30 seconds for wall(1) to exit we'll
+        * throw off our schedule.
         */
-       if (!setjmp(alarmbuf)) {
-               (void)signal(SIGALRM, timeout);
-               (void)alarm((u_int)30);
-               (void)pclose(pf);
-               (void)alarm((u_int)0);
-               (void)signal(SIGALRM, SIG_DFL);
+       signal(SIGALRM, timeout);
+       siginterrupt(SIGALRM, 1);
+       alarm(30);
+       while ((wpid = wait(NULL)) != pid && !timed_out)
+               continue;
+       alarm(0);
+       signal(SIGALRM, SIG_DFL);
+       if (timed_out) {
+               syslog(LOG_NOTICE,
+                   "wall[%ld] is taking too long to exit; continuing",
+                   (long)pid);
+               timed_out = 0;
        }
 }
 
 void
 timeout(int signo)
 {
-       longjmp(alarmbuf, 1);           /* XXX signal/longjmp resource leaks */
+       timed_out = 1;
 }
 
 void

Reply via email to