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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
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 pgpmLT1UbA1q3.pgp Description: PGP signature
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 pgpk62KB0oDZR.pgp Description: PGP signature
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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r230869 - head/usr.sbin/daemon
On Sun, Feb 12, 2012 at 12:41 PM, Andrey Zonov and...@zonov.org 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-h...@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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r230869 - head/usr.sbin/daemon
On Feb 12, 2012 1:32 PM, Mikolaj Golub troc...@freebsd.org 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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
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 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 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
+ *
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 pgpaDTzpjwPcp.pgp Description: PGP signature
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 pgpBXYYzhyBCd.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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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 pgpMtLJPjhHTB.pgp Description: PGP signature
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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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 sys/param.h
+#include sys/wait.h
#include err.h
#include errno.h
@@ -43,15 +44,16 @@ __FBSDID($FreeBSD$);
#include unistd.h
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-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
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 sys/param.h
+#include sys/wait.h
#include err.h
#include errno.h
@@ -43,15 +44,16 @@ __FBSDID($FreeBSD$);
#include unistd.h
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-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
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 sys/param.h
+#include sys/wait.h
#include err.h
#include errno.h
@@ -43,15 +44,16 @@ __FBSDID($FreeBSD$);
#include unistd.h
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
pgpV0iSNHgCc4.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 pgpVVADgPlh78.pgp Description: PGP 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/name.pid into /var/run/name/pid where name 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 pgppgCgR3iIYZ.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 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 pgpBzEtwkYHv3.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 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);
+
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 pgposeBGithwi.pgp Description: PGP signature
Re: svn commit: r230869 - head/usr.sbin/daemon
Hi Pawel, On Feb 5, 2012 1:48 PM, Pawel Jakub Dawidek p...@freebsd.org 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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
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 sys/param.h
+#include sys/wait.h
#include err.h
#include errno.h
@@ -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-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-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/name.pid into /var/run/name/pid wherename 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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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/name.pid into /var/run/name/pid where name 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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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/name.pid into /var/run/name/pid wherename 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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
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/name.pid into /var/run/name/pid wherename 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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-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/name.pid into /var/run/name/pid where name 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 pgpGHuoFBMHb2.pgp Description: PGP signature
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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
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-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
