Do we want this syslogd feature that it only removes files it created
at startup?

On Sat, Feb 10, 2018 at 12:28:31AM +0100, Alexander Bluhm wrote:
> The file removal is part of a regression introduced by moving to
> fork+exec.  Before only files that were actually open were removed.
> Now the reexec parent does not know that and removes all files
> passed with -p, -a, or -s.
> 
> This can be fixed by passing the files that have been successfully
> opened with -R to the reexec parent.  While doing that I found a
> missing realpath(3) before chdir(2).  It is not clever to remove
> relative files in / that were created somewhere else.
> 
> Instead of using the global variables nunix, path_unix, and
> path_ctlsock I pass the files to be removed to the parent explicitly.
> 
> When I started syslogd -d I noticed that it was doing almost what
> you want.  It checks whether a unix domain socket is used by another
> server.  I guess that this is only done in debug mode to debug
> syslogd while another instance is running.
> 
> I consider it a bad idea to change behavior in debug mode.  That
> makes debugging real problems harder.  So let's rearrange the
> existing connect(2) code in a way that only unconnected unix domain
> sockets are removed before creating a new server socket.
> 
> ok?
> 
> bluhm
> 
> Index: usr.sbin/syslogd/privsep.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/privsep.c,v
> retrieving revision 1.67
> diff -u -p -r1.67 privsep.c
> --- usr.sbin/syslogd/privsep.c        5 Apr 2017 11:31:45 -0000       1.67
> +++ usr.sbin/syslogd/privsep.c        9 Feb 2018 21:48:21 -0000
> @@ -94,7 +94,8 @@ static void must_write(int, void *, size
>  static int  may_read(int, void *, size_t);
>  
>  void
> -priv_init(int lockfd, int nullfd, int argc, char *argv[])
> +priv_init(int lockfd, int nullfd, int nremove, char *path_remove[],
> +    int argc, char *argv[])
>  {
>       int i, socks[2];
>       struct passwd *pw;
> @@ -135,6 +136,13 @@ priv_init(int lockfd, int nullfd, int ar
>               execpath = argv[0];
>       else if ((execpath = realpath(argv[0], NULL)) == NULL)
>               err(1, "realpath %s", argv[0]);
> +     for (i = 0; i < nremove; i++) {
> +             char *tmp;
> +
> +             if ((tmp = realpath(path_remove[i], NULL)) == NULL)
> +                     err(1, "remove path %s", path_remove[i]);
> +             path_remove[i] = tmp;
> +     }
>       if (chdir("/") != 0)
>               err(1, "chdir /");
>  
> @@ -153,11 +161,16 @@ priv_init(int lockfd, int nullfd, int ar
>               err(1, "closefrom 4 failed");
>  
>       snprintf(childnum, sizeof(childnum), "%d", child_pid);
> -     if ((privargv = reallocarray(NULL, argc + 3, sizeof(char *))) == NULL)
> +     if ((privargv = reallocarray(NULL, argc + 2 * nremove + 3,
> +         sizeof(char *))) == NULL)
>               err(1, "alloc priv argv failed");
>       privargv[0] = execpath;
>       for (i = 1; i < argc; i++)
>               privargv[i] = argv[i];
> +     while (nremove > 0) {
> +             privargv[i++] = "-R";
> +             privargv[i++] = path_remove[--nremove];
> +     }
>       privargv[i++] = "-P";
>       privargv[i++] = childnum;
>       privargv[i++] = NULL;
> @@ -166,7 +179,8 @@ priv_init(int lockfd, int nullfd, int ar
>  }
>  
>  __dead void
> -priv_exec(char *conf, int numeric, int child, int argc, char *argv[])
> +priv_exec(char *conf, int numeric, int child, int nremove, char 
> *path_remove[],
> +    int argc, char *argv[])
>  {
>       int i, fd, sock, cmd, addr_len, result, restart;
>       size_t path_len, protoname_len, hostname_len, servname_len;
> @@ -406,10 +420,8 @@ priv_exec(char *conf, int numeric, int c
>       close(sock);
>  
>       /* Unlink any domain sockets that have been opened */
> -     for (i = 0; i < nunix; i++)
> -             (void)unlink(path_unix[i]);
> -     if (path_ctlsock != NULL)
> -             (void)unlink(path_ctlsock);
> +     for (i = 0; i < nremove; i++)
> +             (void)unlink(path_remove[i]);
>  
>       if (restart) {
>               int status;
> Index: usr.sbin/syslogd/syslogd.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.c,v
> retrieving revision 1.254
> diff -u -p -r1.254 syslogd.c
> --- usr.sbin/syslogd/syslogd.c        24 Nov 2017 23:11:42 -0000      1.254
> +++ usr.sbin/syslogd/syslogd.c        9 Feb 2018 22:49:37 -0000
> @@ -214,8 +214,6 @@ char      *TypeNames[] = {
>  SIMPLEQ_HEAD(filed_list, filed) Files;
>  struct       filed consfile;
>  
> -int  nunix;                  /* Number of Unix domain sockets requested */
> -char **path_unix;            /* Paths to Unix domain sockets */
>  int  Debug;                  /* debug flag */
>  int  Foreground;             /* run in foreground, instead of daemonizing */
>  char LocalHostName[HOST_NAME_MAX+1]; /* our hostname */
> @@ -232,7 +230,6 @@ int       NoDNS = 0;              /* when true, refrain fr
>  int  ZuluTime = 0;           /* display date and time in UTC ISO format */
>  int  IncludeHostname = 0;    /* include RFC 3164 hostnames when forwarding */
>  int  Family = PF_UNSPEC;     /* protocol family, may disable IPv4 or IPv6 */
> -char *path_ctlsock = NULL;   /* Path to control socket */
>  
>  struct       tls *server_ctx;
>  struct       tls_config *client_config, *server_config;
> @@ -371,7 +368,9 @@ main(int argc, char *argv[])
>       int              ch, i;
>       int              lockpipe[2] = { -1, -1}, pair[2], nullfd, fd;
>       int              fd_ctlsock, fd_klog, fd_sendsys, *fd_bind, *fd_listen;
> -     int             *fd_tls, *fd_unix, nbind, nlisten, ntls;
> +     int             *fd_tls, *fd_unix;
> +     int              nunix, nremove, nbind, nlisten, ntls;
> +     char            *path_ctlsock, **path_unix, **path_remove;
>       char            **bind_host, **bind_port, **listen_host, **listen_port;
>       char            *tls_hostport, **tls_host, **tls_port;
>  
> @@ -381,10 +380,13 @@ main(int argc, char *argv[])
>       if (sigprocmask(SIG_SETMASK, &sigmask, NULL) == -1)
>               err(1, "sigprocmask block");
>  
> +     path_ctlsock = NULL;
>       if ((path_unix = malloc(sizeof(*path_unix))) == NULL)
>               err(1, "malloc %s", _PATH_LOG);
>       path_unix[0] = _PATH_LOG;
>       nunix = 1;
> +     path_remove = NULL;
> +     nremove = 0;
>  
>       bind_host = listen_host = tls_host = NULL;
>       bind_port = listen_port = tls_port = NULL;
> @@ -392,7 +394,7 @@ main(int argc, char *argv[])
>       nbind = nlisten = ntls = 0;
>  
>       while ((ch = getopt(argc, argv,
> -         "46a:C:c:dFf:hK:k:m:nP:p:rS:s:T:U:uVZ")) != -1) {
> +         "46a:C:c:dFf:hK:k:m:nP:p:R:rS:s:T:U:uVZ")) != -1) {
>               switch (ch) {
>               case '4':               /* disable IPv6 */
>                       Family = PF_INET;
> @@ -447,6 +449,12 @@ main(int argc, char *argv[])
>               case 'p':               /* path */
>                       path_unix[0] = optarg;
>                       break;
> +             case 'R':               /* used internally, remove at exit */
> +                     if ((path_remove = reallocarray(path_remove,
> +                         nremove + 1, sizeof(*path_remove))) == NULL)
> +                             err(1, "remove path %s", optarg);
> +                     path_remove[nremove++] = optarg;
> +                     break;
>               case 'r':
>                       Repeat++;
>                       break;
> @@ -497,7 +505,8 @@ main(int argc, char *argv[])
>       }
>  
>       if (PrivChild > 1)
> -             priv_exec(ConfFile, NoDNS, PrivChild, argc, argv);
> +             priv_exec(ConfFile, NoDNS, PrivChild, nremove, path_remove,
> +                 argc, argv);
>  
>       consfile.f_type = F_CONSOLE;
>       (void)strlcpy(consfile.f_un.f_fname, ctty,
> @@ -547,6 +556,10 @@ main(int argc, char *argv[])
>                       log_warnx("socket listen tls failed");
>       }
>  
> +     nremove = 0;
> +     if ((path_remove = reallocarray(path_remove, nunix + 1,
> +         sizeof(*path_remove))) == NULL)
> +             fatal("allocate remove path");
>       if ((fd_unix = reallocarray(NULL, nunix, sizeof(*fd_unix))) == NULL)
>               fatal("allocate unix fd");
>       for (i = 0; i < nunix; i++) {
> @@ -556,6 +569,7 @@ main(int argc, char *argv[])
>                               log_warnx("log socket %s failed", path_unix[i]);
>                       continue;
>               }
> +             path_remove[nremove++] = path_unix[i];
>               double_sockbuf(fd_unix[i], SO_RCVBUF);
>       }
>  
> @@ -574,6 +588,7 @@ main(int argc, char *argv[])
>               if (fd_ctlsock == -1) {
>                       log_warnx("control socket %s failed", path_ctlsock);
>               } else {
> +                     path_remove[nremove++] = path_ctlsock;
>                       if (listen(fd_ctlsock, 5) == -1) {
>                               log_warn("listen control socket");
>                               close(fd_ctlsock);
> @@ -750,7 +765,7 @@ main(int argc, char *argv[])
>       }
>  
>       /* Privilege separation begins here */
> -     priv_init(lockpipe[1], nullfd, argc, argv);
> +     priv_init(lockpipe[1], nullfd, nremove, path_remove, argc, argv);
>  
>       if (pledge("stdio unix inet recvfd", NULL) == -1)
>               err(1, "pledge");
> @@ -2999,14 +3014,12 @@ unix_socket(char *path, int type, mode_t
>               return (-1);
>       }
>  
> -     if (Debug) {
> -             if (connect(fd, (struct sockaddr *)&s_un, sizeof(s_un)) == 0 ||
> -                 errno == EPROTOTYPE) {
> -                     close(fd);
> -                     errno = EISCONN;
> -                     log_warn("connect unix \"%s\"", path);
> -                     return (-1);
> -             }
> +     errno = EISCONN;
> +     if (connect(fd, (struct sockaddr *)&s_un, sizeof(s_un)) == 0 ||
> +         errno == EPROTOTYPE || errno == ENOTSOCK) {
> +             close(fd);
> +             log_warn("connect unix \"%s\"", path);
> +             return (-1);
>       }
>  
>       old_umask = umask(0177);
> Index: usr.sbin/syslogd/syslogd.h
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/usr.sbin/syslogd/syslogd.h,v
> retrieving revision 1.32
> diff -u -p -r1.32 syslogd.h
> --- usr.sbin/syslogd/syslogd.h        5 Oct 2017 16:15:24 -0000       1.32
> +++ usr.sbin/syslogd/syslogd.h        9 Feb 2018 21:17:49 -0000
> @@ -24,8 +24,8 @@
>  #include <stdarg.h>
>  
>  /* Privilege separation */
> -void  priv_init(int, int, int, char **);
> -__dead void priv_exec(char *, int, int, int, char **);
> +void  priv_init(int, int, int, char **, int, char **);
> +__dead void priv_exec(char *, int, int, int, char**, int, char **);
>  int   priv_open_tty(const char *);
>  int   priv_open_log(const char *);
>  FILE *priv_open_utmp(void);
> @@ -43,11 +43,6 @@ void ttymsg(struct iovec *, int, char *)
>  /* File descriptor send/recv */
>  void send_fd(int, int);
>  int  receive_fd(int);
> -
> -/* The list of domain sockets */
> -extern int nunix;
> -extern char **path_unix;
> -extern char *path_ctlsock;
>  
>  #define ERRBUFSIZE   256
>  void vlogmsg(int pri, const char *, const char *, va_list);

Reply via email to