Skip site navigation (1)Skip section navigation (2)
Date:      Sun, 05 Feb 2012 23:27:10 +0200
From:      Mikolaj Golub <trociny@freebsd.org>
To:        Pawel Jakub Dawidek <pjd@FreeBSD.org>
Cc:        svn-src-head@FreeBSD.org, Guy Helmer <ghelmer@palisadesystems.com>, svn-src-all@FreeBSD.org, Andrey Zonov <andrey@zonov.org>, src-committers@FreeBSD.org
Subject:   Re: svn commit: r230869 - head/usr.sbin/daemon
Message-ID:  <86lioh7yz5.fsf@kopusha.home.net>
In-Reply-To: <20120205093938.GC30033@garage.freebsd.pl> (Pawel Jakub Dawidek's message of "Sun, 5 Feb 2012 10:39:38 %2B0100")
References:  <201202011641.q11Gf0j6095461@svn.freebsd.org> <20120204074201.GA1694@garage.freebsd.pl> <4F2CEB1D.10607@zonov.org> <27A0A960-F767-4D2C-BF3E-31F73FBF4E28@palisadesystems.com> <86zkcy5ur9.fsf@kopusha.home.net> <20120205093938.GC30033@garage.freebsd.pl>

next in thread | previous in thread | raw e-mail | index | archive | help
--=-=-=


On Sun, 5 Feb 2012 10:39:38 +0100 Pawel Jakub Dawidek wrote:

 PJD> On Sat, Feb 04, 2012 at 08:16:42PM +0200, Mikolaj Golub wrote:
 >> ref8-amd64:/home/trociny% uname -r
 >> 8.2-STABLE
 >> ref8-amd64:/home/trociny% daemon -p /tmp/sleep.pid sleep 10
 >> ref8-amd64:/home/trociny% daemon -p /tmp/sleep.pid sleep 10
 >> daemon: process already running, pid: 19799
 >> 
 >> kopusha:~% uname -r                         
 >> 10.0-CURRENT
 >> kopusha:~% daemon -p /tmp/sleep.pid sleep 10
 >> kopusha:~% daemon -p /tmp/sleep.pid sleep 10
 >> kopusha:~% 

 PJD> Mikolaj, eventhough what we had in 8.2-STABLE looks correct, it also
 PJD> isn't correct.

 PJD> Passing open descriptor to a process that doesn't expect that is bad
 PJD> behaviour. If you pass, eg. open descriptor to a directory and the
 PJD> process is using chroot(2) or jail(2) to sandbox itself it will be able
 PJD> to escape from that sandbox. Passing descriptor to a file has smaller
 PJD> security implication, but it is still wrong. For example hastd, as you
 PJD> probably know, asserts, before sandboxing, that he knows about all open
 PJD> descriptors - if there are some unknown descriptors open it won't run.

 PJD> Also, daemon was passing open descriptor to a pidfile that the child
 PJD> process cannot clean up, because he doesn't know its name. This leaves
 PJD> pidfile with stale PID in it once the process exits, which is also bad.

 PJD> In my opinion, to make daemon(8) work with pidfiles, it cannot exit
 PJD> after executing the given command. It should stay around with pidfile
 PJD> open and just wait for the child to exit. Once the child exits, it
 PJD> should remove the pidfile and also exit.

Ok, using hastd code as a reference :-) here is my implementation.

-- 
Mikolaj Golub


--=-=-=
Content-Type: text/x-patch
Content-Disposition: inline; filename=daemon.c.patch

Index: usr.sbin/daemon/daemon.c
===================================================================
--- usr.sbin/daemon/daemon.c	(revision 231014)
+++ usr.sbin/daemon/daemon.c	(working copy)
@@ -32,26 +32,31 @@
 __FBSDID("$FreeBSD$");
 
 #include <sys/param.h>
+#include <sys/wait.h>
 
 #include <err.h>
 #include <errno.h>
-#include <pwd.h>
 #include <libutil.h>
 #include <login_cap.h>
+#include <pwd.h>
+#include <signal.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <unistd.h>
 
 static void restrict_process(const char *);
