Subject: Re: [bug#41507] [PATCH Shepherd 2/2] shepherd: Use 'signalfd' when possible.

From: Ludovic Courtès <[email-masked]>
Date: Sat, 30 May 2020 19:44:12 +0200

Hello Mathieu!

Mathieu Othacehe <[email-masked]> skribis:

>> +        (begin
>> +          ;; Unblock any signals that might have been blocked by the parent
>> +          ;; process.
>> +          (unblock-signals (list SIGCHLD SIGINT SIGHUP SIGTERM))
>
> This made me realize that we may want to disable/reset SIGINT and
> SIGHUP, in the same way that we do for SIGTERM? This is not related to
> your patch anyway.

I looked into this and came up with the patches below.

What it does is unconditionally block these four signals before forking.
Then the child process installs SIG_DFL handlers for them and unblocks
them.

That way, the child is guaranteed to never execute the original
handlers, and neither the parent nor the child misses any of these
signals (previously, the temporary (sigaction SIGTERM SIG_DFL)
introduced a window during which shepherd could be killed by a SIGTERM
instead of handling it gracefully.)

WDYT?

(I also thought about using ‘call-with-blocked-asyncs’ instead of
‘with-blocked-signals’.  That would have prevented signal handling at
the Scheme level from happening, but the unhandled signals in the child
would be lost.)

If it works for you, I’ll do some more testing, and then hopefully we
can release 0.8.1 in the coming days!

Ludo’.

>From bc74b5e33625a082ad0d44fe4409d459222aa295 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <[email-masked]>
Date: Sat, 30 May 2020 17:44:07 +0200
Subject: [PATCH 1/3] system: 'sigprocmask' returns the previous set of blocked
 signals.

* modules/shepherd/system.scm.in (sigismember, sigset->list): New
procedures.
(sigprocmask): Return the old set of signals.
---
 modules/shepherd/system.scm.in | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/modules/shepherd/system.scm.in b/modules/shepherd/system.scm.in
index ac822f8..9c55c69 100644
--- a/modules/shepherd/system.scm.in
+++ b/modules/shepherd/system.scm.in
@@ -148,6 +148,11 @@ ctrlaltdel(8) and see kernel/reboot.c in Linux."
 (define sigaddset
   (syscall->procedure int "sigaddset" `(* ,int)))
 
+(define sigismember
+  (let ((proc (syscall->procedure int "sigismember" `(* ,int))))
+    (lambda (set signal)
+      (not (zero? (proc set signal))))))
+
 (define (sigset signals)
   "Return a pointer to a fresh 'sigset_t' for SIGNALS."
   (let ((set (allocate-sigset)))
@@ -155,6 +160,20 @@ ctrlaltdel(8) and see kernel/reboot.c in Linux."
     (for-each (cut sigaddset set <>) signals)
     set))
 
