On Mon, Dec 26, 2016 at 04:54:54PM -0700, Theo de Raadt wrote:
> I think programs should only block the absolutely critical things, and this
> is overreach.

Yes, blocking SIGINT and SIGQUIT is not clever.  I thought there
were races with SIGCHLD and SIGTERM where only one process would
survive.  But races don't exist as parent and child communicate
over the socketpair and terminate if the other one dies.

I have seen problems with SIGHUP during debugging, instead of
restarting syslogd died.  I think this could also happen during
normal operation.

- edit syslog.conf
- send SIGHUP
- syslogd execs itself
- newsyslog rotates logfile and sends SIGHUP
- syslogd dies as signal handlers are not installed yet

So I suggest to block SIGHUP during startup.

bluhm

Index: usr.sbin/syslogd/privsep.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
retrieving revision 1.65
diff -u -p -r1.65 privsep.c
--- usr.sbin/syslogd/privsep.c  27 Dec 2016 19:16:24 -0000      1.65
+++ usr.sbin/syslogd/privsep.c  30 Dec 2016 18:24:00 -0000
@@ -175,6 +175,7 @@ priv_exec(char *conf, int numeric, int c
        struct stat cf_info, cf_stat;
        struct addrinfo hints, *res0;
        struct sigaction sa;
+       sigset_t sigmask;
 
        if (pledge("stdio rpath wpath cpath dns getpw sendfd id proc exec",
            NULL) == -1)
@@ -209,6 +210,10 @@ priv_exec(char *conf, int numeric, int c
        setproctitle("[priv]");
        logdebug("[priv]: fork+exec done\n");
 
+       sigemptyset(&sigmask);
+       if (sigprocmask(SIG_SETMASK, &sigmask, NULL) == -1)
+               err(1, "sigprocmask priv");
+
        if (stat(conf, &cf_info) < 0)
                err(1, "stat config file failed");
 
@@ -409,6 +414,10 @@ priv_exec(char *conf, int numeric, int c
                int status;
 
                waitpid(child_pid, &status, 0);
+               sigemptyset(&sigmask);
+               sigaddset(&sigmask, SIGHUP);
+               if (sigprocmask(SIG_SETMASK, &sigmask, NULL) == -1)
+                       err(1, "sigprocmask exec");
                execvp(argv[0], argv);
                err(1, "exec restart '%s' failed", argv[0]);
        }
Index: usr.sbin/syslogd/syslogd.c
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
retrieving revision 1.225
diff -u -p -r1.225 syslogd.c
--- usr.sbin/syslogd/syslogd.c  27 Dec 2016 19:16:24 -0000      1.225
+++ usr.sbin/syslogd/syslogd.c  30 Dec 2016 17:55:37 -0000
@@ -354,6 +354,7 @@ main(int argc, char *argv[])
        struct event    *ev_klog, *ev_sendsys, *ev_udp, *ev_udp6,
                        *ev_bind, *ev_listen, *ev_tls, *ev_unix,
                        *ev_hup, *ev_int, *ev_quit, *ev_term, *ev_mark;
+       sigset_t         sigmask;
        const char      *errstr;
        char            *p;
        int              ch, i;
@@ -361,6 +362,12 @@ main(int argc, char *argv[])
        int              fd_ctlsock, fd_klog, fd_sendsys, fd_bind, fd_listen;
        int             *fd_unix;
 
+       /* block signal until handler is set up */
+       sigemptyset(&sigmask);
+       sigaddset(&sigmask, SIGHUP);
+       if (sigprocmask(SIG_SETMASK, &sigmask, NULL) == -1)
+               err(1, "sigprocmask block");
+
        if ((path_unix = malloc(sizeof(*path_unix))) == NULL)
                err(1, "malloc %s", _PATH_LOG);
        path_unix[0] = _PATH_LOG;
@@ -839,6 +846,10 @@ main(int argc, char *argv[])
 
        logmsg(LOG_SYSLOG|LOG_INFO, "syslogd: start", LocalHostName, ADDDATE);
        logdebug("syslogd: started\n");
+
+       sigemptyset(&sigmask);
+       if (sigprocmask(SIG_SETMASK, &sigmask, NULL) == -1)
+               err(1, "sigprocmask unblock");
 
        event_dispatch();
        /* NOTREACHED */


Reply via email to