Re: svn commit: r230869 - head/usr.sbin/daemon
On 13.02.2012 0:56, Dmitry Morozovsky wrote: On Mon, 13 Feb 2012, Andrey Zonov wrote: [snip] Please don't. Even if you can't write the pidfile, you should run the service. The same as for pidfile_open() failure as documented in example. Feel free to warn about problem with writing to pidfile, but don't treat it as critial error. The problem is the following you cannot stop such a service with standard rc.d script and empty pidfile. As for me, unstoppable (via standard way) service is at least slightly better than unstartable. OK, another solution for this problem is do not automatically remove pidfile when pidfile_write() fails. I can explain this. Sometimes daemons crash and I want to restart them. I use cron for this purpose like this: */5 * * * * root /usr/local/etc/rc.d/mydaemon status > /dev/null || /usr/local/etc/rc.d/mydaemon start and if mydaemon doesn't listen any socket or pidfile_write() fails and remove pidfile (it doesn't hold lock on it, in fact) mydaemon will start. If you have other solution -- welcome. -- Andrey Zonov ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Feb 12, 2012 1:32 PM, "Mikolaj Golub" wrote: > > > On Sun, 12 Feb 2012 12:56:58 -0800 Jos Backus wrote: > > JB> Right. So why not add a Unix socket listener to daemon(8) so the rc.d > JB> script can send commands over the socket instead of using the pidfile? > JB> This is what supervise/svc let you do today. > > JB> I don't understand why this solution isn't obvious once you are > JB> committed to running daemon(8) for each service anyway. And then you > JB> don't need pidfiles because now you have a much better, standard > JB> control interface (sending commands to daemon(8) and gathering > JB> responses). > > Why do you think one is committed to running daemon(8) for each service? > daemon(8) is for a program that can't daemonize itself and you want an easy > way to run it detached from a terminal. And have an easy way to integrate it > in rc(8) using rc.subr(8). And rc.subr(8) knows about pidfiles but knows > nothing about unix sockets. I realize that. But ISTR someone mentioned earlier keeping daemon(8) running to keep the pidfile open or something to that effect. > > Although I don't say that the idea to use a socket file for monitoring and > control is bad in general. Right. This approach has a number of benefits. I emailed the daemontools- encore author about including it in FreeBSD but so far he hasn't responded. Jos > > -- > Mikolaj Golub ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sun, 12 Feb 2012 12:56:58 -0800 Jos Backus wrote: JB> Right. So why not add a Unix socket listener to daemon(8) so the rc.d JB> script can send commands over the socket instead of using the pidfile? JB> This is what supervise/svc let you do today. JB> I don't understand why this solution isn't obvious once you are JB> committed to running daemon(8) for each service anyway. And then you JB> don't need pidfiles because now you have a much better, standard JB> control interface (sending commands to daemon(8) and gathering JB> responses). Why do you think one is committed to running daemon(8) for each service? daemon(8) is for a program that can't daemonize itself and you want an easy way to run it detached from a terminal. And have an easy way to integrate it in rc(8) using rc.subr(8). And rc.subr(8) knows about pidfiles but knows nothing about unix sockets. Although I don't say that the idea to use a socket file for monitoring and control is bad in general. -- Mikolaj Golub ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Mon, 13 Feb 2012, Andrey Zonov wrote: [snip] > > Please don't. Even if you can't write the pidfile, you should run the > > service. The same as for pidfile_open() failure as documented in > > example. Feel free to warn about problem with writing to pidfile, but > > don't treat it as critial error. > > The problem is the following you cannot stop such a service with standard rc.d > script and empty pidfile. As for me, unstoppable (via standard way) service is at least slightly better than unstartable. -- Sincerely, D.Marck [DM5020, MCK-RIPE, DM3-RIPN] [ FreeBSD committer: ma...@freebsd.org ] *** Dmitry Morozovsky --- D.Marck --- Wild Woozle --- ma...@rinet.ru *** ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sun, Feb 12, 2012 at 12:41 PM, Andrey Zonov wrote: > On 13.02.2012 0:02, Pawel Jakub Dawidek wrote: >> >> On Sun, Feb 12, 2012 at 09:06:55PM +0200, Mikolaj Golub wrote: >>> >>> AZ> Check return code from pidfile_write() function. I saw many times >>> AZ> when pid could not be written because of there is not enough free >>> AZ> space (but file was created). Unfortunately, I have no suggestions >>> AZ> how to handle this properly. >>> >>> We could return with error in this case (for me this almost the same as >>> if we >>> don't create file at all). But if we check pidfile_write() status we >>> should >>> resign the pidfile_write() feature that allows to pass NULL pidfh and >>> check if >>> pidfile option is used. Something like in this patch: >>> >>> http://people.freebsd.org/~trociny/daemon/daemon.pidfile_write.1.patch >>> >>> Not sure I should commit this though. >> >> >> Please don't. Even if you can't write the pidfile, you should run the >> service. The same as for pidfile_open() failure as documented in >> example. Feel free to warn about problem with writing to pidfile, but >> don't treat it as critial error. > > > The problem is the following you cannot stop such a service with standard > rc.d script and empty pidfile. Right. So why not add a Unix socket listener to daemon(8) so the rc.d script can send commands over the socket instead of using the pidfile? This is what supervise/svc let you do today. I don't understand why this solution isn't obvious once you are committed to running daemon(8) for each service anyway. And then you don't need pidfiles because now you have a much better, standard control interface (sending commands to daemon(8) and gathering responses). Jos >> >> We can also add such a warning to the example in the manual page. >> > > -- > Andrey Zonov > > ___ > svn-src-head@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/svn-src-head > To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org" -- Jos Backus jos at catnook.com ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On 13.02.2012 0:02, Pawel Jakub Dawidek wrote: On Sun, Feb 12, 2012 at 09:06:55PM +0200, Mikolaj Golub wrote: AZ> Check return code from pidfile_write() function. I saw many times AZ> when pid could not be written because of there is not enough free AZ> space (but file was created). Unfortunately, I have no suggestions AZ> how to handle this properly. We could return with error in this case (for me this almost the same as if we don't create file at all). But if we check pidfile_write() status we should resign the pidfile_write() feature that allows to pass NULL pidfh and check if pidfile option is used. Something like in this patch: http://people.freebsd.org/~trociny/daemon/daemon.pidfile_write.1.patch Not sure I should commit this though. Please don't. Even if you can't write the pidfile, you should run the service. The same as for pidfile_open() failure as documented in example. Feel free to warn about problem with writing to pidfile, but don't treat it as critial error. The problem is the following you cannot stop such a service with standard rc.d script and empty pidfile. We can also add such a warning to the example in the manual page. -- Andrey Zonov ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sat, Feb 11, 2012 at 12:35:27PM +0200, Mikolaj Golub wrote: > Thank you. Here are the patches I would like to commit if there are no > objections or other suggestions: > > http://people.freebsd.org/~trociny/daemon/daemon.spawn.1.patch The patch looks good to me. I'd just fix style nit: please compare 'pidfile' with NULL, don't treat it as bool. > http://people.freebsd.org/~trociny/daemon/daemon.SIGTERM.1.patch > http://people.freebsd.org/~trociny/daemon/daemon.restart.1.patch I'd prefer not to be listed in 'Discussed by' for those two patches. As I said before, I'm not convinced those are desired functionalities. I don't object strongly against them, but having my name there suggests we reached some consensus and we didn't:) -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpSrQQzxzFze.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sun, Feb 12, 2012 at 09:06:55PM +0200, Mikolaj Golub wrote: > AZ> Check return code from pidfile_write() function. I saw many times > AZ> when pid could not be written because of there is not enough free > AZ> space (but file was created). Unfortunately, I have no suggestions > AZ> how to handle this properly. > > We could return with error in this case (for me this almost the same as if we > don't create file at all). But if we check pidfile_write() status we should > resign the pidfile_write() feature that allows to pass NULL pidfh and check if > pidfile option is used. Something like in this patch: > > http://people.freebsd.org/~trociny/daemon/daemon.pidfile_write.1.patch > > Not sure I should commit this though. Please don't. Even if you can't write the pidfile, you should run the service. The same as for pidfile_open() failure as documented in example. Feel free to warn about problem with writing to pidfile, but don't treat it as critial error. We can also add such a warning to the example in the manual page. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpTbTNm19XRg.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sat, 11 Feb 2012 16:16:16 +0400 Andrey Zonov wrote: AZ> On 11.02.2012 14:35, Mikolaj Golub wrote: >> >> Thank you. Here are the patches I would like to commit if there are no >> objections or other suggestions: >> >> http://people.freebsd.org/~trociny/daemon/daemon.spawn.1.patch >> http://people.freebsd.org/~trociny/daemon/daemon.SIGTERM.1.patch >> http://people.freebsd.org/~trociny/daemon/daemon.restart.1.patch The restart patch has been updated: the variable initialization was lost when separting the patches. http://people.freebsd.org/~trociny/daemon/daemon.restart.2.patch >> AZ> There are two more suggestions, if you don't mind. AZ> Use madvise(MADV_PROTECT). It would be useful because of the AZ> daemon(8) should not leak or eats much memory. I also thought about this and it looks like a good idea for me too. http://people.freebsd.org/~trociny/daemon/daemon.madvise.1.patch AZ> Check return code from pidfile_write() function. I saw many times AZ> when pid could not be written because of there is not enough free AZ> space (but file was created). Unfortunately, I have no suggestions AZ> how to handle this properly. We could return with error in this case (for me this almost the same as if we don't create file at all). But if we check pidfile_write() status we should resign the pidfile_write() feature that allows to pass NULL pidfh and check if pidfile option is used. Something like in this patch: http://people.freebsd.org/~trociny/daemon/daemon.pidfile_write.1.patch Not sure I should commit this though. -- Mikolaj Golub ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On 11.02.2012 14:35, Mikolaj Golub wrote: Thank you. Here are the patches I would like to commit if there are no objections or other suggestions: http://people.freebsd.org/~trociny/daemon/daemon.spawn.1.patch http://people.freebsd.org/~trociny/daemon/daemon.SIGTERM.1.patch http://people.freebsd.org/~trociny/daemon/daemon.restart.1.patch There are two more suggestions, if you don't mind. Use madvise(MADV_PROTECT). It would be useful because of the daemon(8) should not leak or eats much memory. Check return code from pidfile_write() function. I saw many times when pid could not be written because of there is not enough free space (but file was created). Unfortunately, I have no suggestions how to handle this properly. -- Andrey Zonov ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Wed, 8 Feb 2012 10:06:01 +0100 Pawel Jakub Dawidek wrote: PJD> On Wed, Feb 08, 2012 at 10:32:41AM +0200, Mikolaj Golub wrote: >> >> On Mon, 6 Feb 2012 23:17:43 +0100 Pawel Jakub Dawidek wrote: >> >> PJD> On Mon, Feb 06, 2012 at 11:46:24PM +0200, Mikolaj Golub wrote: >> >> >> Thanks. The updated version is attached. >> >> PJD> Looks good to me. >> >> Thanks. But I still think that adding some signal handling is a good idea. I >> agree that there may be no sense in trying to handle many different signals, >> but handling SIGTERM (software termination signal :-) nicely looks like a >> good >> thing. PJD> Ok:) In that case could you break you patch into one that only fixes the PJD> problem we discussed and the other that implements new functionality? >> This would solve problems like stale pid files after shutdown or orphaned >> programs (again with stale pid files and a possibility to start another >> instance) due to a user mistakenly terminated the supervising daemons. >> >> Also it is possible then to add "automatic restart" option, as Andrey has >> proposed. >> >> Here is the patch that does it. It also change proctitle to make >> identifying a >> a supervisor with its charge. PJD> I'd prefer to see more general solution to that problem, but I guess PJD> this can't hurt. I've only one suggestion based on my experience. PJD> Before you restart the program, wait for 1 second. This helps a lot when PJD> you have misbehaving program or some misconfiguration that make the PJD> process to exit immediately. >> A technical question concerning the patch :-). Does sombody know if >> sigwaitinfo() may be interrupted in my case and I should check for EINTR, >> as I >> do in the patch? PJD> Calling sigwaitinfo() with second argument equal to NULL is equivalent PJD> to calling sigwait(). The only difference is that sigwait() cannot be PJD> interrupted by signal, thus never sets errno to EINTR. Why not to use PJD> just that? Thank you. Here are the patches I would like to commit if there are no objections or other suggestions: http://people.freebsd.org/~trociny/daemon/daemon.spawn.1.patch http://people.freebsd.org/~trociny/daemon/daemon.SIGTERM.1.patch http://people.freebsd.org/~trociny/daemon/daemon.restart.1.patch -- Mikolaj Golub ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Wed, Feb 08, 2012 at 10:32:41AM +0200, Mikolaj Golub wrote: > > On Mon, 6 Feb 2012 23:17:43 +0100 Pawel Jakub Dawidek wrote: > > PJD> On Mon, Feb 06, 2012 at 11:46:24PM +0200, Mikolaj Golub wrote: > > >> Thanks. The updated version is attached. > > PJD> Looks good to me. > > Thanks. But I still think that adding some signal handling is a good idea. I > agree that there may be no sense in trying to handle many different signals, > but handling SIGTERM (software termination signal :-) nicely looks like a good > thing. Ok:) In that case could you break you patch into one that only fixes the problem we discussed and the other that implements new functionality? > This would solve problems like stale pid files after shutdown or orphaned > programs (again with stale pid files and a possibility to start another > instance) due to a user mistakenly terminated the supervising daemons. > > Also it is possible then to add "automatic restart" option, as Andrey has > proposed. > > Here is the patch that does it. It also change proctitle to make identifying a > a supervisor with its charge. I'd prefer to see more general solution to that problem, but I guess this can't hurt. I've only one suggestion based on my experience. Before you restart the program, wait for 1 second. This helps a lot when you have misbehaving program or some misconfiguration that make the process to exit immediately. > A technical question concerning the patch :-). Does sombody know if > sigwaitinfo() may be interrupted in my case and I should check for EINTR, as I > do in the patch? Calling sigwaitinfo() with second argument equal to NULL is equivalent to calling sigwait(). The only difference is that sigwait() cannot be interrupted by signal, thus never sets errno to EINTR. Why not to use just that? -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpyHj174aEEq.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Mon, 6 Feb 2012 23:17:43 +0100 Pawel Jakub Dawidek wrote:
PJD> On Mon, Feb 06, 2012 at 11:46:24PM +0200, Mikolaj Golub wrote:
>> Thanks. The updated version is attached.
PJD> Looks good to me.
Thanks. But I still think that adding some signal handling is a good idea. I
agree that there may be no sense in trying to handle many different signals,
but handling SIGTERM (software termination signal :-) nicely looks like a good
thing.
This would solve problems like stale pid files after shutdown or orphaned
programs (again with stale pid files and a possibility to start another
instance) due to a user mistakenly terminated the supervising daemons.
Also it is possible then to add "automatic restart" option, as Andrey has
proposed.
Here is the patch that does it. It also change proctitle to make identifying a
a supervisor with its charge.
On the other hand not being an active user of daemon(8) utility, I don't have
a strong opinion and would like to see what other people, especially those who
use it, think about this.
A technical question concerning the patch :-). Does sombody know if
sigwaitinfo() may be interrupted in my case and I should check for EINTR, as I
do in the patch?
--
Mikolaj Golub
Index: usr.sbin/daemon/daemon.c
===
--- usr.sbin/daemon/daemon.c (revision 231014)
+++ usr.sbin/daemon/daemon.c (working copy)
@@ -32,30 +32,36 @@
__FBSDID("$FreeBSD$");
#include
+#include
#include
#include
-#include
#include
#include
+#include
+#include
#include
#include
#include
+static void dummy_sighandler(int);
static void restrict_process(const char *);
+static int wait_child(pid_t, sigset_t *);
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, restart;
const char *pidfile, *user;
- pid_t otherpid;
+ pid_t otherpid, pid;
nochdir = noclose = 1;
+ restart = 0;
pidfile = user = NULL;
- while ((ch = getopt(argc, argv, "-cfp:u:")) != -1) {
+ while ((ch = getopt(argc, argv, "-cfp:ru:")) != -1) {
switch (ch) {
case 'c':
nochdir = 0;
@@ -66,6 +72,9 @@ main(int argc, char *argv[])
case 'p':
pidfile = optarg;
break;
+ case 'r':
+ restart = 1;
+ break;
case 'u':
user = optarg;
break;
@@ -79,14 +88,12 @@ main(int argc, char *argv[])
if (argc == 0)
usage();
- if (user != NULL)
- restrict_process(user);
-
+ pfh = NULL;
/*
* Try to open the pidfile before calling daemon(3),
* to be able to report the error intelligently
*/
- if (pidfile) {
+ if (pidfile != NULL) {
pfh = pidfile_open(pidfile, 0600, &otherpid);
if (pfh == NULL) {
if (errno == EEXIST) {
@@ -99,26 +106,83 @@ main(int argc, char *argv[])
if (daemon(nochdir, noclose) == -1)
err(1, NULL);
+ /*
+ * If pidfile or restart option is specified the daemon
+ * executes the command in a forked process and wait on child
+ * exit to remove the pidfile or restart the command.
+ * Normally we don't want the monitoring daemon to be
+ * terminated leaving the running process and the stale
+ * pidfile, so we catch SIGTERM and pass it to the children
+ * expecting to get SIGCHLD eventually.
+ */
+ pid = -1;
+ if (pidfile != NULL || restart) {
+ /*
+ * Restore default action for SIGTERM in case the
+ * parent process decided to ignore it.
+ */
+ 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, SIGTERM);
+ sigaddset(&mask, SIGCHLD);
+ if (sigprocmask(SIG_SETMASK, &mask, &oldmask) == -1)
+ err(1, "sigprocmask");
+restart:
+ /*
+ * Spawn a child to exec the command, so in the parent
+ * we could wait for it to exit and remove pidfile.
+ */
+ pid = fork();
+ if (pid == -1) {
+ pidfile_remove(pfh);
+ err(1, "fork");
+ }
+ }
+ if (pid <= 0) {
+ 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 */
- if (pidfile)
+ /* Now that we are the child, write out the pid. */
pidfile_write(pfh);
- execvp(argv[0], argv);
+ if (user != NULL)
+ restrict_process(user);
- /*
- * execvp() failed -- unlink pidfile if any, and
- * report the error
- */
- errcode = errno; /* Preserve errcode -- unlink may reset it */
- if (pidfile)
- pidfile_remove(pfh);
+ execvp(argv[0], argv);
- /* The child is now running, so the exit status doesn't matter. */
- errc(1, errcode, "%s", argv[0]);
+ /*
+ * execvp() failed -- report the error. The child is
+ * now running, so the exit status doesn't matter.
+ */
+ err(1
Re: svn commit: r230869 - head/usr.sbin/daemon
On Mon, Feb 06, 2012 at 11:46:24PM +0200, Mikolaj Golub wrote:
>
> On Mon, 6 Feb 2012 09:27:06 +0100 Pawel Jakub Dawidek wrote:
>
> PJD> For the patch itself.
>
> PJD> You don't have to have two separate cases depending on request for
> PJD> pidfile. You can specify NULL pfh to the pidfile functions.
> PJD> Even in example from the manual page when pfh is NULL there is a case
> PJD> where we warn, but continue execution and call pidfile functions.
> PJD> This should simplify the code.
>
> PJD> If you do that (actually even if you don't), remember to either use
> PJD> warn(3) before pidfile_remove(3) and exit(3) after or preserve errno
> PJD> before calling pidfile_remove(3), as pidfile_remove(3) can modify it if
> PJD> unlink(2) is unsuccessful or pfh is NULL.
>
> Thanks. The updated version is attached.
Looks good to me.
> Index: usr.sbin/daemon/daemon.c
> ===
> --- usr.sbin/daemon/daemon.c (revision 231014)
> +++ usr.sbin/daemon/daemon.c (working copy)
> @@ -32,6 +32,7 @@
> __FBSDID("$FreeBSD$");
>
> #include
> +#include
>
> #include
> #include
> @@ -43,15 +44,16 @@ __FBSDID("$FreeBSD$");
> #include
>
> static void restrict_process(const char *);
> +static void wait_child(pid_t pid);
> static void usage(void);
>
> int
> main(int argc, char *argv[])
> {
> struct pidfh *pfh = NULL;
> - int ch, nochdir, noclose, errcode;
> + int ch, nochdir, noclose;
> const char *pidfile, *user;
> - pid_t otherpid;
> + pid_t otherpid, pid;
>
> nochdir = noclose = 1;
> pidfile = user = NULL;
> @@ -79,9 +81,7 @@ main(int argc, char *argv[])
> if (argc == 0)
> usage();
>
> - if (user != NULL)
> - restrict_process(user);
> -
> + pfh = NULL;
> /*
>* Try to open the pidfile before calling daemon(3),
>* to be able to report the error intelligently
> @@ -100,22 +100,36 @@ main(int argc, char *argv[])
> if (daemon(nochdir, noclose) == -1)
> err(1, NULL);
>
> - /* Now that we are the child, write out the pid */
> - if (pidfile)
> + pid = 0;
> + if (pidfile) {
> + /*
> + * Spawn a child to exec the command, so in the parent
> + * we could wait for it to exit and remove pidfile.
> + */
> + pid = fork();
> + if (pid == -1) {
> + pidfile_remove(pfh);
> + err(1, "fork");
> + }
> + }
> + if (pid == 0) {
> + /* Now that we are the child, write out the pid. */
> pidfile_write(pfh);
>
> - execvp(argv[0], argv);
> + if (user != NULL)
> + restrict_process(user);
>
> - /*
> - * execvp() failed -- unlink pidfile if any, and
> - * report the error
> - */
> - errcode = errno; /* Preserve errcode -- unlink may reset it */
> - if (pidfile)
> - pidfile_remove(pfh);
> + execvp(argv[0], argv);
>
> - /* The child is now running, so the exit status doesn't matter. */
> - errc(1, errcode, "%s", argv[0]);
> + /*
> + * execvp() failed -- report the error. The child is
> + * now running, so the exit status doesn't matter.
> + */
> + err(1, "%s", argv[0]);
> + }
> + wait_child(pid);
> + pidfile_remove(pfh);
> + exit(0); /* Exit status does not metter. */
> }
>
> static void
> @@ -132,6 +146,19 @@ restrict_process(const char *user)
> }
>
> static void
> +wait_child(pid_t pid)
> +{
> + int status;
> +
> + while (waitpid(pid, &status, 0) == -1) {
> + if (errno != EINTR) {
> + warn("waitpid");
> + break;
> + }
> + }
> +}
> +
> +static void
> usage(void)
> {
> (void)fprintf(stderr,
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
pgpEIbsUS5eKZ.pgp
Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Feb 6, 2012, at 3:46 PM, Mikolaj Golub wrote:
>
> On Mon, 6 Feb 2012 09:27:06 +0100 Pawel Jakub Dawidek wrote:
>
> PJD> For the patch itself.
>
> PJD> You don't have to have two separate cases depending on request for
> PJD> pidfile. You can specify NULL pfh to the pidfile functions.
> PJD> Even in example from the manual page when pfh is NULL there is a case
> PJD> where we warn, but continue execution and call pidfile functions.
> PJD> This should simplify the code.
>
> PJD> If you do that (actually even if you don't), remember to either use
> PJD> warn(3) before pidfile_remove(3) and exit(3) after or preserve errno
> PJD> before calling pidfile_remove(3), as pidfile_remove(3) can modify it if
> PJD> unlink(2) is unsuccessful or pfh is NULL.
>
> Thanks. The updated version is attached.
>
> --
> Mikolaj Golub
>
> Index: usr.sbin/daemon/daemon.c
> ===
> --- usr.sbin/daemon/daemon.c (revision 231014)
> +++ usr.sbin/daemon/daemon.c (working copy)
> @@ -32,6 +32,7 @@
> __FBSDID("$FreeBSD$");
>
> #include
> +#include
>
> #include
> #include
> @@ -43,15 +44,16 @@ __FBSDID("$FreeBSD$");
> #include
>
> static void restrict_process(const char *);
> +static void wait_child(pid_t pid);
> static void usage(void);
>
> int
> main(int argc, char *argv[])
> {
> struct pidfh *pfh = NULL;
> - int ch, nochdir, noclose, errcode;
> + int ch, nochdir, noclose;
> const char *pidfile, *user;
> - pid_t otherpid;
> + pid_t otherpid, pid;
>
> nochdir = noclose = 1;
> pidfile = user = NULL;
> @@ -79,9 +81,7 @@ main(int argc, char *argv[])
> if (argc == 0)
> usage();
>
> - if (user != NULL)
> - restrict_process(user);
> -
> + pfh = NULL;
> /*
>* Try to open the pidfile before calling daemon(3),
>* to be able to report the error intelligently
> @@ -100,22 +100,36 @@ main(int argc, char *argv[])
> if (daemon(nochdir, noclose) == -1)
> err(1, NULL);
>
> - /* Now that we are the child, write out the pid */
> - if (pidfile)
> + pid = 0;
> + if (pidfile) {
> + /*
> + * Spawn a child to exec the command, so in the parent
> + * we could wait for it to exit and remove pidfile.
> + */
> + pid = fork();
> + if (pid == -1) {
> + pidfile_remove(pfh);
> + err(1, "fork");
> + }
> + }
> + if (pid == 0) {
> + /* Now that we are the child, write out the pid. */
> pidfile_write(pfh);
>
> - execvp(argv[0], argv);
> + if (user != NULL)
> + restrict_process(user);
>
> - /*
> - * execvp() failed -- unlink pidfile if any, and
> - * report the error
> - */
> - errcode = errno; /* Preserve errcode -- unlink may reset it */
> - if (pidfile)
> - pidfile_remove(pfh);
> + execvp(argv[0], argv);
>
> - /* The child is now running, so the exit status doesn't matter. */
> - errc(1, errcode, "%s", argv[0]);
> + /*
> + * execvp() failed -- report the error. The child is
> + * now running, so the exit status doesn't matter.
> + */
> + err(1, "%s", argv[0]);
> + }
> + wait_child(pid);
> + pidfile_remove(pfh);
> + exit(0); /* Exit status does not metter. */
> }
>
> static void
> @@ -132,6 +146,19 @@ restrict_process(const char *user)
> }
>
> static void
> +wait_child(pid_t pid)
> +{
> + int status;
> +
> + while (waitpid(pid, &status, 0) == -1) {
> + if (errno != EINTR) {
> + warn("waitpid");
> + break;
> + }
> + }
> +}
> +
> +static void
> usage(void)
> {
> (void)fprintf(stderr,
Generally looks good to me -- I had patches to do a similar change but yours
looks better. When I get a chance, I will test your change with the O_CLOEXEC
flag added back to the open() call in pidfile_open() -- I'm not sure how soon I
will be able to do that, though.
Guy
This message has been scanned by ComplianceSafe, powered by Palisade's
PacketSure.
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Mon, 6 Feb 2012 09:27:06 +0100 Pawel Jakub Dawidek wrote:
PJD> For the patch itself.
PJD> You don't have to have two separate cases depending on request for
PJD> pidfile. You can specify NULL pfh to the pidfile functions.
PJD> Even in example from the manual page when pfh is NULL there is a case
PJD> where we warn, but continue execution and call pidfile functions.
PJD> This should simplify the code.
PJD> If you do that (actually even if you don't), remember to either use
PJD> warn(3) before pidfile_remove(3) and exit(3) after or preserve errno
PJD> before calling pidfile_remove(3), as pidfile_remove(3) can modify it if
PJD> unlink(2) is unsuccessful or pfh is NULL.
Thanks. The updated version is attached.
--
Mikolaj Golub
Index: usr.sbin/daemon/daemon.c
===
--- usr.sbin/daemon/daemon.c (revision 231014)
+++ usr.sbin/daemon/daemon.c (working copy)
@@ -32,6 +32,7 @@
__FBSDID("$FreeBSD$");
#include
+#include
#include
#include
@@ -43,15 +44,16 @@ __FBSDID("$FreeBSD$");
#include
static void restrict_process(const char *);
+static void wait_child(pid_t pid);
static void usage(void);
int
main(int argc, char *argv[])
{
struct pidfh *pfh = NULL;
- int ch, nochdir, noclose, errcode;
+ int ch, nochdir, noclose;
const char *pidfile, *user;
- pid_t otherpid;
+ pid_t otherpid, pid;
nochdir = noclose = 1;
pidfile = user = NULL;
@@ -79,9 +81,7 @@ main(int argc, char *argv[])
if (argc == 0)
usage();
- if (user != NULL)
- restrict_process(user);
-
+ pfh = NULL;
/*
* Try to open the pidfile before calling daemon(3),
* to be able to report the error intelligently
@@ -100,22 +100,36 @@ main(int argc, char *argv[])
if (daemon(nochdir, noclose) == -1)
err(1, NULL);
- /* Now that we are the child, write out the pid */
- if (pidfile)
+ pid = 0;
+ if (pidfile) {
+ /*
+ * Spawn a child to exec the command, so in the parent
+ * we could wait for it to exit and remove pidfile.
+ */
+ pid = fork();
+ if (pid == -1) {
+ pidfile_remove(pfh);
+ err(1, "fork");
+ }
+ }
+ if (pid == 0) {
+ /* Now that we are the child, write out the pid. */
pidfile_write(pfh);
- execvp(argv[0], argv);
+ if (user != NULL)
+ restrict_process(user);
- /*
- * execvp() failed -- unlink pidfile if any, and
- * report the error
- */
- errcode = errno; /* Preserve errcode -- unlink may reset it */
- if (pidfile)
- pidfile_remove(pfh);
+ execvp(argv[0], argv);
- /* The child is now running, so the exit status doesn't matter. */
- errc(1, errcode, "%s", argv[0]);
+ /*
+ * execvp() failed -- report the error. The child is
+ * now running, so the exit status doesn't matter.
+ */
+ err(1, "%s", argv[0]);
+ }
+ wait_child(pid);
+ pidfile_remove(pfh);
+ exit(0); /* Exit status does not metter. */
}
static void
@@ -132,6 +146,19 @@ restrict_process(const char *user)
}
static void
+wait_child(pid_t pid)
+{
+ int status;
+
+ while (waitpid(pid, &status, 0) == -1) {
+ if (errno != EINTR) {
+ warn("waitpid");
+ break;
+ }
+ }
+}
+
+static void
usage(void)
{
(void)fprintf(stderr,
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On 06.02.2012 19:25, John Baldwin wrote: My expectation is that as long as parent process holds pidfile descriptor open and locked, the pidfile should remain locked even after fork(2)/execve(2). Worth checking, though. Yes, if the daemon process hung around that would work. Note that I think you would need to do a double-fork for that to work though since users expect daemon to return instantly and not need to be put in the background. It would be also nice to have an option for automatically respawn the child. This option has GNU version of daemon. What do you think? -- Andrey Zonov ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Monday, February 06, 2012 9:43:39 am Pawel Jakub Dawidek wrote: > On Mon, Feb 06, 2012 at 08:31:47AM -0600, Guy Helmer wrote: > > If my understanding of flock(2) semantics is correct, with open(2) O_CLOEXEC or fcntl(2) FD_CLOEXEC set on the pidfile, the closing of the pidfile file descriptor during an exec will result in loss of the lock on the pidfile regardless of whether daemon(8) hangs around to wait for the child exit. > > My expectation is that as long as parent process holds pidfile > descriptor open and locked, the pidfile should remain locked even after > fork(2)/execve(2). Worth checking, though. Yes, if the daemon process hung around that would work. Note that I think you would need to do a double-fork for that to work though since users expect daemon to return instantly and not need to be put in the background. -- John Baldwin ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Mon, Feb 06, 2012 at 08:31:47AM -0600, Guy Helmer wrote: > If my understanding of flock(2) semantics is correct, with open(2) O_CLOEXEC > or fcntl(2) FD_CLOEXEC set on the pidfile, the closing of the pidfile file > descriptor during an exec will result in loss of the lock on the pidfile > regardless of whether daemon(8) hangs around to wait for the child exit. My expectation is that as long as parent process holds pidfile descriptor open and locked, the pidfile should remain locked even after fork(2)/execve(2). Worth checking, though. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpHYfRAcwaGv.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Feb 5, 2012, at 3:39 AM, Pawel Jakub Dawidek wrote: > 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:~% > > Mikolaj, eventhough what we had in 8.2-STABLE looks correct, it also > isn't correct. > > Passing open descriptor to a process that doesn't expect that is bad > behaviour. If you pass, eg. open descriptor to a directory and the > process is using chroot(2) or jail(2) to sandbox itself it will be able > to escape from that sandbox. Passing descriptor to a file has smaller > security implication, but it is still wrong. For example hastd, as you > probably know, asserts, before sandboxing, that he knows about all open > descriptors - if there are some unknown descriptors open it won't run. > > Also, daemon was passing open descriptor to a pidfile that the child > process cannot clean up, because he doesn't know its name. This leaves > pidfile with stale PID in it once the process exits, which is also bad. > > In my opinion, to make daemon(8) work with pidfiles, it cannot exit > after executing the given command. It should stay around with pidfile > open and just wait for the child to exit. Once the child exits, it > should remove the pidfile and also exit. If my understanding of flock(2) semantics is correct, with open(2) O_CLOEXEC or fcntl(2) FD_CLOEXEC set on the pidfile, the closing of the pidfile file descriptor during an exec will result in loss of the lock on the pidfile regardless of whether daemon(8) hangs around to wait for the child exit. Guy This message has been scanned by ComplianceSafe, powered by Palisade's PacketSure. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Mon, Feb 06, 2012 at 09:39:47AM +0200, Mikolaj Golub wrote: > > On Sun, 5 Feb 2012 22:46:48 +0100 Pawel Jakub Dawidek wrote: > > PJD> On Sun, Feb 05, 2012 at 11:27:10PM +0200, Mikolaj Golub wrote: > >> Ok, using hastd code as a reference :-) here is my implementation. > > PJD> - I'd not pass selected signals to the child. The parent can still be > PJD> killed with a whole bunch of different signals that are not passed or > PJD> cannot be caught or the child process handle them gracefully. > PJD> Signals should be send to the PID from the pidfile anyway. If someone > PJD> is sending signals to the parent he has no right to expect well > PJD> behaviour from the parent. > > Well, sending a whole bunch of different signals to parent we might not expect > right behavior, but why not to provide it for the "standard" ones? E.g. on > shutdown init(8) will send SIGTERM and the daemon will gracefully exit > terminating the child and cleaning up the pidfile. If the the child process > does not handle SIGTERM gracefully I don't see much difference from having > only this one process alive or two (with its monitoring daemon). > > The pidfile is seen in ps(1) output for the daemon process, which allows to > identify the monitoring daemon with its child. Or we could change its > proctitle to something like "daemon: cmdname[pid]", similar to what sshd does. > So people would expect that terminating a daemon will terminate the process it > monitors. > > PJD> - Now that we handle the pidfile fully in the parent, I'd move dropping > PJD> provileges after fork(2) and pidfile_write(3). This way pidfiles will > PJD> always be created with root privileges and we can forget about all the > PJD> mess with pid directories, etc. > > PJD> - With the above you can wait for child to exit with simple wait(2). > > Yes, it looks like much simpler, see the attached patch. But I don't think I > like it much as it still looks like a half measure to me. I like this approach much better, as it is just simpler, but it is your call, Mikolaj. For the patch itself. You don't have to have two separate cases depending on request for pidfile. You can specify NULL pfh to the pidfile functions. Even in example from the manual page when pfh is NULL there is a case where we warn, but continue execution and call pidfile functions. This should simplify the code. If you do that (actually even if you don't), remember to either use warn(3) before pidfile_remove(3) and exit(3) after or preserve errno before calling pidfile_remove(3), as pidfile_remove(3) can modify it if unlink(2) is unsuccessful or pfh is NULL. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpG2XgVdkUJ2.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sun, 5 Feb 2012 22:46:48 +0100 Pawel Jakub Dawidek wrote:
PJD> On Sun, Feb 05, 2012 at 11:27:10PM +0200, Mikolaj Golub wrote:
>> Ok, using hastd code as a reference :-) here is my implementation.
PJD> - I'd not pass selected signals to the child. The parent can still be
PJD> killed with a whole bunch of different signals that are not passed or
PJD> cannot be caught or the child process handle them gracefully.
PJD> Signals should be send to the PID from the pidfile anyway. If someone
PJD> is sending signals to the parent he has no right to expect well
PJD> behaviour from the parent.
Well, sending a whole bunch of different signals to parent we might not expect
right behavior, but why not to provide it for the "standard" ones? E.g. on
shutdown init(8) will send SIGTERM and the daemon will gracefully exit
terminating the child and cleaning up the pidfile. If the the child process
does not handle SIGTERM gracefully I don't see much difference from having
only this one process alive or two (with its monitoring daemon).
The pidfile is seen in ps(1) output for the daemon process, which allows to
identify the monitoring daemon with its child. Or we could change its
proctitle to something like "daemon: cmdname[pid]", similar to what sshd does.
So people would expect that terminating a daemon will terminate the process it
monitors.
PJD> - Now that we handle the pidfile fully in the parent, I'd move dropping
PJD> provileges after fork(2) and pidfile_write(3). This way pidfiles will
PJD> always be created with root privileges and we can forget about all the
PJD> mess with pid directories, etc.
PJD> - With the above you can wait for child to exit with simple wait(2).
Yes, it looks like much simpler, see the attached patch. But I don't think I
like it much as it still looks like a half measure to me.
--
Mikolaj Golub
Index: usr.sbin/daemon/daemon.c
===
--- usr.sbin/daemon/daemon.c (revision 231060)
+++ usr.sbin/daemon/daemon.c (working copy)
@@ -32,6 +32,7 @@
__FBSDID("$FreeBSD$");
#include
+#include
#include
#include
@@ -49,9 +50,9 @@ int
main(int argc, char *argv[])
{
struct pidfh *pfh = NULL;
- int ch, nochdir, noclose, errcode;
+ int ch, nochdir, noclose, status;
const char *pidfile, *user;
- pid_t otherpid;
+ pid_t otherpid, pid;
nochdir = noclose = 1;
pidfile = user = NULL;
@@ -79,43 +80,61 @@ main(int argc, char *argv[])
if (argc == 0)
usage();
- 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);
+ if (user != NULL)
+ restrict_process(user);
+
+ 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
*/
- 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);
- /* Now that we are the child, write out the pid */
- if (pidfile)
+ pid = fork();
+ if (pid == -1) {
+ pidfile_remove(pfh);
+ err(1, "fork");
+ }
+ if (pid == 0) {
+ /* Now that we are the child, write out the pid. */
pidfile_write(pfh);
- execvp(argv[0], argv);
+ if (user != NULL)
+ restrict_process(user);
- /*
- * execvp() failed -- unlink pidfile if any, and
- * report the error
- */
- errcode = errno; /* Preserve errcode -- unlink may reset it */
- if (pidfile)
- pidfile_remove(pfh);
+ execvp(argv[0], argv);
- /* The child is now running, so the exit status doesn't matter. */
- errc(1, errcode, "%s", argv[0]);
+ /* execvp() failed. */
+ err(1, "%s", argv[0]);
+ }
+ (void)wait(&status);
+ pidfile_remove(pfh);
+ exit(0);
}
static void
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
Hi Pawel, On Feb 5, 2012 1:48 PM, "Pawel Jakub Dawidek" wrote: > > On Sun, Feb 05, 2012 at 11:27:10PM +0200, Mikolaj Golub wrote: > > Ok, using hastd code as a reference :-) here is my implementation. > > - I'd not pass selected signals to the child. The parent can still be > killed with a whole bunch of different signals that are not passed or > cannot be caught or the child process handle them gracefully. > Signals should be send to the PID from the pidfile anyway. If someone > is sending signals to the parent he has no right to expect well > behaviour from the parent. > > - Now that we handle the pidfile fully in the parent, I'd move dropping > provileges after fork(2) and pidfile_write(3). This way pidfiles will > always be created with root privileges and we can forget about all the > mess with pid directories, etc. > > - With the above you can wait for child to exit with simple wait(2). If you are going to wait for the child anyway, you are doing almost everything supervise does. All you now need is a Unix domain socket interface so you can receive commands in daemon(1), and run daemon(1) at boot. AND you can remove all the pidfile code :) Jos ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sun, Feb 05, 2012 at 11:27:10PM +0200, Mikolaj Golub wrote: > Ok, using hastd code as a reference :-) here is my implementation. - I'd not pass selected signals to the child. The parent can still be killed with a whole bunch of different signals that are not passed or cannot be caught or the child process handle them gracefully. Signals should be send to the PID from the pidfile anyway. If someone is sending signals to the parent he has no right to expect well behaviour from the parent. - Now that we handle the pidfile fully in the parent, I'd move dropping provileges after fork(2) and pidfile_write(3). This way pidfiles will always be created with root privileges and we can forget about all the mess with pid directories, etc. - With the above you can wait for child to exit with simple wait(2). -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpEewgyvkM0J.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
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
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
+#include
#include
#include
-#include
#include
#include
+#include
+#include
#include
#include
#include
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);
+ sigaddse
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sun, Feb 05, 2012 at 04:46:53AM -0800, Doug Barton wrote: > On 02/05/2012 01:59, Pawel Jakub Dawidek wrote: > > > I seem to miss positives of the other approach. Leaving stale PIDs in > > pidfile is something we should avoid at all costs, so recommending that > > in the manual page is not the best recommendation. > > Which is worse ... potentially stale pidfiles that get cleaned up at > every boot, or stale directories that never do? Every boot might be very rare situation on servers. Those directories should be cleaned when application is deinstalled and not when process exits. > I'm also not sure why you think this method will leave behind a stale > pidfile. The idea is that the pidfile is pre-created with the ownership > that daemon is going to su to, for the express purpose of allowing it to > delete the pidfile when the process exits. If you're saying that this > method doesn't work then please point out the problem ASAP because > numerous ports rc.d scripts do this now. Great, but this is not how UNIX permissions work. To remove directory entry you have to have rights to modify the directory. Having write permission to file within the directory won't allow you to remove it. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpxeCBmGNsRq.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On 02/05/2012 01:59, Pawel Jakub Dawidek wrote: > I seem to miss positives of the other approach. Leaving stale PIDs in > pidfile is something we should avoid at all costs, so recommending that > in the manual page is not the best recommendation. Which is worse ... potentially stale pidfiles that get cleaned up at every boot, or stale directories that never do? I'm also not sure why you think this method will leave behind a stale pidfile. The idea is that the pidfile is pre-created with the ownership that daemon is going to su to, for the express purpose of allowing it to delete the pidfile when the process exits. If you're saying that this method doesn't work then please point out the problem ASAP because numerous ports rc.d scripts do this now. Doug -- It's always a long day; 86400 doesn't fit into a short. Breadth of IT experience, and depth of knowledge in the DNS. Yours for the right price. :) http://SupersetSolutions.com/ signature.asc Description: OpenPGP digital signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sat, Feb 04, 2012 at 10:32:56AM -0600, Guy Helmer wrote: > > On Feb 4, 2012, at 1:42 AM, Pawel Jakub Dawidek wrote: > > > On Wed, Feb 01, 2012 at 04:41:00PM +, Guy Helmer wrote: > >> Author: ghelmer > >> Date: Wed Feb 1 16:40:59 2012 > >> New Revision: 230869 > >> URL: http://svn.freebsd.org/changeset/base/230869 > >> > >> Log: > >> Change the notes about the pidfile to include Doug's preference > >> for pre-creating the pidfile with appropriate owner and permissions. > >> > >> Requested by dougb > > > > Pre-creating pidfiles? That sounds weird. The common practise is to turn > > eg. /var/run/.pid into /var/run//pid where directory > > has appropriate permissions. Pre-creating pidfiles is simply wrong, > > because applications create pidfile on start and unlink it on exit. > > If application has no permission to remove files from /var/run/ it will > > leave pidfile with stale PID in it, which is bad. Changing application > > to truncate pidfile on exit instead of unlinking it also is a bad idea > > especially because there is working solution - pid directory. > > I prefer this approach, but dougb prefers the other approach. Each has > positives and negatives. I tried to accommodate both approaches. I seem to miss positives of the other approach. Leaving stale PIDs in pidfile is something we should avoid at all costs, so recommending that in the manual page is not the best recommendation. I for one would prefer to recommend against it. Even if pidfile is truncated on exit it still leave a mess in /var/run/. But currently it is not truncated on exit (pidfile(3) just unlinks the file, without truncating it first), so we end up with stale PIDs. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpdJ0GKSGer1.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
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:~% Mikolaj, eventhough what we had in 8.2-STABLE looks correct, it also isn't correct. Passing open descriptor to a process that doesn't expect that is bad behaviour. If you pass, eg. open descriptor to a directory and the process is using chroot(2) or jail(2) to sandbox itself it will be able to escape from that sandbox. Passing descriptor to a file has smaller security implication, but it is still wrong. For example hastd, as you probably know, asserts, before sandboxing, that he knows about all open descriptors - if there are some unknown descriptors open it won't run. Also, daemon was passing open descriptor to a pidfile that the child process cannot clean up, because he doesn't know its name. This leaves pidfile with stale PID in it once the process exits, which is also bad. In my opinion, to make daemon(8) work with pidfiles, it cannot exit after executing the given command. It should stay around with pidfile open and just wait for the child to exit. Once the child exits, it should remove the pidfile and also exit. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpbLwNve4Rfr.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sat, 4 Feb 2012 10:30:00 -0600 Guy Helmer wrote:
GH> On Feb 4, 2012, at 2:23 AM, Andrey Zonov wrote:
>> On 04.02.2012 11:42, Pawel Jakub Dawidek wrote:
>>> On Wed, Feb 01, 2012 at 04:41:00PM +, Guy Helmer wrote:
Author: ghelmer
Date: Wed Feb 1 16:40:59 2012
New Revision: 230869
URL: http://svn.freebsd.org/changeset/base/230869
Log:
Change the notes about the pidfile to include Doug's preference
for pre-creating the pidfile with appropriate owner and permissions.
Requested by dougb
>>>
>>> Pre-creating pidfiles? That sounds weird. The common practise is to turn
>>> eg. /var/run/.pid into /var/run//pid where directory
>>> has appropriate permissions. Pre-creating pidfiles is simply wrong,
>>> because applications create pidfile on start and unlink it on exit.
>>> If application has no permission to remove files from /var/run/ it will
>>> leave pidfile with stale PID in it, which is bad. Changing application
>>> to truncate pidfile on exit instead of unlinking it also is a bad idea
>>> especially because there is working solution - pid directory.
>>>
>>
>> Hi,
>>
>> There's even worse problem - kernel closes pidfile in execvp() because of
>> FD_CLOEXEC flag is set and daemon doesn't hold lock on pidfile.
>>
>> I reported about that earlier, but was ignored.
GH> I don't understand your concern about this -- the daemon(8) program
GH> exists to start a program that does not manage its own user authority or
GH> pid file, and it is inappropriate to leak the open pidfile descriptor to
GH> the program that daemon(8) execs.
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:~%
--
Mikolaj Golub
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Feb 4, 2012, at 2:23 AM, Andrey Zonov wrote: > On 04.02.2012 11:42, Pawel Jakub Dawidek wrote: >> On Wed, Feb 01, 2012 at 04:41:00PM +, Guy Helmer wrote: >>> Author: ghelmer >>> Date: Wed Feb 1 16:40:59 2012 >>> New Revision: 230869 >>> URL: http://svn.freebsd.org/changeset/base/230869 >>> >>> Log: >>> Change the notes about the pidfile to include Doug's preference >>> for pre-creating the pidfile with appropriate owner and permissions. >>> >>> Requested by dougb >> >> Pre-creating pidfiles? That sounds weird. The common practise is to turn >> eg. /var/run/.pid into /var/run//pid where directory >> has appropriate permissions. Pre-creating pidfiles is simply wrong, >> because applications create pidfile on start and unlink it on exit. >> If application has no permission to remove files from /var/run/ it will >> leave pidfile with stale PID in it, which is bad. Changing application >> to truncate pidfile on exit instead of unlinking it also is a bad idea >> especially because there is working solution - pid directory. >> > > Hi, > > There's even worse problem - kernel closes pidfile in execvp() because of > FD_CLOEXEC flag is set and daemon doesn't hold lock on pidfile. > > I reported about that earlier, but was ignored. I don't understand your concern about this -- the daemon(8) program exists to start a program that does not manage its own user authority or pid file, and it is inappropriate to leak the open pidfile descriptor to the program that daemon(8) execs. Guy This message has been scanned by ComplianceSafe, powered by Palisade's PacketSure. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Feb 4, 2012, at 1:42 AM, Pawel Jakub Dawidek wrote: > On Wed, Feb 01, 2012 at 04:41:00PM +, Guy Helmer wrote: >> Author: ghelmer >> Date: Wed Feb 1 16:40:59 2012 >> New Revision: 230869 >> URL: http://svn.freebsd.org/changeset/base/230869 >> >> Log: >> Change the notes about the pidfile to include Doug's preference >> for pre-creating the pidfile with appropriate owner and permissions. >> >> Requested by dougb > > Pre-creating pidfiles? That sounds weird. The common practise is to turn > eg. /var/run/.pid into /var/run//pid where directory > has appropriate permissions. Pre-creating pidfiles is simply wrong, > because applications create pidfile on start and unlink it on exit. > If application has no permission to remove files from /var/run/ it will > leave pidfile with stale PID in it, which is bad. Changing application > to truncate pidfile on exit instead of unlinking it also is a bad idea > especially because there is working solution - pid directory. I prefer this approach, but dougb prefers the other approach. Each has positives and negatives. I tried to accommodate both approaches. Guy This message has been scanned by ComplianceSafe, powered by Palisade's PacketSure. ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On 04.02.2012 11:42, Pawel Jakub Dawidek wrote: On Wed, Feb 01, 2012 at 04:41:00PM +, Guy Helmer wrote: Author: ghelmer Date: Wed Feb 1 16:40:59 2012 New Revision: 230869 URL: http://svn.freebsd.org/changeset/base/230869 Log: Change the notes about the pidfile to include Doug's preference for pre-creating the pidfile with appropriate owner and permissions. Requested by dougb Pre-creating pidfiles? That sounds weird. The common practise is to turn eg. /var/run/.pid into /var/run//pid where directory has appropriate permissions. Pre-creating pidfiles is simply wrong, because applications create pidfile on start and unlink it on exit. If application has no permission to remove files from /var/run/ it will leave pidfile with stale PID in it, which is bad. Changing application to truncate pidfile on exit instead of unlinking it also is a bad idea especially because there is working solution - pid directory. Hi, There's even worse problem - kernel closes pidfile in execvp() because of FD_CLOEXEC flag is set and daemon doesn't hold lock on pidfile. I reported about that earlier, but was ignored. -- Andrey Zonov ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r230869 - head/usr.sbin/daemon
On Wed, Feb 01, 2012 at 04:41:00PM +, Guy Helmer wrote: > Author: ghelmer > Date: Wed Feb 1 16:40:59 2012 > New Revision: 230869 > URL: http://svn.freebsd.org/changeset/base/230869 > > Log: > Change the notes about the pidfile to include Doug's preference > for pre-creating the pidfile with appropriate owner and permissions. > > Requested by dougb Pre-creating pidfiles? That sounds weird. The common practise is to turn eg. /var/run/.pid into /var/run//pid where directory has appropriate permissions. Pre-creating pidfiles is simply wrong, because applications create pidfile on start and unlink it on exit. If application has no permission to remove files from /var/run/ it will leave pidfile with stale PID in it, which is bad. Changing application to truncate pidfile on exit instead of unlinking it also is a bad idea especially because there is working solution - pid directory. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpmTeqIOkaMr.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
Thanks! Sorry I didn't get a chance to reply to your last message, totally buried lately. Doug On 02/01/2012 08:41, Guy Helmer wrote: > Author: ghelmer > Date: Wed Feb 1 16:40:59 2012 > New Revision: 230869 > URL: http://svn.freebsd.org/changeset/base/230869 > > Log: > Change the notes about the pidfile to include Doug's preference > for pre-creating the pidfile with appropriate owner and permissions. > > Requested by dougb > > Modified: > head/usr.sbin/daemon/daemon.8 > > Modified: head/usr.sbin/daemon/daemon.8 > == > --- head/usr.sbin/daemon/daemon.8 Wed Feb 1 15:57:49 2012 > (r230868) > +++ head/usr.sbin/daemon/daemon.8 Wed Feb 1 16:40:59 2012 > (r230869) > @@ -61,8 +61,9 @@ using the > functionality. > If the > .Fl u > -option is used, the directory to contain the pidfile must be writable > -by the specified user. > +option is used, either the pidfile needs to have been pre-created > +with appropriate ownership and permissions, or the directory to contain > +the pidfile must be writable by the specified user. > Note, that the file will be created shortly before the process is > actually executed, and will remain after the process exits (although > it will be removed if the execution fails). > -- It's always a long day; 86400 doesn't fit into a short. Breadth of IT experience, and depth of knowledge in the DNS. Yours for the right price. :) http://SupersetSolutions.com/ ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
svn commit: r230869 - head/usr.sbin/daemon
Author: ghelmer Date: Wed Feb 1 16:40:59 2012 New Revision: 230869 URL: http://svn.freebsd.org/changeset/base/230869 Log: Change the notes about the pidfile to include Doug's preference for pre-creating the pidfile with appropriate owner and permissions. Requested by dougb Modified: head/usr.sbin/daemon/daemon.8 Modified: head/usr.sbin/daemon/daemon.8 == --- head/usr.sbin/daemon/daemon.8 Wed Feb 1 15:57:49 2012 (r230868) +++ head/usr.sbin/daemon/daemon.8 Wed Feb 1 16:40:59 2012 (r230869) @@ -61,8 +61,9 @@ using the functionality. If the .Fl u -option is used, the directory to contain the pidfile must be writable -by the specified user. +option is used, either the pidfile needs to have been pre-created +with appropriate ownership and permissions, or the directory to contain +the pidfile must be writable by the specified user. Note, that the file will be created shortly before the process is actually executed, and will remain after the process exits (although it will be removed if the execution fails). ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