+(define sigset->list
+  (let ((all-signals
+         (filter integer?
+                 (module-map (lambda (symbol variable)
+                               (let ((str (symbol->string symbol)))
+                                 (and (string-prefix? "SIG" str)
+                                      (not (string-prefix? "SIG_" str))
+                                      (variable-ref variable))))
+                             (resolve-interface '(guile))))))
+    (lambda (set)
+      "Return the list of integers (signal numbers) corresponding to SET, a
+sigset pointer."
+      (filter (cut sigismember set <>) all-signals))))
+
 (define %sizeof-struct-signalfd-siginfo
   ;; Size of 'struct signalfd_siginfo' or zero if it doesn't exist, as is the
   ;; case on GNU/Hurd.
@@ -186,13 +205,17 @@ number of the signal received."
   (let ((proc (syscall->procedure int "pthread_sigmask" `(,int * *))))
     (lambda (how signals)
       "Add SIGNALS, a list of SIG* values, to the set of blocked signals if
-HOW is SIG_BLOCK, or unblock them if HOW is SIG_UNBLOCK."
+HOW is SIG_BLOCK, or unblock them if HOW is SIG_UNBLOCK.  Return the previous
+set of blocked signals as a list of SIG* values."
+      (define old
+        (allocate-sigset))
+
       (let-values (((result err)
-                    (proc how (sigset signals) %null-pointer)))
+                    (proc how (sigset signals) old)))
         (if (= -1 result)
             (throw 'system-error "sigprocmask" "~A"
                    (list (strerror err)) (list err))
-            result)))))
+            (sigset->list old))))))
 
 (define (block-signals signals)
   "Block SIGNALS, a list of SIG* values, in the current thread."
-- 
2.26.2

>From ec3631115f8ef070c939f392bb316ad44360a83c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <[email-masked]>
Date: Sat, 30 May 2020 19:28:35 +0200
Subject: [PATCH 2/3] system: Add 'with-blocked-signals'.

* configure.ac: Compute and substitute 'SIG_SETMASK'.
* modules/shepherd/system.scm.in (SIG_SETMASK): New variable.
(set-blocked-signals, call-with-blocked-signals): New procedures.
(with-blocked-signals): New macro.
---
 .dir-locals.el                 |  4 +++-
 configure.ac                   |  2 ++
 modules/shepherd/system.scm.in | 21 +++++++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/.dir-locals.el b/.dir-locals.el
index 8361cb6..3e64a3e 100644
--- a/.dir-locals.el
+++ b/.dir-locals.el
@@ -9,6 +9,8 @@
      (bug-reference-url-format . "http://bugs.gnu.org/%s")
      (bug-reference-bug-regexp
       . "<https?://\\(debbugs\\|bugs\\)\\.gnu\\.org/\\([0-9]+\\)>")))
- (scheme-mode     . ((indent-tabs-mode . nil)))
+ (scheme-mode
+  . ((indent-tabs-mode . nil)
+     (eval . (put 'with-blocked-signals 'scheme-indent-function 1))))
  (texinfo-mode    . ((indent-tabs-mode . nil)
                      (fill-column . 72))))
diff --git a/configure.ac b/configure.ac
index 052a826..6c2c7b3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -95,6 +95,7 @@ AC_MSG_CHECKING([<sys/signalfd.h> and <sys/signal.h> constants])
 AC_COMPUTE_INT([SFD_CLOEXEC], [SFD_CLOEXEC], [#include <sys/signalfd.h>])
 AC_COMPUTE_INT([SIG_BLOCK], [SIG_BLOCK], [#include <sys/signal.h>])
 AC_COMPUTE_INT([SIG_UNBLOCK], [SIG_UNBLOCK], [#include <sys/signal.h>])
+AC_COMPUTE_INT([SIG_SETMASK], [SIG_SETMASK], [#include <sys/signal.h>])
 AC_MSG_RESULT([done])
 
 SIZEOF_STRUCT_SIGNALFD_SIGINFO="$ac_cv_sizeof_struct_signalfd_siginfo"
@@ -104,6 +105,7 @@ AC_SUBST([SIZEOF_SIGSET_T])
 AC_SUBST([SFD_CLOEXEC])
 AC_SUBST([SIG_BLOCK])
 AC_SUBST([SIG_UNBLOCK])
+AC_SUBST([SIG_SETMASK])
 
 AC_MSG_CHECKING([whether to build crash handler])
 case "$host_os" in
diff --git a/modules/shepherd/system.scm.in b/modules/shepherd/system.scm.in
index 9c55c69..d606573 100644
--- a/modules/shepherd/system.scm.in
+++ b/modules/shepherd/system.scm.in
@@ -37,6 +37,8 @@
             consume-signalfd-siginfo
             block-signals
             unblock-signals
+            set-blocked-signals
+            with-blocked-signals
             without-automatic-finalization))
 
 ;; The <sys/reboot.h> constants.
@@ -200,6 +202,7 @@ number of the signal received."
 
 (define SIG_BLOCK @SIG_BLOCK@)
 (define SIG_UNBLOCK @SIG_UNBLOCK@)
+(define SIG_SETMASK @SIG_SETMASK@)
 
 (define sigprocmask
   (let ((proc (syscall->procedure int "pthread_sigmask" `(,int * *))))
@@ -225,6 +228,24 @@ set of blocked signals as a list of SIG* values."
   "Unblock SIGNALS, a list of SIG* values, in the current thread."
   (sigprocmask SIG_UNBLOCK signals))
 
+(define (set-blocked-signals signals)
+  "Block exactly the signals listed in SIGNALS, a list of SIG* values, in the
+current thread."
+  (sigprocmask SIG_SETMASK signals))
+
+(define (call-with-blocked-signals signals thunk)
+  (let ((previous-set #f))
+    (dynamic-wind
+      (lambda ()
+        (set! previous-set (block-signals signals)))
+      thunk
+      (lambda ()
+        (set-blocked-signals previous-set)))))
+
+(define-syntax-rule (with-blocked-signals signals exp ...)
+  "Evaluate EXP... in a context where SIGNALS are blocked."
+  (call-with-blocked-signals signals (lambda () exp ...)))
+
 
 ;;;
 ;;; Guile shenanigans.
-- 
2.26.2

>From 576ac6155dcabea47dfdf69fb9a8cc07cecf9695 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= <[email-masked]>
Date: Sat, 30 May 2020 19:32:43 +0200
Subject: [PATCH 3/3] service: 'fork+exec-command' blocks handled signals
 before forking.

This is a followup to d190773751ddeddbe0daa8e4a43f76b73c4fd7ac, which
addressed only SIGTERM instead of all of %PRECIOUS-SIGNALS.
Furthermore, setting the SIGTERM handler introduced a window in the
shepherd process during which SIGTERM instances would be lost.

* modules/shepherd/service.scm (%precious-signals): New variable.
(fork+exec-command): Remove calls to 'sigaction' for SIGTERM.  Wrap
'let' in 'with-blocked-signals'.  Restore default signal handlers in the
child before unblocking signals.
---
 modules/shepherd/service.scm | 53 ++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/modules/shepherd/service.scm b/modules/shepherd/service.scm
index f3ac32a..1bc77b1 100644
--- a/modules/shepherd/service.scm
+++ b/modules/shepherd/service.scm
@@ -870,6 +870,10 @@ false."
                   program (strerror (system-error-errno args)))
           (primitive-exit 1))))))))
 
