Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

suppres SIGHUP on forking #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Maxime2
Copy link
Contributor

@Maxime2 Maxime2 commented Jul 30, 2015

We need to suppress SIGHUP until we set up as the parent's exit on forking would throw it.

@rustyrussell
Copy link
Owner

Maxim Zakharov [email protected] writes:

We need to suppress SIGHUP until we set up as the parent's exit on forking would throw it.

Would it? I'm surprised.

If so, we have a problem: the caller might expect a SIGHUP and hope to
handle it.

Cheers,
Rusty.

@Maxime2
Copy link
Contributor Author

Maxime2 commented Jul 31, 2015

But if the caller to do not expect it default action for SIGHUP is to terminate the process.

glibc's implementation of daemon() doesn't not block SIGHUP, while freebsd's does: https://svnweb.freebsd.org/base/stable/9/lib/libc/gen/daemon.c?view=markup

Also both versions call setsid() straight after fork(), so if you decide not to block SIGHUP it makes sense to move setsid() call up after fork() as it insulates of receiving SIGHUP from control terminal once executed.

@dgibson
Copy link
Collaborator

dgibson commented Jul 31, 2015

On Thu, Jul 30, 2015 at 05:57:40PM -0700, Maxim Zakharov wrote:

But if the caller to do not expect it default action for SIGHUP is to terminate the process.

glibc's implementation of daemon() doesn't not block SIGHUP, while freebsd's does: https://svnweb.freebsd.org/base/stable/9/lib/libc/gen/daemon.c?view=markup

Also both versions call setsid() straight after fork(), so if you
decide not to block SIGHUP it makes sense to move setsid() call up
after fork() as it insulates of receiving SIGHUP from control
terminal once executed.

Are you saying that moving the setsid() prevents the SIGHUP? If so,
that sounds like a better fix than actualy masking SIGHUP over the
transition.

David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT the other
| way around!
http://www.ozlabs.org/~dgibson

@Maxime2
Copy link
Contributor Author

Maxime2 commented Aug 1, 2015

Moving setsid() set the possibility of SIGHUP to a minimum, but not prevents it completely.

@rustyrussell
Copy link
Owner

Maxim Zakharov [email protected] writes:

But if the caller to do not expect it default action for SIGHUP is to terminate the process.

Ah, I see!

So, if the parent is already the session group leader, then it will
cause a SIGHUP to the child if it hasn't called setsid() yet.

My complaint with your version since it might miss a real SIGHUP. Most
users probably don't care, but it's worth handling:

(This is untested, but gives the idea...)

Cheers,
Rusty.

diff --git a/ccan/daemonize/daemonize.c b/ccan/daemonize/daemonize.c
index bd32ecb..7a762ea 100644
--- a/ccan/daemonize/daemonize.c
+++ b/ccan/daemonize/daemonize.c
@@ -11,13 +11,26 @@
bool daemonize(void)
{
pid_t pid;

  • int done[2];
  • /* Create a pipe to wait for child to do setsid() */
  • if (pipe(done) != 0)
  •   return false;
    
  • /* Separate from our parent via fork, so init inherits us. */
    if ((pid = fork()) < 0)
    return false;
  • /* use _exit() to avoid triggering atexit() processing */
  • if (pid != 0)
  • if (pid != 0) {
  •   char c;
    
  •   close(done[1]);
    
  •   if (read(done[0], &c, 1) != 1)
    
  •       return false;
    
  •   /\* use _exit() to avoid triggering atexit() processing */
    _exit(0);
    
  • }
  • close(done[0]);

/* Don't hold files open. /
close(STDIN_FILENO);
@@ -27,19 +40,24 @@ bool daemonize(void)
/
Many routines write to stderr; that can cause chaos if used

  • for something else, so set it here. */
    if (open("/dev/null", O_WRONLY) != 0)

  •   return false;
    
  •   _exit(1);
    

    if (dup2(0, STDERR_FILENO) != STDERR_FILENO)

  •   return false;
    
  •   _exit(1);
    

    close(0);

    /* Session leader so ^C doesn't whack us. */
    if (setsid() == (pid_t)-1)

  •       return false;
    
  •       _exit(1);
    

    /* Move off any mount points we might be in. */
    if (chdir("/") != 0)

  •   return false;
    
  •       _exit(1);
    

    /* Discard our parent's old-fashioned umask prejudices. */
    umask(0);
    +

  • /* Tell parent to exit: we're ready. */

  • if (write(done[1], "", 1) != 1)

  •   _exit(1);
    
  • close(done[1]);
    return true;
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants