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