+(define %precious-signals
+  ;; Signals that the shepherd process handles.
+  (list SIGCHLD SIGINT SIGHUP SIGTERM))
+
 (define* (fork+exec-command command
                             #:key
                             (user #f)
@@ -886,33 +890,28 @@ its PID."
     (sigaction SIGCHLD handle-SIGCHLD SA_NOCLDSTOP)
     (set! %sigchld-handler-installed? #t))
 
-  ;; When forking a process, the signal handlers are inherited, until it
-  ;; forks. If SIGTERM is received by the forked process, before it calls
-  ;; execv, the installed SIGTERM handler, stopping Shepherd will be called.
-  ;; To avoid this, save the SIGTERM handler, disable it, and restore it once,
-  ;; the process has been forked. This way, the forked process will use the
-  ;; default SIGTERM handler stopping the process.
-  (let ((term-handler (match (sigaction SIGTERM)
-                        ((proc . _)
-                         proc)))
-        (pid (and (sigaction SIGTERM SIG_DFL)
-                  (primitive-fork))))
-    (if (zero? pid)
-        (begin
-          ;; Unblock any signals that might have been blocked by the parent
-          ;; process if using 'signalfd'.
-          (unblock-signals (list SIGCHLD SIGINT SIGHUP SIGTERM))
-
-          (exec-command command
-                        #:user user
-                        #:group group
-                        #:log-file log-file
-                        #:directory directory
-                        #:file-creation-mask file-creation-mask
-                        #:environment-variables environment-variables))
-        (begin
-          ;; Restore the initial SIGTERM handler.
-          (sigaction SIGTERM term-handler)
+  ;; Child processes inherit signal handlers until they exec.  If one of
+  ;; %PRECIOUS-SIGNALS is received by the child before it execs, the installed
+  ;; handler, which stops shepherd, is called.  To avoid this, block signals
+  ;; so that the child process never executes those handlers.
+  (with-blocked-signals %precious-signals
+    (let ((pid (primitive-fork)))
+      (if (zero? pid)
+          (begin
+            ;; First restore the default handlers.
+            (for-each (cut sigaction <> SIG_DFL) %precious-signals)
+
+            ;; Unblock any signals that have been blocked by the parent
+            ;; process.
+            (unblock-signals %precious-signals)
+
+            (exec-command command
+                          #:user user
+                          #:group group
+                          #:log-file log-file
+                          #:directory directory
+                          #:file-creation-mask file-creation-mask
+                          #:environment-variables environment-variables))
           pid))))
 
 (define* (make-forkexec-constructor command
-- 
2.26.2