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);
