From debbugs-submit-bounces@debbugs.gnu.org Sun Feb 05 07:23:16 2017 Received: (at submit) by debbugs.gnu.org; 5 Feb 2017 12:23:16 +0000 Received: from localhost ([127.0.0.1]:56514 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caLqd-00072X-OL for submit@debbugs.gnu.org; Sun, 05 Feb 2017 07:23:15 -0500 Received: from eggs.gnu.org ([208.118.235.92]:33630) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caLqb-00072K-Fn for submit@debbugs.gnu.org; Sun, 05 Feb 2017 07:23:13 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1caLqV-0002PR-96 for submit@debbugs.gnu.org; Sun, 05 Feb 2017 07:23:08 -0500 X-Spam-Checker-Version: SpamAssassin 3.3.2 (2011-06-06) on eggs.gnu.org X-Spam-Level: X-Spam-Status: No, score=0.8 required=5.0 tests=BAYES_50 autolearn=disabled version=3.3.2 Received: from lists.gnu.org ([2001:4830:134:3::11]:52414) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1caLqV-0002PL-5f for submit@debbugs.gnu.org; Sun, 05 Feb 2017 07:23:07 -0500 Received: from eggs.gnu.org ([2001:4830:134:3::10]:52832) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1caLqT-0008HI-Dd for bug-coreutils@gnu.org; Sun, 05 Feb 2017 07:23:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1caLqQ-0002Nd-8C for bug-coreutils@gnu.org; Sun, 05 Feb 2017 07:23:05 -0500 Received: from mout.kundenserver.de ([217.72.192.75]:52507) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1caLqP-0002MS-TT for bug-coreutils@gnu.org; Sun, 05 Feb 2017 07:23:02 -0500 Received: from localhost ([79.234.34.66]) by mrelayeu.kundenserver.de (mreue104 [212.227.15.145]) with ESMTPSA (Nemesis) id 0Mf1KL-1cpW9b2wlV-00OYv5 for ; Sun, 05 Feb 2017 13:22:58 +0100 Date: Sun, 5 Feb 2017 13:23:22 +0100 From: Tobias Stoeckmann To: bug-coreutils@gnu.org Subject: [PATCH] timeout: Fix signal race in SIGALRM handling Message-ID: <20170205122322.GA24928@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Provags-ID: V03:K0:cPFRAYSo6CC38CsTsr+htGLaZQitgwzdWtf9WwRbveS96I8DXSB W/NZiCVePmr9+JxHYNttye/4fkhmBO1vh9hMWnc0VYdpjfbexQgcFn02BEZYPNBGYpcX3/O 9sWCeeLnq42j6GeARknsqw6tEyh8jo03Eo0j/vIdOzdKcYvILC0u3uo4rfJ9jXK4/Pk0G8L smz2gYmrggEMST7YQ0KwQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:eYZjQmUYI0E=:kYKlMNzEGeK5Bd1vNA/xDi R37+Pif59zLu+trlzOCY3g1rzzAX/yAP2TWRzpWtlBik1P2OQs7JL5moNrE6RfFKT5uhiW2qr RZznTjaYQSQWYsBRRq3uUeO+TEqD+1bDTKIRf8rVjJJg28Q23WZiC/Q73GsYf4+vVwwMBpHHW 3tXXu0BaJmSSy+TTEsCy+RsfyhClcMUKT36N22tNnBUOAu2oaNmzqNVUx/Dmc8FYQH0dvymuP p4Tgk7dB0kQXlQfSAiHgw8NiIqe5Sb/yCzEKHI6n9R1AmWimeVYfNOAHM8GrFUAHmxz0eLxAd QzO+E9cEjC1XNr3/YM8du3CEsPKxd1dANLHVZ0oMp+4TiAaRQRCtfl+uQeNmcvAK12hDQk7pe QCy86HHW/PyCMqrBVloYzn8/aCd2qINpKumrsxCqLA+SGZfyjxJJYRqUfbORa1QeUFZ74D1UV H0N4Dtr5LOPvSdCHotxRKzw2JiAD4I0pULiswQQW1OmszgLGmPK8Ywnvkjuk5TZK+GQ94XWMS uUKcNbarxz0VPQgj/tvjCJfrder+xUV3e8mFYez3y4+tC/26nEmyhnPcREnjqx1R6zop21lgC 2YmbdEksWWOSk2AKzcaOHVPVhitWf5+DR7LfQ5cGkdlL0bBEZOpqHwbPLrXIjHgfTANeQaMZB 2er3LbPAj6IMdvqmnNu+I1R/GnAAd4wreqoEXCh5GXVrHN8+ufdOrUzLHwCKymffVqSA= X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.6.x X-Received-From: 2001:4830:134:3::11 X-Spam-Score: -5.0 (-----) X-Debbugs-Envelope-To: submit X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -5.0 (-----) It is possible to trigger a signal race in timeout if the alarm is triggered AFTER the child process already finished and was waited for by the parent. If a child terminates, it becomes a zombie process until the parent called waitpid() (or an equivalent system call) on it. This means that the process id cannot ge given to a new process. But if waitpid() has been called, the process id stored in the variable monitored_pid could already be assigned to another one. If the alarm -- due to timeout -- is triggered afterwards, the tool can send a signal to that new process instead. To fix this, I have introduced a critical section around waitpid(), which blocks SIGALRM and resets monitored_pid to 0 if waitpid() returned anything except 0 (be it a success or a failure). This way, a later alarm won't send any signal. While at it, I changed int to pid_t, because that's the actual data type for process ids. Granted, normally it can be assumed that pid_t is an int. --- src/timeout.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/src/timeout.c b/src/timeout.c index c38e3657a..633ecef3d 100644 --- a/src/timeout.c +++ b/src/timeout.c @@ -79,7 +79,7 @@ static int timed_out; static int term_signal = SIGTERM; /* same default as kill command. */ -static int monitored_pid; +static pid_t monitored_pid; static double kill_after; static bool foreground; /* whether to use another program group. */ static bool preserve_status; /* whether to use a timeout status or not. */ @@ -103,6 +103,16 @@ static struct option const long_options[] = }; static void +block_signal (int sig, sigset_t *old_set) +{ + sigset_t block_set; + sigemptyset (&block_set); + sigaddset (&block_set, sig); + if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0) + error (0, errno, _("warning: sigprocmask")); +} + +static void unblock_signal (int sig) { sigset_t unblock_set; @@ -121,10 +131,6 @@ static void settimeout (double duration, bool warn) { - /* We configure timers below so that SIGALRM is sent on expiry. - Therefore ensure we don't inherit a mask blocking SIGALRM. */ - unblock_signal (SIGALRM); - /* timer_settime() provides potentially nanosecond resolution. setitimer() is more portable (to Darwin for example), but only provides microsecond resolution and thus is @@ -165,7 +171,7 @@ settimeout (double duration, bool warn) /* send SIG avoiding the current process. */ static int -send_sig (int where, int sig) +send_sig (pid_t where, int sig) { /* If sending to the group, then ignore the signal, so we don't go into a signal loop. Note that this will ignore any of the @@ -179,6 +185,13 @@ send_sig (int where, int sig) return kill (where, sig); } +/* Signal handler which is required for sigsuspend() to be interrupted + whenever SIGCHLD is received. */ +static void +chld (int sig) +{ +} + static void cleanup (int sig) { @@ -436,7 +449,7 @@ main (int argc, char **argv) install_signal_handlers (term_signal); signal (SIGTTIN, SIG_IGN); /* Don't stop if background child needs tty. */ signal (SIGTTOU, SIG_IGN); /* Don't stop if background child needs tty. */ - signal (SIGCHLD, SIG_DFL); /* Don't inherit CHLD handling from parent. */ + signal (SIGCHLD, chld); /* Use a signal handler for SIGCHLD. */ monitored_pid = fork (); if (monitored_pid == -1) @@ -460,13 +473,16 @@ main (int argc, char **argv) else { pid_t wait_result; + sigset_t old_set; int status; settimeout (timeout, true); - while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0 - && errno == EINTR) - continue; + block_signal (SIGALRM, &old_set); + while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0) + sigsuspend (&old_set); + monitored_pid = 0; + unblock_signal (SIGALRM); if (wait_result < 0) { -- 2.11.1 From debbugs-submit-bounces@debbugs.gnu.org Sun Feb 05 12:57:02 2017 Received: (at 25624) by debbugs.gnu.org; 5 Feb 2017 17:57:02 +0000 Received: from localhost ([127.0.0.1]:56842 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caR3d-0007l7-OF for submit@debbugs.gnu.org; Sun, 05 Feb 2017 12:57:01 -0500 Received: from midir.magicbluesmoke.com ([82.195.144.46]:57016 helo=mail.magicbluesmoke.com) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caR3b-0007kr-8p for 25624@debbugs.gnu.org; Sun, 05 Feb 2017 12:57:00 -0500 Received: from [10.0.0.126] (c-73-170-254-78.hsd1.ca.comcast.net [73.170.254.78]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.magicbluesmoke.com (Postfix) with ESMTPSA id 45CA294AF; Sun, 5 Feb 2017 17:56:57 +0000 (GMT) Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling To: Tobias Stoeckmann , 25624@debbugs.gnu.org References: <20170205122322.GA24928@localhost> From: =?UTF-8?Q?P=c3=a1draig_Brady?= Message-ID: Date: Sun, 5 Feb 2017 09:56:54 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20170205122322.GA24928@localhost> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 25624 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 0.0 (/) On 05/02/17 04:23, Tobias Stoeckmann wrote: > It is possible to trigger a signal race in timeout if the alarm is > triggered AFTER the child process already finished and was waited for > by the parent. > If a child terminates, it becomes a zombie process until the parent > called waitpid() (or an equivalent system call) on it. This means that > the process id cannot ge given to a new process. But if waitpid() has > been called, the process id stored in the variable monitored_pid could > already be assigned to another one. If the alarm -- due to timeout -- > is triggered afterwards, the tool can send a signal to that new process > instead. > > To fix this, I have introduced a critical section around waitpid(), > which blocks SIGALRM and resets monitored_pid to 0 if waitpid() returned > anything except 0 (be it a success or a failure). This way, a later > alarm won't send any signal. > diff --git a/src/timeout.c b/src/timeout.c > settimeout (double duration, bool warn) > { > > - /* We configure timers below so that SIGALRM is sent on expiry. > - Therefore ensure we don't inherit a mask blocking SIGALRM. */ > - unblock_signal (SIGALRM); Removing the call above causes this test to fail: tests/misc/timeout-blocked.pl http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=blob;f=tests/misc/timeout-blocked.pl;hb=HEAD > - while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0 > - && errno == EINTR) > - continue; > + block_signal (SIGALRM, &old_set); > + while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0) > + sigsuspend (&old_set); > + monitored_pid = 0; > + unblock_signal (SIGALRM); I think we'd need some conditional code to allow sigsuspend() to compile on mingw, MSVC 9. In general this is a largely theoretical race right? I.E. pids would need to be recycled between the waitpid() and exit()? Just trying to get an idea of the practicality of the issue. thanks! Pádraig From debbugs-submit-bounces@debbugs.gnu.org Sun Feb 05 13:26:46 2017 Received: (at 25624) by debbugs.gnu.org; 5 Feb 2017 18:26:46 +0000 Received: from localhost ([127.0.0.1]:56850 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caRWP-0008Rp-DP for submit@debbugs.gnu.org; Sun, 05 Feb 2017 13:26:46 -0500 Received: from zimbra.cs.ucla.edu ([131.179.128.68]:53306) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caRWN-0008Rb-4f for 25624@debbugs.gnu.org; Sun, 05 Feb 2017 13:26:43 -0500 Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 2477516006A; Sun, 5 Feb 2017 10:26:37 -0800 (PST) Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id 6g95GaHJRSes; Sun, 5 Feb 2017 10:26:36 -0800 (PST) Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 1B09716007A; Sun, 5 Feb 2017 10:26:36 -0800 (PST) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id GUTXyEH5YF9l; Sun, 5 Feb 2017 10:26:36 -0800 (PST) Received: from [192.168.1.9] (unknown [47.153.188.248]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id F007E16006A; Sun, 5 Feb 2017 10:26:35 -0800 (PST) Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling To: =?UTF-8?Q?P=c3=a1draig_Brady?= , Tobias Stoeckmann , 25624@debbugs.gnu.org References: <20170205122322.GA24928@localhost> From: Paul Eggert Organization: UCLA Computer Science Department Message-ID: <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> Date: Sun, 5 Feb 2017 10:26:35 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable X-Spam-Score: -0.0 (/) X-Debbugs-Envelope-To: 25624 X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -0.0 (/) P=C3=A1draig Brady wrote: > In general this is a largely theoretical race right? > I.E. pids would need to be recycled between the waitpid() and exit()? Not that theoretical, in the common case of systems with wraparaound PIDs= and a=20 small PID space. All you need is a long-running child on a busy system. From debbugs-submit-bounces@debbugs.gnu.org Sun Feb 05 13:50:09 2017 Received: (at 25624) by debbugs.gnu.org; 5 Feb 2017 18:50:09 +0000 Received: from localhost ([127.0.0.1]:56885 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caRt2-0000aB-VZ for submit@debbugs.gnu.org; Sun, 05 Feb 2017 13:50:09 -0500 Received: from mout.kundenserver.de ([212.227.126.134]:49384) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caRt1-0000Zn-SQ for 25624@debbugs.gnu.org; Sun, 05 Feb 2017 13:50:08 -0500 Received: from localhost ([79.234.34.66]) by mrelayeu.kundenserver.de (mreue005 [212.227.15.129]) with ESMTPSA (Nemesis) id 0MJodE-1cZLx421TA-0017a9; Sun, 05 Feb 2017 19:49:50 +0100 Date: Sun, 5 Feb 2017 19:50:14 +0100 From: Tobias Stoeckmann To: Paul Eggert Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling Message-ID: <20170205185013.GA6797@localhost> References: <20170205122322.GA24928@localhost> <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> X-Provags-ID: V03:K0:BSr6t7iMqlLgiYQ0agBMqxQERAngF5v9HXHnkeozR5kWpDK6IyJ E9DiLGEvxfm9b8lMHUYqChqgqNW8u71wEpQy5OR7l+re9BfblPAL2JhUolaOAviqpu947jQ +aSVNoRSPUI7+G8yu2SAelejkZL4KkemfIFBvRfSa5iGZS5TegybR1qh9PlWyEIAVa8Jh60 npSoy4aPULIEvo3iwSHfg== X-UI-Out-Filterresults: notjunk:1;V01:K0:g1DW2RixE8A=:2ZRLCMfk0clvotFjDwjtyo zx4ReXIZJpr3NBhLPJL9sC4CBxjnX0SJ5gRt7oVG4x//5GTUchdEChu+SoAQqtyul038dfCjX BR9GU1/u2qPJVivBPSPtyZHLM0aLAIpg8rYX6KRWsH5h+WqJdpr5Vl9OapTwUthHrofYaoM8i xtoFfWjC05W0t1iwU1yh4A3twbt20hIg3QbECT1aDFx5CBbONS6QMlPBq3tlrxgkhXkl7QZRx 9jUCwmI9cWra6xA/AJOIe5G01s2f2a+hNUDujovB/E2R4oMGInp3zYVSFqgDP9XaTZvQC7AX3 7G39+CEdXTfwLzScrEiV5yqZAA7T6S8eOJH2jCKdT7f3NBrhbhjMLJKlJJI3Sp3aOPtctmb4K +mYOTgDm+LdH8jy7FZfXRyIxaWat8jYWZ6Knf5AefrvkwThTDdd6ybQ3W2pA5SaDpCPDBobev dTFX5FSJPTO6eNbTH9bmnFUvg2nNTb1Wz5Q7ex2LhuLetlsG3p+bpTOK9WgBhTo9cfzq6K86C gH8+p+ePhNQP66MPBtszIKoqdL263cVkUSPOOOz5Mk13Yc0t4S+RmbUr+d9A6a0YDaQrmjup1 WdlU9lMmrl3UusxpFk9YKwENw2s78jqORDBs8RlDLL68JF3LD4gexcafHhAmA5x6cnoYX1d/M V8kKe7D/yas0dPcmwJXxB1pvOogx5vnDz8x33lGo7Dchp7WGr0rRa76FDG/pDPNSpAT0= X-Spam-Score: -1.9 (-) X-Debbugs-Envelope-To: 25624 Cc: =?iso-8859-1?Q?P=E1draig?= Brady , 25624@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.9 (-) On Sun, Feb 05, 2017 at 10:26:35AM -0800, Paul Eggert wrote: > Pádraig Brady wrote: > > In general this is a largely theoretical race right? > > I.E. pids would need to be recycled between the waitpid() and exit()? > > Not that theoretical, in the common case of systems with wraparaound PIDs > and a small PID space. All you need is a long-running child on a busy > system. Yes, normally it is small enough to overflow in less than a minute if an attacker runs fork() kill() in a loop. I have updated the patch so it passes the test. As I don't have enough experience in portable #ifdef's for all supported systems, I hope you can adjust the patch as needed. Tobias From debbugs-submit-bounces@debbugs.gnu.org Sun Feb 05 13:50:30 2017 Received: (at 25624) by debbugs.gnu.org; 5 Feb 2017 18:50:30 +0000 Received: from localhost ([127.0.0.1]:56888 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caRtO-0000aj-5r for submit@debbugs.gnu.org; Sun, 05 Feb 2017 13:50:30 -0500 Received: from mout.kundenserver.de ([212.227.126.130]:55702) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caRtM-0000aW-OW for 25624@debbugs.gnu.org; Sun, 05 Feb 2017 13:50:29 -0500 Received: from localhost ([79.234.34.66]) by mrelayeu.kundenserver.de (mreue005 [212.227.15.129]) with ESMTPSA (Nemesis) id 0LrXNx-1cSWHB3JBF-013ROq; Sun, 05 Feb 2017 19:50:12 +0100 Date: Sun, 5 Feb 2017 19:50:37 +0100 From: Tobias Stoeckmann To: Paul Eggert Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling Message-ID: <20170205185037.GB6797@localhost> References: <20170205122322.GA24928@localhost> <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> X-Provags-ID: V03:K0:bAqoHOCjEALq1n11srJTR/wzmdpHq2a5AyjWGRhTag/O7S4+VGO jJ67xR+NSX440JOSCdR9kfEqDPDVhOpOVCds3YgInKLhgezPnmvg9qfv0rXO2XyU7Cwq4lI rPXVhKD0oJp54QLf4bLRrfBoKi1fd8XnaGngFjK3KkEOSVj4RG+CpgL16i8TAvE3sumR89w YElF0lwbP91cVxYXxALXA== X-UI-Out-Filterresults: notjunk:1;V01:K0:RFUOhbNC3GI=:dVfP/yQbB2rsOBQRn5giVJ Zqjfu6+AtdH12RqW4yKKD4b258UtoyJ88w4Eg1JbYm+8Lee+DlNpz8WUOJXg3n2WGrltMDUKG KQO4TEhWmmF9VPja6oFI38iKshCeLinLIyDzmwLh+wMPLTh/2QbQSCc78mTapQoXSyykXVr3R 6aBeQ5A2z8jCp6FQ7yZQoTacIrOBxYtmB2EVdVQvqNkyeNYJq6A+IX8KHqlyKGGBDMF3BJGdI dy+jQZltBgtsWou3lluwfM7HaZU/v+tIXF2JnWq4wXfyY+4/UwSsXhsritlTTYl8PsqZHmTe4 +ESfEmpgpeZb50xUF2YdgLo/HhQfRV5O4YMZJzGT0Y7mi1ic/AGW32zy60pJLsCuMVK8I4t3X xaxaqifL5XhWcbioOSPpxtGzvpxvzV7Gv3aSxepVPJtkd8ma83WbmPLKp5uEzMAThia6k4k1U uWw0C1XCLZjZkS4G3PPICIhGasnzfvr1xX6kQGZplMEuCbulpcsLahBW/2I6GRyi5j7wttZz1 sQEpyiJRs6peOfgZcQncrf84N4LTQhcooKcJYL7mHPZmFt8zprlpFOjKfoiDVe42PnNONaVfX tY7eGAzIfTqA7JmmpobYthEGQyII+drL0XGMOs6wyDB6/8vZZD6qzgHEo2GfUzmSAJ8rjwYzA vC+RgHUBi+7Aoz4H8sGxBXVFsqyR6K8xJ2tTbAQx8YkdmAiMET0DNkebIVs3Zsfchwn4= X-Spam-Score: -1.4 (-) X-Debbugs-Envelope-To: 25624 Cc: =?iso-8859-1?Q?P=E1draig?= Brady , 25624@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.4 (-) It is possible to trigger a signal race in timeout if the alarm is triggered AFTER the child process already finished and was waited for by the parent. If a child terminates, it becomes a zombie process until the parent called waitpid() (or an equivalent system call) on it. This means that the process id cannot ge given to a new process. But if waitpid() has been called, the process id stored in the variable monitored_pid could already be assigned to another one. If the alarm -- due to timeout -- is triggered afterwards, the tool can send a signal to that new process instead. To fix this, I have introduced a critical section around waitpid(), which blocks SIGALRM and resets monitored_pid to 0 if waitpid() returned anything except 0 (be it a success or a failure). This way, a later alarm won't send any signal. While at it, I changed int to pid_t, because that's the actual data type for process ids. Granted, normally it can be assumed that pid_t is an int. --- src/timeout.c | 39 +++++++++++++++++++++++++++++---------- 1 file changed, 29 insertions(+), 10 deletions(-) diff --git a/src/timeout.c b/src/timeout.c index c38e3657a..99819840a 100644 --- a/src/timeout.c +++ b/src/timeout.c @@ -79,7 +79,7 @@ static int timed_out; static int term_signal = SIGTERM; /* same default as kill command. */ -static int monitored_pid; +static pid_t monitored_pid; static double kill_after; static bool foreground; /* whether to use another program group. */ static bool preserve_status; /* whether to use a timeout status or not. */ @@ -103,6 +103,16 @@ static struct option const long_options[] = }; static void +block_signal (int sig, sigset_t *old_set) +{ + sigset_t block_set; + sigemptyset (&block_set); + sigaddset (&block_set, sig); + if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0) + error (0, errno, _("warning: sigprocmask")); +} + +static void unblock_signal (int sig) { sigset_t unblock_set; @@ -121,10 +131,6 @@ static void settimeout (double duration, bool warn) { - /* We configure timers below so that SIGALRM is sent on expiry. - Therefore ensure we don't inherit a mask blocking SIGALRM. */ - unblock_signal (SIGALRM); - /* timer_settime() provides potentially nanosecond resolution. setitimer() is more portable (to Darwin for example), but only provides microsecond resolution and thus is @@ -165,7 +171,7 @@ settimeout (double duration, bool warn) /* send SIG avoiding the current process. */ static int -send_sig (int where, int sig) +send_sig (pid_t where, int sig) { /* If sending to the group, then ignore the signal, so we don't go into a signal loop. Note that this will ignore any of the @@ -179,6 +185,13 @@ send_sig (int where, int sig) return kill (where, sig); } +/* Signal handler which is required for sigsuspend() to be interrupted + whenever SIGCHLD is received. */ +static void +chld (int sig) +{ +} + static void cleanup (int sig) { @@ -436,7 +449,7 @@ main (int argc, char **argv) install_signal_handlers (term_signal); signal (SIGTTIN, SIG_IGN); /* Don't stop if background child needs tty. */ signal (SIGTTOU, SIG_IGN); /* Don't stop if background child needs tty. */ - signal (SIGCHLD, SIG_DFL); /* Don't inherit CHLD handling from parent. */ + signal (SIGCHLD, chld); /* Use a signal handler for SIGCHLD. */ monitored_pid = fork (); if (monitored_pid == -1) @@ -460,13 +473,19 @@ main (int argc, char **argv) else { pid_t wait_result; + sigset_t old_set; int status; + /* We configure timers below so that SIGALRM is sent on expiry. + Therefore ensure we don't inherit a mask blocking SIGALRM. */ + unblock_signal (SIGALRM); settimeout (timeout, true); - while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0 - && errno == EINTR) - continue; + block_signal (SIGALRM, &old_set); + while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0) + sigsuspend (&old_set); + monitored_pid = 0; + unblock_signal (SIGALRM); if (wait_result < 0) { -- 2.11.1 From debbugs-submit-bounces@debbugs.gnu.org Sun Feb 05 21:13:56 2017 Received: (at 25624) by debbugs.gnu.org; 6 Feb 2017 02:13:56 +0000 Received: from localhost ([127.0.0.1]:57044 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caYoV-0003qc-P8 for submit@debbugs.gnu.org; Sun, 05 Feb 2017 21:13:55 -0500 Received: from midir.magicbluesmoke.com ([82.195.144.46]:58120 helo=mail.magicbluesmoke.com) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1caYoT-0003qT-W8 for 25624@debbugs.gnu.org; Sun, 05 Feb 2017 21:13:54 -0500 Received: from [10.0.0.126] (c-73-170-254-78.hsd1.ca.comcast.net [73.170.254.78]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.magicbluesmoke.com (Postfix) with ESMTPSA id 30087D8; Mon, 6 Feb 2017 02:13:50 +0000 (GMT) Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling To: Tobias Stoeckmann References: <20170205122322.GA24928@localhost> <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> <20170205185037.GB6797@localhost> From: =?UTF-8?Q?P=c3=a1draig_Brady?= Message-ID: <139f2d2b-5bf8-a72b-ef25-9f45222b3ed7@draigBrady.com> Date: Sun, 5 Feb 2017 18:13:47 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20170205185037.GB6797@localhost> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 25624 Cc: 25624@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 0.0 (/) On 05/02/17 10:50, Tobias Stoeckmann wrote: > { > pid_t wait_result; > + sigset_t old_set; > int status; > > + /* We configure timers below so that SIGALRM is sent on expiry. > + Therefore ensure we don't inherit a mask blocking SIGALRM. */ > + unblock_signal (SIGALRM); cool > settimeout (timeout, true); > > - while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0 > - && errno == EINTR) > - continue; > + block_signal (SIGALRM, &old_set); SIGALRM isn't the only signal causing this race? If a SIGTERM etc. is sent to the timeout process during the race, cleanup() will be called with the same consequences. Don't we need to block all signals for the cleanup() handler? > + while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0) > + sigsuspend (&old_set); > + monitored_pid = 0; > + unblock_signal (SIGALRM); There doesn't seem to be much point in re-enabling SIGALRM (or other signals) here, as if it does fire it means we'll return 143 rather than the correct 124. Since the child is reaped, we can just proceed and process the exit status of the child and exit() I think. cheers, Pádraig. From debbugs-submit-bounces@debbugs.gnu.org Thu Feb 09 15:09:46 2017 Received: (at 25624) by debbugs.gnu.org; 9 Feb 2017 20:09:46 +0000 Received: from localhost ([127.0.0.1]:33456 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cbv2I-00041q-EQ for submit@debbugs.gnu.org; Thu, 09 Feb 2017 15:09:46 -0500 Received: from mout.kundenserver.de ([212.227.126.135]:52905) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cbv2G-00041e-UX for 25624@debbugs.gnu.org; Thu, 09 Feb 2017 15:09:45 -0500 Received: from localhost ([91.7.166.128]) by mrelayeu.kundenserver.de (mreue004 [212.227.15.129]) with ESMTPSA (Nemesis) id 0LehOU-1c4H3X217O-00qSUh; Thu, 09 Feb 2017 21:09:37 +0100 Date: Thu, 9 Feb 2017 21:09:42 +0100 From: Tobias Stoeckmann To: =?iso-8859-1?Q?P=E1draig?= Brady Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling Message-ID: <20170209200942.GA20270@localhost> References: <20170205122322.GA24928@localhost> <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> <20170205185037.GB6797@localhost> <139f2d2b-5bf8-a72b-ef25-9f45222b3ed7@draigBrady.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <139f2d2b-5bf8-a72b-ef25-9f45222b3ed7@draigBrady.com> X-Provags-ID: V03:K0:0TUHSP0lXymed8IZc27GyIPGf96rUqEOifcW1ZkduG/4qtgjobz xbTYbL7KMiuMHFNTk8UNfnK5eBxP86ESXWDK00hvIg0EksWLYYnNKOxBfNweS7QOFAdAR0w 6mSoQ8mqVweRFKWj6MWfOif4zoji7lvsEIbC/x6h4nCaZzIaP5wlI1sWWn3fMXa+z6mXirC p/fg9pNmTpMrSsYsp+Mpw== X-UI-Out-Filterresults: notjunk:1;V01:K0:ZpMex1ab0Vo=:CSCtLwuOitLxPEiyjasMn2 m0k18L76Va3HI+fYe4W1JJbqKXyKEyh6TNnef/JIDP92Wx3ObqJEaLl3Vw4j6fW59yzzT6Kx3 X6+eyM8eVyV0KYLjNZsvgSdfDAJKB6XMOZJBZzbn80zdclTs/EgwirXiEX/ktl9u8i9bZyk8x hd6nZQuHyBQeXDuwKS8ARoOwMNC1YfBu2IVhwadPiXHwU3DLF+LI/mdsVh15B0b8YhHNvra+k wEc9zQjYCky5wBYnd0Ro49qteNynWREGCNcp/pBR00nZzphdVtCSZjHBSqxvqkN7qivQaSQwR 01zgOt4jQJoFufBeC/6z0PaNRAZJX+h2KxKAxcAhANDyhV/Ey6/JTN/RNh4L3bzWACoupfxr/ ORwkFdj7tyQQSeLgD3FXvwytCvBcCrcuVwbV7a9klpgaeN6Z1JpgrLbadB3uFm1VqbBCW0lsc hvsJgJrrOdkoOKxFRPVJ5ovX4dvl1VchPI5h5LK/fM7Cf80C1KIdet9FAP9y7DuZy27I9cKba lMtONMsOqOY+vIfEnOJccW9mid3H/kA5DEKunST6YPnGwGNpPCx0hzUSFjzxWyb01elUfYsuO wFWZ6vI0ugcKPGwHGKeI7iPkh6OH1RIZCOJXK77wJgyBMHM251UlY0aZY57Gu8+s3yEiYPLRs YACzXMqHa1O7I6XNYXFpYLD3gg+D37jFQ4gRuIGLzm+ZLYaVxKW8RoGoFbrvhmdVm4No= X-Spam-Score: -1.4 (-) X-Debbugs-Envelope-To: 25624 Cc: 25624@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: -1.4 (-) It is possible to trigger a signal race in timeout if the alarm is triggered AFTER the child process already finished and was waited for by the parent. If a child terminates, it becomes a zombie process until the parent called waitpid() (or an equivalent system call) on it. This means that the process id cannot ge given to a new process. But if waitpid() has been called, the process id stored in the variable monitored_pid could already be assigned to another one. If the alarm -- due to timeout -- is triggered afterwards, the tool can send a signal to that new process instead. To fix this, I have introduced a critical section around waitpid(), which blocks SIGALRM and resets monitored_pid to 0 if waitpid() returned anything except 0 (be it a success or a failure). This way, a later alarm won't send any signal. While at it, I changed int to pid_t, because that's the actual data type for process ids. Granted, normally it can be assumed that pid_t is an int. --- Thanks for your great review and feedback. :) --- src/timeout.c | 43 +++++++++++++++++++++++++++++++++---------- 1 file changed, 33 insertions(+), 10 deletions(-) diff --git a/src/timeout.c b/src/timeout.c index c38e3657a..b9f8ef5a2 100644 --- a/src/timeout.c +++ b/src/timeout.c @@ -79,7 +79,7 @@ static int timed_out; static int term_signal = SIGTERM; /* same default as kill command. */ -static int monitored_pid; +static pid_t monitored_pid; static double kill_after; static bool foreground; /* whether to use another program group. */ static bool preserve_status; /* whether to use a timeout status or not. */ @@ -102,6 +102,22 @@ static struct option const long_options[] = {NULL, 0, NULL, 0} }; +/* Blocks all signals which were registered with cleanup + as signal handler. */ +static void +block_cleanup (sigset_t *old_set) +{ + sigset_t block_set; + sigemptyset (&block_set); + sigaddset (&block_set, SIGALRM); + sigaddset (&block_set, SIGINT); + sigaddset (&block_set, SIGQUIT); + sigaddset (&block_set, SIGHUP); + sigaddset (&block_set, SIGTERM); + if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0) + error (0, errno, _("warning: sigprocmask")); +} + static void unblock_signal (int sig) { @@ -121,10 +137,6 @@ static void settimeout (double duration, bool warn) { - /* We configure timers below so that SIGALRM is sent on expiry. - Therefore ensure we don't inherit a mask blocking SIGALRM. */ - unblock_signal (SIGALRM); - /* timer_settime() provides potentially nanosecond resolution. setitimer() is more portable (to Darwin for example), but only provides microsecond resolution and thus is @@ -165,7 +177,7 @@ settimeout (double duration, bool warn) /* send SIG avoiding the current process. */ static int -send_sig (int where, int sig) +send_sig (pid_t where, int sig) { /* If sending to the group, then ignore the signal, so we don't go into a signal loop. Note that this will ignore any of the @@ -179,6 +191,13 @@ send_sig (int where, int sig) return kill (where, sig); } +/* Signal handler which is required for sigsuspend() to be interrupted + whenever SIGCHLD is received. */ +static void +chld (int sig) +{ +} + static void cleanup (int sig) { @@ -436,7 +455,7 @@ main (int argc, char **argv) install_signal_handlers (term_signal); signal (SIGTTIN, SIG_IGN); /* Don't stop if background child needs tty. */ signal (SIGTTOU, SIG_IGN); /* Don't stop if background child needs tty. */ - signal (SIGCHLD, SIG_DFL); /* Don't inherit CHLD handling from parent. */ + signal (SIGCHLD, chld); /* Use a signal handler for SIGCHLD. */ monitored_pid = fork (); if (monitored_pid == -1) @@ -460,13 +479,17 @@ main (int argc, char **argv) else { pid_t wait_result; + sigset_t old_set; int status; + /* We configure timers below so that SIGALRM is sent on expiry. + Therefore ensure we don't inherit a mask blocking SIGALRM. */ + unblock_signal (SIGALRM); settimeout (timeout, true); - while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0 - && errno == EINTR) - continue; + block_cleanup (&old_set); + while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0) + sigsuspend (&old_set); if (wait_result < 0) { -- 2.11.1 From debbugs-submit-bounces@debbugs.gnu.org Fri Feb 10 00:39:43 2017 Received: (at 25624-done) by debbugs.gnu.org; 10 Feb 2017 05:39:43 +0000 Received: from localhost ([127.0.0.1]:33681 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cc3vn-0005by-MD for submit@debbugs.gnu.org; Fri, 10 Feb 2017 00:39:43 -0500 Received: from midir.magicbluesmoke.com ([82.195.144.46]:57684 helo=mail.magicbluesmoke.com) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cc3vh-0005bl-P1 for 25624-done@debbugs.gnu.org; Fri, 10 Feb 2017 00:39:37 -0500 Received: from [10.0.0.126] (c-73-170-254-78.hsd1.ca.comcast.net [73.170.254.78]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.magicbluesmoke.com (Postfix) with ESMTPSA id BEE934A68; Fri, 10 Feb 2017 05:39:31 +0000 (GMT) Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling To: Tobias Stoeckmann References: <20170205122322.GA24928@localhost> <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> <20170205185037.GB6797@localhost> <139f2d2b-5bf8-a72b-ef25-9f45222b3ed7@draigBrady.com> <20170209200942.GA20270@localhost> From: =?UTF-8?Q?P=c3=a1draig_Brady?= Message-ID: <8f8221eb-8a10-2861-44b2-ce3c002c41c7@draigBrady.com> Date: Thu, 9 Feb 2017 21:39:29 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20170209200942.GA20270@localhost> Content-Type: multipart/mixed; boundary="------------22DE78771BC5CC669184DEE5" X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 25624-done Cc: 25624-done@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 0.0 (/) This is a multi-part message in MIME format. --------------22DE78771BC5CC669184DEE5 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit That looks good. I've changed the naming a bit, ensured we also block the term_signal, made the build of timeout optional on sigsuspend availability, adjusted the commit message, and added a NEWS entry. Marking this as done now. I'll push in your name later. thanks! Pádraig --------------22DE78771BC5CC669184DEE5 Content-Type: text/x-patch; name="timeout-race.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="timeout-race.patch" >From 85c006bed19e4c3e0ec0f1be0f9863bf940ab630 Mon Sep 17 00:00:00 2001 From: Tobias Stoeckmann Date: Sun, 5 Feb 2017 13:23:22 +0100 Subject: [PATCH] timeout: fix race possibly terminating wrong process The race is unlikely, as timeout(1) needs to receive a signal in the few operations between waitpid() returning and exit(). Also the system needs to have reallocated the just released pid in this time window. Previously we never disables the signal handler that sent the termination signal to the "child" pid. However once waitpid() has reaped the child, the system is free to allocate that pid, so we must ensure we don't process any further signals. * build-aux/gen-lists-of-programs.sh: Build timeout(1) optionally... * configure.ac: ...predicated on sigsuspend() being available. * src/timeout.c (block_cleanup): A new function to ensure the cleanup() handler is disabled after waitpid has returned. (main): Use sigsuspend() to wait with cleanup() enabled but disabled once it returns, and thus disabled for the waitpid() call. (monitored_pid): Change to the more accurate pid_t. * NEWS: Mention the fix. Fixes http://bugs.gnu.org/25624 --- NEWS | 6 +++ build-aux/gen-lists-of-programs.sh | 2 +- configure.ac | 2 + src/timeout.c | 75 ++++++++++++++++++++++++++------------ 4 files changed, 61 insertions(+), 24 deletions(-) diff --git a/NEWS b/NEWS index 9528f89..deaab7b 100644 --- a/NEWS +++ b/NEWS @@ -16,6 +16,12 @@ GNU coreutils NEWS -*- outline -*- which could be triggered especially when tail was suspended and resumed. [bug introduced with inotify support added in coreutils-7.5] + timeout no longer has a race that may terminate the wrong process. + The race is unlikely, as timeout(1) needs to receive a signal right + after the command being monitored finishes. Also the system needs + to have reallocated that command's pid in that short time window. + [bug introduced when timeout was added in coreutils-7.0] + wc --bytes --files0-from now correctly reports byte counts. Previously it may have returned values that were too large, depending on the size of the first file processed. diff --git a/build-aux/gen-lists-of-programs.sh b/build-aux/gen-lists-of-programs.sh index 2666492..cdbcd0a 100755 --- a/build-aux/gen-lists-of-programs.sh +++ b/build-aux/gen-lists-of-programs.sh @@ -32,6 +32,7 @@ build_if_possible_progs=' pinky stdbuf stty + timeout uptime users who @@ -120,7 +121,6 @@ normal_progs=' tail tee test - timeout touch tr true diff --git a/configure.ac b/configure.ac index a13295a..7c7c8c2 100644 --- a/configure.ac +++ b/configure.ac @@ -252,6 +252,8 @@ AC_CHECK_FUNCS([chroot], gl_ADD_PROG([optional_bin_progs], [chroot])) AC_CHECK_FUNCS([gethostid], gl_ADD_PROG([optional_bin_progs], [hostid])) +AC_CHECK_FUNCS([sigsuspend], + gl_ADD_PROG([optional_bin_progs], [timeout])) gl_WINSIZE_IN_PTEM diff --git a/src/timeout.c b/src/timeout.c index c38e365..d5a90fd 100644 --- a/src/timeout.c +++ b/src/timeout.c @@ -79,7 +79,7 @@ static int timed_out; static int term_signal = SIGTERM; /* same default as kill command. */ -static int monitored_pid; +static pid_t monitored_pid; static double kill_after; static bool foreground; /* whether to use another program group. */ static bool preserve_status; /* whether to use a timeout status or not. */ @@ -102,16 +102,6 @@ static struct option const long_options[] = {NULL, 0, NULL, 0} }; -static void -unblock_signal (int sig) -{ - sigset_t unblock_set; - sigemptyset (&unblock_set); - sigaddset (&unblock_set, sig); - if (sigprocmask (SIG_UNBLOCK, &unblock_set, NULL) != 0) - error (0, errno, _("warning: sigprocmask")); -} - /* Start the timeout after which we'll receive a SIGALRM. Round DURATION up to the next representable value. Treat out-of-range values as if they were maximal, @@ -121,10 +111,6 @@ static void settimeout (double duration, bool warn) { - /* We configure timers below so that SIGALRM is sent on expiry. - Therefore ensure we don't inherit a mask blocking SIGALRM. */ - unblock_signal (SIGALRM); - /* timer_settime() provides potentially nanosecond resolution. setitimer() is more portable (to Darwin for example), but only provides microsecond resolution and thus is @@ -165,11 +151,11 @@ settimeout (double duration, bool warn) /* send SIG avoiding the current process. */ static int -send_sig (int where, int sig) +send_sig (pid_t where, int sig) { /* If sending to the group, then ignore the signal, so we don't go into a signal loop. Note that this will ignore any of the - signals registered in install_signal_handlers(), that are sent after we + signals registered in install_cleanup(), that are sent after we propagate the first one, which hopefully won't be an issue. Note this process can be implicitly multithreaded due to some timer_settime() implementations, therefore a signal sent to the group, can be sent @@ -179,6 +165,14 @@ send_sig (int where, int sig) return kill (where, sig); } +/* Signal handler which is required for sigsuspend() to be interrupted + whenever SIGCHLD is received. */ +static void +chld (int sig) +{ +} + + static void cleanup (int sig) { @@ -330,7 +324,7 @@ parse_duration (const char* str) } static void -install_signal_handlers (int sigterm) +install_cleanup (int sigterm) { struct sigaction sa; sigemptyset (&sa.sa_mask); /* Allow concurrent calls to handler */ @@ -346,6 +340,33 @@ install_signal_handlers (int sigterm) sigaction (sigterm, &sa, NULL); /* user specified termination signal. */ } +/* Blocks all signals which were registered with cleanup + as signal handler. Return original mask in OLD_SET. */ +static void +block_cleanup (int sigterm, sigset_t *old_set) +{ + sigset_t block_set; + sigemptyset (&block_set); + sigaddset (&block_set, SIGALRM); + sigaddset (&block_set, SIGINT); + sigaddset (&block_set, SIGQUIT); + sigaddset (&block_set, SIGHUP); + sigaddset (&block_set, SIGTERM); + sigaddset (&block_set, sigterm); + if (sigprocmask (SIG_BLOCK, &block_set, old_set) != 0) + error (0, errno, _("warning: sigprocmask")); +} + +static void +unblock_signal (int sig) +{ + sigset_t unblock_set; + sigemptyset (&unblock_set); + sigaddset (&unblock_set, sig); + if (sigprocmask (SIG_UNBLOCK, &unblock_set, NULL) != 0) + error (0, errno, _("warning: sigprocmask")); +} + /* Try to disable core dumps for this process. Return TRUE if successful, FALSE otherwise. */ static bool @@ -433,10 +454,10 @@ main (int argc, char **argv) /* Setup handlers before fork() so that we handle any signals caused by child, without races. */ - install_signal_handlers (term_signal); + install_cleanup (term_signal); signal (SIGTTIN, SIG_IGN); /* Don't stop if background child needs tty. */ signal (SIGTTOU, SIG_IGN); /* Don't stop if background child needs tty. */ - signal (SIGCHLD, SIG_DFL); /* Don't inherit CHLD handling from parent. */ + signal (SIGCHLD, chld); /* Interrupt sigsuspend() when child exits. */ monitored_pid = fork (); if (monitored_pid == -1) @@ -462,11 +483,19 @@ main (int argc, char **argv) pid_t wait_result; int status; + /* We configure timers so that SIGALRM is sent on expiry. + Therefore ensure we don't inherit a mask blocking SIGALRM. */ + unblock_signal (SIGALRM); + settimeout (timeout, true); - while ((wait_result = waitpid (monitored_pid, &status, 0)) < 0 - && errno == EINTR) - continue; + /* Ensure we don't cleanup() after waitpid() reaps the child, + to avoid sending signals to a possibly different process. */ + sigset_t cleanup_set; + block_cleanup (term_signal, &cleanup_set); + + while ((wait_result = waitpid (monitored_pid, &status, WNOHANG)) == 0) + sigsuspend (&cleanup_set); /* Wait with cleanup signals unblocked. */ if (wait_result < 0) { -- 2.5.5 --------------22DE78771BC5CC669184DEE5-- From debbugs-submit-bounces@debbugs.gnu.org Fri Feb 10 01:15:10 2017 Received: (at 25624) by debbugs.gnu.org; 10 Feb 2017 06:15:11 +0000 Received: from localhost ([127.0.0.1]:33702 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cc4UA-0006XZ-Hn for submit@debbugs.gnu.org; Fri, 10 Feb 2017 01:15:10 -0500 Received: from midir.magicbluesmoke.com ([82.195.144.46]:57770 helo=mail.magicbluesmoke.com) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1cc4U8-0006XR-Mb for 25624@debbugs.gnu.org; Fri, 10 Feb 2017 01:15:08 -0500 Received: from [10.0.0.126] (c-73-170-254-78.hsd1.ca.comcast.net [73.170.254.78]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.magicbluesmoke.com (Postfix) with ESMTPSA id 953344AC8; Fri, 10 Feb 2017 06:15:07 +0000 (GMT) Subject: Re: bug#25624: [PATCH] timeout: Fix signal race in SIGALRM handling To: Tobias Stoeckmann References: <20170205122322.GA24928@localhost> <42ebfea0-ac17-8a89-69e3-942a130ee9c1@cs.ucla.edu> <20170205185037.GB6797@localhost> <139f2d2b-5bf8-a72b-ef25-9f45222b3ed7@draigBrady.com> <20170209200942.GA20270@localhost> <8f8221eb-8a10-2861-44b2-ce3c002c41c7@draigBrady.com> From: =?UTF-8?Q?P=c3=a1draig_Brady?= Message-ID: <231877f2-c6b9-3d98-0f1c-16ab08cbc263@draigBrady.com> Date: Thu, 9 Feb 2017 22:15:05 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <8f8221eb-8a10-2861-44b2-ce3c002c41c7@draigBrady.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 25624 Cc: 25624@debbugs.gnu.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 0.0 (/) On 09/02/17 21:39, Pádraig Brady wrote: > That looks good. > > I've changed the naming a bit, > ensured we also block the term_signal, > made the build of timeout optional on sigsuspend availability, > adjusted the commit message, > and added a NEWS entry. > > Marking this as done now. > > I'll push in your name later. Actually to ensure we indicate when the monitored command was terminated with a signal, we need to unblock that signal as follows. I'll merge that in too. diff --git a/src/timeout.c b/src/timeout.c index d5a90fd..6fe0542 100644 --- a/src/timeout.c +++ b/src/timeout.c @@ -516,6 +516,7 @@ main (int argc, char **argv) { /* exit with the signal flag set. */ signal (sig, SIG_DFL); + unblock_signal (sig); raise (sig); } status = sig + 128; /* what sh returns for signaled processes. * From unknown Sun Sep 07 11:47:01 2025 Received: (at fakecontrol) by fakecontrolmessage; To: internal_control@debbugs.gnu.org From: Debbugs Internal Request Subject: Internal Control Message-Id: bug archived. Date: Fri, 10 Mar 2017 12:24:04 +0000 User-Agent: Fakemail v42.6.9 # This is a fake control message. # # The action: # bug archived. thanks # This fakemail brought to you by your local debbugs # administrator