On Thu, Feb 22, 2018 at 01:09:02PM -0700, Todd C. Miller wrote:
> On Thu, 22 Feb 2018 13:50:13 -0600, Scott Cheloha wrote:
> 
> > 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.
> 
> Agreed.
> 
> > [...]
> >
> >   * fprintf -> dprintf, fwrite -> write, and
> >
> > [...]
> 
> You could use fdopen() and keep using stdio but I suppose it doesn't
> really matter--dprintf() will effectively get you line buffering.

Could that difference effect the behavior of the program in practice?

Attached diff is using the file stream functions still, for comparison.

But the dprintf diff feels more natural; keeping the stream functions
means mucking with fdopen and the introduction of a label to handle the
failure case, or a new if clause, which makes the diff even larger.

So unless you or someone else has concerns about breakage I'll stick
with dprintf.

> The only thing that concerns me is the possibility of closing the
> wrong fd if stdin is not actually open (unlikely).  I prefer an
> idiom like the following:
> 
>       if (dup2(fd[0], STDIN_FILENO) == -1) {
>               syslog(LOG_ERR, "dup2: %m");
>               _exit(1);
>       }
>       if (fd[0] != STDIN_FILENO)
>               close(fd[0]);
>       ...

Ah, I was wondering why other programs around the tree did that.
The attached diff now does this, I believe.

How would stdin not be open in this case?  Or is it a more general
good-to-do-just-in-case when you're piping to stdin?

--
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 20:39:03 -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,20 +276,44 @@ 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);
+               }
+               if (fd[0] != STDIN_FILENO)
+                       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]);
+       }
+       if ((pf = fdopen(fd[1], "w")) == NULL) {
+               syslog(LOG_ERR, "fdopen: %m");
+               close(fd[1]);
+               goto wait;
        }
 
        (void)fprintf(pf,
@@ -314,24 +335,31 @@ timewarn(int timeleft)
 
        if (mbuflen)
                (void)fwrite(mbuf, sizeof(*mbuf), mbuflen, pf);
-
+       fclose(pf);
+wait:
        /*
-        * 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