[RFC,v2,2/7] misc: Ignore SIGHUP in daemon () while forking

Message ID 20230419160207.65988-3-bugaevc@gmail.com
State Superseded
Headers
Series O_IGNORE_CTTY everywhere & misc fixes |

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent

Commit Message

Sergey Bugaev April 19, 2023, 4:02 p.m. UTC
  Under certain conditions, SIGHUP will be sent to the child process when
the parent process (the one calling daemon ()) terminates -- namely, if
the parent process was the session leader and the child process was in
the same session (which will be the case after fork and until setsid)
and in the foreground process group. To prevent this SIGHUP from killing
the child, temporarily ignore it. Once we leave the parent's session, we
can restore the original SIGHUP sigaction.

Or that's what you'd hope would happen. Unfortunately for us, nothing
guarantess that signal delivery is synchronous. This means that a SIGHUP
may be generated and enqueued for the child process while it's still a
member of the original session, but only delivered to it some time
later, once it has already left the session and restored the original
sigaction.

Still, on many systems signal delivery is synchronous enough, and all
pending signals will get reliably delivered upon performing a syscall,
specifically setsid () in this case, so this change is still worth it.

Also, do not ignore erros from chdir ("/").

Suggested-by: Cristian Rodríguez <crrodriguez@opensuse.org>
Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>
---
 misc/daemon.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)
  

Comments

Adhemerval Zanella Netto April 21, 2023, 12:55 p.m. UTC | #1
On 19/04/23 13:02, Sergey Bugaev wrote:
> Under certain conditions, SIGHUP will be sent to the child process when
> the parent process (the one calling daemon ()) terminates -- namely, if
> the parent process was the session leader and the child process was in
> the same session (which will be the case after fork and until setsid)
> and in the foreground process group. To prevent this SIGHUP from killing
> the child, temporarily ignore it. Once we leave the parent's session, we
> can restore the original SIGHUP sigaction.
> 
> Or that's what you'd hope would happen. Unfortunately for us, nothing
> guarantess that signal delivery is synchronous. This means that a SIGHUP
> may be generated and enqueued for the child process while it's still a
> member of the original session, but only delivered to it some time
> later, once it has already left the session and restored the original
> sigaction.
> 
> Still, on many systems signal delivery is synchronous enough, and all
> pending signals will get reliably delivered upon performing a syscall,
> specifically setsid () in this case, so this change is still worth it.
> 
> Also, do not ignore erros from chdir ("/").
> 
> Suggested-by: Cristian Rodríguez <crrodriguez@opensuse.org>
> Signed-off-by: Sergey Bugaev <bugaevc@gmail.com>

Wouldn't a double fork avoid binding the daemon process to terminal to
reacquire a controlling terminal, thus avoid SIGHUP? This is BZ#19144 [1],
and this is being used by various libc implementations.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=19144

> ---
>  misc/daemon.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/misc/daemon.c b/misc/daemon.c
> index 14577e40..58dde4f0 100644
> --- a/misc/daemon.c
> +++ b/misc/daemon.c
> @@ -35,6 +35,7 @@ static char sccsid[] = "@(#)daemon.c	8.1 (Berkeley) 6/4/93";
>  #include <fcntl.h>
>  #include <paths.h>
>  #include <unistd.h>
> +#include <signal.h>
>  #include <sys/stat.h>
>  
>  #include <device-nrs.h>
> @@ -44,6 +45,14 @@ int
>  daemon (int nochdir, int noclose)
>  {
>    int fd;
> +  int set_sigaction;
> +  struct sigaction act, oact;
> +
> +  /* When the parent process exits, the child might get a SIGHUP if the parent
> +     was a session leader.  Arrange things so that it doesn't terminate us.  */
> +  memset (&act, 0, sizeof (act));
> +  act.sa_handler = SIG_IGN;
> +  set_sigaction = __libc_sigaction (SIGHUP, &act, &oact) == 0;
>  
>    switch (__fork ())
>      {
> @@ -60,8 +69,14 @@ daemon (int nochdir, int noclose)
>    if (__setsid () == -1)
>      return -1;
>  
> +  /* Now that we have left the parent's session, we should no longer be at
> +     risk of receiving SIGHUP because of the parent process exiting.  */
> +  if (__glibc_likely (set_sigaction))
> +    __libc_sigaction (SIGHUP, &oact, NULL);
> +
>    if (!nochdir)
> -    (void) __chdir ("/");
> +    if (__glibc_unlikely (__chdir ("/") == -1))
> +      return -1;
>  
>    if (!noclose)
>      {
  

Patch

diff --git a/misc/daemon.c b/misc/daemon.c
index 14577e40..58dde4f0 100644
--- a/misc/daemon.c
+++ b/misc/daemon.c
@@ -35,6 +35,7 @@  static char sccsid[] = "@(#)daemon.c	8.1 (Berkeley) 6/4/93";
 #include <fcntl.h>
 #include <paths.h>
 #include <unistd.h>
+#include <signal.h>
 #include <sys/stat.h>
 
 #include <device-nrs.h>
@@ -44,6 +45,14 @@  int
 daemon (int nochdir, int noclose)
 {
   int fd;
+  int set_sigaction;
+  struct sigaction act, oact;
+
+  /* When the parent process exits, the child might get a SIGHUP if the parent
+     was a session leader.  Arrange things so that it doesn't terminate us.  */
+  memset (&act, 0, sizeof (act));
+  act.sa_handler = SIG_IGN;
+  set_sigaction = __libc_sigaction (SIGHUP, &act, &oact) == 0;
 
   switch (__fork ())
     {
@@ -60,8 +69,14 @@  daemon (int nochdir, int noclose)
   if (__setsid () == -1)
     return -1;
 
+  /* Now that we have left the parent's session, we should no longer be at
+     risk of receiving SIGHUP because of the parent process exiting.  */
+  if (__glibc_likely (set_sigaction))
+    __libc_sigaction (SIGHUP, &oact, NULL);
+
   if (!nochdir)
-    (void) __chdir ("/");
+    if (__glibc_unlikely (__chdir ("/") == -1))
+      return -1;
 
   if (!noclose)
     {