+static void wait_child(pid_t, sigset_t *);
+static void dummy_sighandler(int);
 static void usage(void);
 
 int
 main(int argc, char *argv[])
 {
 	struct pidfh *pfh = NULL;
-	int ch, nochdir, noclose, errcode;
+	sigset_t mask, oldmask;
+	int ch, nochdir, noclose;
 	const char *pidfile, *user;
-	pid_t otherpid;
+	pid_t otherpid, pid;
 
 	nochdir = noclose = 1;
 	pidfile = user = NULL;
@@ -82,40 +87,96 @@ main(int argc, char *argv[])
 	if (user != NULL)
 		restrict_process(user);
 
+	if (pidfile == NULL) {
+		/*
+		 * This is a simple case. Daemonize and exec.
+		 */
+		if (daemon(nochdir, noclose) == -1)
+			err(1, NULL);
+
+		execvp(argv[0], argv);
+
+		/*
+		 * execvp() failed -- report the error. The child is
+		 * now running, so the exit status doesn't matter.
+		 */
+		err(1, "%s", argv[0]);
+	}
+
 	/*
 	 * Try to open the pidfile before calling daemon(3),
-	 * to be able to report the error intelligently
+	 * to be able to report the error intelligently.
 	 */
-	if (pidfile) {
-		pfh = pidfile_open(pidfile, 0600, &otherpid);
-		if (pfh == NULL) {
-			if (errno == EEXIST) {
-				errx(3, "process already running, pid: %d",
-				    otherpid);
-			}
-			err(2, "pidfile ``%s''", pidfile);
+	pfh = pidfile_open(pidfile, 0600, &otherpid);
+	if (pfh == NULL) {
+		if (errno == EEXIST) {
+			errx(3, "process already running, pid: %d",
+			    otherpid);
 		}
+		err(2, "pidfile ``%s''", pidfile);
 	}
-
 	if (daemon(nochdir, noclose) == -1)
 		err(1, NULL);
+	/*
+	 * We want to keep pidfile open while the command is running
+	 * and remove it on exit. So we execute the command in a
+	 * forked process and wait for the child to exit. We don't
+	 * want the waiting daemon to be killed leaving the running
+	 * process and the stale pidfile, so we pass received SIGHUP,
+	 * SIGINT and SIGTERM to the children expecting to get SIGCHLD
+	 * eventually.
+	 */
 
-	/* Now that we are the child, write out the pid */
-	if (pidfile)
-		pidfile_write(pfh);
-
-	execvp(argv[0], argv);
-
 	/*
-	 * execvp() failed -- unlink pidfile if any, and
-	 * report the error
+	 * Restore default actions for interesting signals in case
+	 * the parent process decided to ignore some of them.
 	 */
-	errcode = errno; /* Preserve errcode -- unlink may reset it */
-	if (pidfile)
+	if (signal(SIGHUP, SIG_DFL) == SIG_ERR)
+		err(1, "signal");
+	if (signal(SIGINT, SIG_DFL) == SIG_ERR)
+		err(1, "signal");
+	if (signal(SIGTERM, SIG_DFL) == SIG_ERR)
+		err(1, "signal");
+	/*
+	 * Because SIGCHLD is ignored by default, setup dummy handler
+	 * for it, so we can mask it.
+	 */
+	if (signal(SIGCHLD, dummy_sighandler) == SIG_ERR)
+		err(1, "signal");
+	/*
+	 * Block interesting signals.
+	 */
+	sigemptyset(&mask);
+	sigaddset(&mask, SIGHUP);
+	sigaddset(&mask, SIGINT);
+	sigaddset(&mask, SIGTERM);
+	sigaddset(&mask, SIGCHLD);
+	if (sigprocmask(SIG_SETMASK, &mask, &oldmask) == -1)
+		err(1, "sigprocmask");
+	/*
+	 * Fork a child to exec and wait until it exits to remove the
+	 * pidfile.
+	 */
+	pid = fork();
+	if (pid == -1) {
 		pidfile_remove(pfh);
+		err(1, "fork");
+	}
+	if (pid == 0) {
+		/* Restore old sigmask in the child. */
+		if (sigprocmask(SIG_SETMASK, &oldmask, NULL) == -1)
+			err(1, "sigprocmask");
+		/* Now that we are the child, write out the pid. */
+		pidfile_write(pfh);
 
-	/* The child is now running, so the exit status doesn't matter. */
-	errc(1, errcode, "%s", argv[0]);
+		execvp(argv[0], argv);
+
+		/* execvp() failed. */
+		err(1, "%s", argv[0]);
+	}
+	wait_child(pid, &mask);
+	pidfile_remove(pfh);
+	exit(0);
 }
 
 static void
@@ -132,6 +193,30 @@ restrict_process(const char *user)
 }
 
 static void
+wait_child(pid_t pid, sigset_t *mask)
+{
+	int signo;
+
+	while ((signo = sigwaitinfo(mask, NULL)) != -1) {
+		switch (signo) {
+		case SIGCHLD:
+			return;
+		default:
+			if (kill(pid, signo) == -1) {
+				warn("kill");
+				return;
+			}
+		}
+	}
+}
+
+static void
+dummy_sighandler(int sig __unused)
+{
+	/* Nothing to do. */
+}
+
+static void
 usage(void)
 {
 	(void)fprintf(stderr,

--=-=-=--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86lioh7yz5.fsf>