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
[-- Attachment #1 --]
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
[-- Attachment #2 --]
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>