From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp1 ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms11 with LMTPS id GE4JGKib0l6fEwAA0tVLHw (envelope-from ) for ; Sat, 30 May 2020 17:45:12 +0000 Received: from aspmx1.migadu.com ([2001:41d0:2:4a6f::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp1 with LMTPS id uJfEE6ib0l6ffwAAbx9fmQ (envelope-from ) for ; Sat, 30 May 2020 17:45:12 +0000 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 874A29401AE for ; Sat, 30 May 2020 17:45:11 +0000 (UTC) Received: from localhost ([::1]:53550 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jf5Xg-0003Jg-R2 for larch@yhetil.org; Sat, 30 May 2020 13:45:08 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33742) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jf5Xa-0003JM-RD for guix-patches@gnu.org; Sat, 30 May 2020 13:45:02 -0400 Received: from debbugs.gnu.org ([209.51.188.43]:47595) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1jf5Xa-0000MH-HJ for guix-patches@gnu.org; Sat, 30 May 2020 13:45:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1jf5Xa-0001rE-DM for guix-patches@gnu.org; Sat, 30 May 2020 13:45:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#41507] [PATCH Shepherd 2/2] shepherd: Use 'signalfd' when possible. Resent-From: Ludovic =?UTF-8?Q?Court=C3=A8s?= Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Sat, 30 May 2020 17:45:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 41507 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: Mathieu Othacehe Cc: 41507@debbugs.gnu.org Received: via spool by 41507-submit@debbugs.gnu.org id=B41507.15908606637075 (code B ref 41507); Sat, 30 May 2020 17:45:02 +0000 Received: (at 41507) by debbugs.gnu.org; 30 May 2020 17:44:23 +0000 Received: from localhost ([127.0.0.1]:59141 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jf5Ww-0001q3-Qa for submit@debbugs.gnu.org; Sat, 30 May 2020 13:44:23 -0400 Received: from eggs.gnu.org ([209.51.188.92]:54384) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1jf5Wv-0001pp-22 for 41507@debbugs.gnu.org; Sat, 30 May 2020 13:44:21 -0400 Received: from fencepost.gnu.org ([2001:470:142:3::e]:38814) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1jf5Wp-0008S3-Pd for 41507@debbugs.gnu.org; Sat, 30 May 2020 13:44:15 -0400 Received: from [2a01:e0a:1d:7270:af76:b9b:ca24:c465] (port=46870 helo=ribbon) by fencepost.gnu.org with esmtpsa (TLS1.2:RSA_AES_256_CBC_SHA1:256) (Exim 4.82) (envelope-from ) id 1jf5Wp-0005X7-0D; Sat, 30 May 2020 13:44:15 -0400 From: Ludovic =?UTF-8?Q?Court=C3=A8s?= References: <20200524143700.6378-1-ludo@gnu.org> <20200524143700.6378-2-ludo@gnu.org> <87a71ww5zx.fsf@gnu.org> Date: Sat, 30 May 2020 19:44:12 +0200 In-Reply-To: <87a71ww5zx.fsf@gnu.org> (Mathieu Othacehe's message of "Mon, 25 May 2020 14:31:30 +0200") Message-ID: <87tuzxs4gj.fsf@gnu.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Spam-Score: -2.3 (--) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-Spam-Score: -3.3 (---) X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: "Guix-patches" X-Scanner: scn0 Authentication-Results: aspmx1.migadu.com; dkim=none; dmarc=none; spf=pass (aspmx1.migadu.com: domain of guix-patches-bounces@gnu.org designates 209.51.188.17 as permitted sender) smtp.mailfrom=guix-patches-bounces@gnu.org X-Spam-Score: -0.91 X-TUID: WlYLba8M/YOH --=-=-= Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Hello Mathieu! Mathieu Othacehe skribis: >> + (begin >> + ;; Unblock any signals that might have been blocked by the pa= rent >> + ;; 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 =E2=80=98call-with-blocked-asyncs=E2=80=99 inst= ead of =E2=80=98with-blocked-signals=E2=80=99. That would have prevented signal h= andling at the Scheme level from happening, but the unhandled signals in the child would be lost.) If it works for you, I=E2=80=99ll do some more testing, and then hopefully = we can release 0.8.1 in the coming days! Ludo=E2=80=99. --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-system-sigprocmask-returns-the-previous-set-of-block.patch >From bc74b5e33625a082ad0d44fe4409d459222aa295 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 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 --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0002-system-Add-with-blocked-signals.patch >From ec3631115f8ef070c939f392bb316ad44360a83c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 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 . ""))) - (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([ and constants]) AC_COMPUTE_INT([SFD_CLOEXEC], [SFD_CLOEXEC], [#include ]) AC_COMPUTE_INT([SIG_BLOCK], [SIG_BLOCK], [#include ]) AC_COMPUTE_INT([SIG_UNBLOCK], [SIG_UNBLOCK], [#include ]) +AC_COMPUTE_INT([SIG_SETMASK], [SIG_SETMASK], [#include ]) 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 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 --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0003-service-fork-exec-command-blocks-handled-signals-bef.patch >From 576ac6155dcabea47dfdf69fb9a8cc07cecf9695 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ludovic=20Court=C3=A8s?= 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 --=-=-=--