From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x335.google.com (mail-ot1-x335.google.com [IPv6:2607:f8b0:4864:20::335]) by sourceware.org (Postfix) with ESMTPS id 10B0F3858D3C for ; Tue, 7 Mar 2023 12:18:33 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 10B0F3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org Received: by mail-ot1-x335.google.com with SMTP id o4-20020a9d6d04000000b00694127788f4so7013163otp.6 for ; Tue, 07 Mar 2023 04:18:33 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1678191513; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:from:to:cc:subject:date:message-id:reply-to; bh=1ZYl+EEoMlNwLVcNHRrlUQZFHHCfDVa5tv30ukfUOXY=; b=xPDfzD8RsWyWXBkZDU5ny0qQjbsHjbB6kNdn29T78uFzjbPr90T7/14yA9Y9OyuwI5 ZylvPj7sBHyIPSRBiWEbfvTE4SMa4NqpsCrSkhq3ZBAXUF+St5fSdmA703ET913eeTYx HnHgHOn6skOmjBAs8urF2rw/MfLxK5aTt4AmAShYKQOO7F0gl8sOfmYOO27Hb0wvkUR2 8fMwoCU7rVNZU6z4zV7ZXDZCJvxZSy2lutNj2WY5CzMsymcwCwvU4BrRrt2QDeSru+T1 r9fcd/n/Ly7ZLLwr1jLBXWRNpdrMU08a5Th2bWYG0wDyherXQ26GekD6jYSb4anmQBJS 4MUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678191513; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=1ZYl+EEoMlNwLVcNHRrlUQZFHHCfDVa5tv30ukfUOXY=; b=mHwU8M3BT5k/OzNWJ2C2VNw34fhZH3Qt61LFYCDSNRpI1PJw45yjwqQt04X9QK4jwM i91CipiuGk9WsNdtOg/IpI+BNKTZlSGU7PyfFr2KpyDFObimBb+YECHNbSvfcej9ne8f MNhBDQemjH/uRkosTSO9ksMBJM7H+o3SEqwrXw8q9oB1zJROHxeDp9qqhyoTSnBDi8Ga kxKQmRguzSvq72kn1y9fy4nPsqpCsej6oElfERZpNVnWD8Ev8I56KUFg8NI1/eoobcs8 oZ1GWoheE3h6HmvaR2oo7Gg6B0hDlCWD/5jZoP0aa9JDx8wrOMvYFM8xjEnbI4kiS2g9 flVQ== X-Gm-Message-State: AO0yUKUVgWywLRHlNGTrKfTLlPWU8WXj+UiZDY1kT1+fGV956zVypBD+ MPf/3I6ITQ466xLtvZVy/YMshg== X-Google-Smtp-Source: AK7set9YKvl9YUiWGeF9mePeN0nnm8tqo9Z7AqfiroYuOID+sNEaohmFVoiV2eZ8iPFpXGsBOg49OQ== X-Received: by 2002:a9d:312:0:b0:68d:4708:c151 with SMTP id 18-20020a9d0312000000b0068d4708c151mr6879189otv.6.1678191513081; Tue, 07 Mar 2023 04:18:33 -0800 (PST) Received: from ?IPV6:2804:1b3:a7c3:d849:85a1:d2e8:5a25:72e7? ([2804:1b3:a7c3:d849:85a1:d2e8:5a25:72e7]) by smtp.gmail.com with ESMTPSA id e26-20020a05683013da00b0068663820588sm2198486otq.44.2023.03.07.04.18.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Mar 2023 04:18:32 -0800 (PST) Message-ID: <6ec79ed7-7798-9566-5a17-4bf47edbf31b@linaro.org> Date: Tue, 7 Mar 2023 09:18:29 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH v5] posix: Fix system blocks SIGCHLD erroneously [BZ #30163] Content-Language: en-US To: Adam Yi , libc-alpha@sourceware.org, Florian Weimer Cc: schwab@suse.de, i@adamyi.com References: From: Adhemerval Zanella Netto Organization: Linaro In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,KAM_SHORT,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On 06/03/23 22:52, Adam Yi wrote: > Fix bug that SIGCHLD is erroneously blocked forever in the following > scenario: > > 1. Thread A calls system but hasn't returned yet > 2. Thread B calls another system but returns > > SIGCHLD would be blocked forever in thread B after its system() returns, > even after the system() in thread A returns. > > Although POSIX does not require, glibc system implementation aims to be > thread and cancellation safe. This bug was introduced in > 5fb7fc96350575c9adb1316833e48ca11553be49 when we moved reverting signal > mask to happen when the last concurrently running system returns, > despite that signal mask is per thread. This commit reverts this logic > and adds a test. LGTM, thanks. I *think* you might need to resend it with "Signed-off-by:" to mark this as a contribution under DCO. I don't have access to the FSF records, Florian might help me (sorry to not have it checked earlier). > --- > stdlib/tst-system.c | 26 +++++++++++++++++++ > support/Makefile | 2 ++ > support/dtotimespec-time64.c | 27 +++++++++++++++++++ > support/dtotimespec.c | 50 ++++++++++++++++++++++++++++++++++++ > support/shell-container.c | 28 ++++++++++++++++++++ > support/timespec.h | 4 +++ > sysdeps/posix/system.c | 6 ++--- > 7 files changed, 140 insertions(+), 3 deletions(-) > create mode 100644 support/dtotimespec-time64.c > create mode 100644 support/dtotimespec.c > > diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c > index 634acfe264..47a0afe6bf 100644 > --- a/stdlib/tst-system.c > +++ b/stdlib/tst-system.c > @@ -25,6 +25,7 @@ > #include > #include > #include > +#include > #include > > static char *tmpdir; > @@ -71,6 +72,20 @@ call_system (void *closure) > } > } > > +static void * > +sleep_and_check_sigchld (void *closure) > +{ > + double *seconds = (double *) closure; > + char cmd[namemax]; > + sprintf (cmd, "sleep %lf" , *seconds); > + TEST_COMPARE (system (cmd), 0); > + > + sigset_t blocked = {0}; > + TEST_COMPARE (sigprocmask (SIG_BLOCK, NULL, &blocked), 0); > + TEST_COMPARE (sigismember (&blocked, SIGCHLD), 0); > + return NULL; > +} > + > static int > do_test (void) > { > @@ -154,6 +169,17 @@ do_test (void) > xchmod (_PATH_BSHELL, st.st_mode); > } > > + { > + pthread_t long_sleep_thread = xpthread_create (NULL, > + sleep_and_check_sigchld, > + &(double) { 0.2 }); > + pthread_t short_sleep_thread = xpthread_create (NULL, > + sleep_and_check_sigchld, > + &(double) { 0.1 }); > + xpthread_join (short_sleep_thread); > + xpthread_join (long_sleep_thread); > + } > + > TEST_COMPARE (system (""), 0); > > return 0; > diff --git a/support/Makefile b/support/Makefile > index d52c472755..05b31159ea 100644 > --- a/support/Makefile > +++ b/support/Makefile > @@ -32,6 +32,8 @@ libsupport-routines = \ > check_hostent \ > check_netent \ > delayed_exit \ > + dtotimespec \ > + dtotimespec-time64 \ > ignore_stderr \ > next_to_fault \ > oom_error \ > diff --git a/support/dtotimespec-time64.c b/support/dtotimespec-time64.c > new file mode 100644 > index 0000000000..b3d5e351e3 > --- /dev/null > +++ b/support/dtotimespec-time64.c > @@ -0,0 +1,27 @@ > +/* Convert double to timespec. 64-bit time support. > + Copyright (C) 2011-2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library and is also part of gnulib. > + Patches to this file should be submitted to both projects. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +#include > + > +#if __TIMESIZE != 64 > +# define timespec __timespec64 > +# define time_t __time64_t > +# define dtotimespec dtotimespec_time64 > +# include "dtotimespec.c" > +#endif > diff --git a/support/dtotimespec.c b/support/dtotimespec.c > new file mode 100644 > index 0000000000..cde5b4d74c > --- /dev/null > +++ b/support/dtotimespec.c > @@ -0,0 +1,50 @@ > +/* Convert double to timespec. > + Copyright (C) 2011-2023 Free Software Foundation, Inc. > + This file is part of the GNU C Library and is also part of gnulib. > + Patches to this file should be submitted to both projects. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + . */ > + > +/* Convert the double value SEC to a struct timespec. Round toward > + positive infinity. On overflow, return an extremal value. */ > + > +#include > +#include > + > +struct timespec > +dtotimespec (double sec) > +{ > + if (sec <= TYPE_MINIMUM (time_t)) > + return make_timespec (TYPE_MINIMUM (time_t), 0); > + else if (sec >= 1.0 + TYPE_MAXIMUM (time_t)) > + return make_timespec (TYPE_MAXIMUM (time_t), TIMESPEC_HZ - 1); > + else > + { > + time_t s = sec; > + double frac = TIMESPEC_HZ * (sec - s); > + long ns = frac; > + ns += ns < frac; > + s += ns / TIMESPEC_HZ; > + ns %= TIMESPEC_HZ; > + > + if (ns < 0) > + { > + s--; > + ns += TIMESPEC_HZ; > + } > + > + return make_timespec (s, ns); > + } > +} > diff --git a/support/shell-container.c b/support/shell-container.c > index ffa3378b5e..b1f9e793c1 100644 > --- a/support/shell-container.c > +++ b/support/shell-container.c > @@ -37,6 +37,7 @@ > #include > > #include > +#include > > /* Design considerations > > @@ -169,6 +170,32 @@ kill_func (char **argv) > return 0; > } > > +/* Emulate the "/bin/sleep" command. No suffix support. Options are > + ignored. */ > +static int > +sleep_func (char **argv) > +{ > + if (argv[0] == NULL) > + { > + fprintf (stderr, "sleep: missing operand\n"); > + return 1; > + } > + char *endptr = NULL; > + double sec = strtod (argv[0], &endptr); > + if (endptr == argv[0] || errno == ERANGE || sec < 0) > + { > + fprintf (stderr, "sleep: invalid time interval '%s'\n", argv[0]); > + return 1; > + } > + struct timespec ts = dtotimespec (sec); > + if (nanosleep (&ts, NULL) < 0) > + { > + fprintf (stderr, "sleep: failed to nanosleep: %s\n", strerror (errno)); > + return 1; > + } > + return 0; > +} > + > /* This is a list of all the built-in commands we understand. */ > static struct { > const char *name; > @@ -179,6 +206,7 @@ static struct { > { "cp", copy_func }, > { "exit", exit_func }, > { "kill", kill_func }, > + { "sleep", sleep_func }, > { NULL, NULL } > }; > > diff --git a/support/timespec.h b/support/timespec.h > index 77b1e4e8d6..9559836d4c 100644 > --- a/support/timespec.h > +++ b/support/timespec.h > @@ -57,6 +57,8 @@ int support_timespec_check_in_range (struct timespec expected, > struct timespec observed, > double lower_bound, double upper_bound); > > +struct timespec dtotimespec (double sec) __attribute__((const)); > + > #else > struct timespec __REDIRECT (timespec_add, (struct timespec, struct timespec), > timespec_add_time64); > @@ -82,6 +84,8 @@ int __REDIRECT (support_timespec_check_in_range, (struct timespec expected, > double lower_bound, > double upper_bound), > support_timespec_check_in_range_time64); > + > +struct timespec __REDIRECT (dtotimespec, (double sec), dtotimespec_time64); > #endif > > /* Check that the timespec on the left represents a time before the > diff --git a/sysdeps/posix/system.c b/sysdeps/posix/system.c > index 2335a99184..d77720a625 100644 > --- a/sysdeps/posix/system.c > +++ b/sysdeps/posix/system.c > @@ -179,16 +179,16 @@ do_system (const char *line) > as if the shell had terminated using _exit(127). */ > status = W_EXITCODE (127, 0); > > + /* sigaction can not fail with SIGINT/SIGQUIT used with old > + disposition. Same applies for sigprocmask. */ > DO_LOCK (); > if (SUB_REF () == 0) > { > - /* sigaction can not fail with SIGINT/SIGQUIT used with old > - disposition. Same applies for sigprocmask. */ > __sigaction (SIGINT, &intr, NULL); > __sigaction (SIGQUIT, &quit, NULL); > - __sigprocmask (SIG_SETMASK, &omask, NULL); > } > DO_UNLOCK (); > + __sigprocmask (SIG_SETMASK, &omask, NULL); > > if (ret != 0) > __set_errno (ret);