On Thu, Feb 22, 2018 at 02:13:16PM -0700, Todd C. Miller wrote: > On Thu, 22 Feb 2018 15:06:04 -0600, Scott Cheloha wrote: > > > Could that difference effect the behavior of the program in practice? > > I don't think so. > > > [...] > > > > So unless you or someone else has concerns about breakage I'll stick > > with dprintf. > > That's fine with me.
Cool. > > > 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]); > > > ... > > > > [...] > > > > 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? > > There's usually no guarantee that stdin, stdout and stderr are > actually open when you execute a program. The caller could have > closed them. On OpenBSD, when running a setuid program, the kernel > will open fds 0-2 if not already open (directing them to /dev/null). > > Since shutdown is setuid, the kernel will do the right thing but I > still think it is worth using the safer idiom. Alright, that makes sense, thanks for the explanation. -- Up-to-date diff attached. Concerns or oks from anyone else? -- 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 21:23:48 -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,84 @@ 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]); } - (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