From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22c.google.com (mail-oi1-x22c.google.com [IPv6:2607:f8b0:4864:20::22c]) by sourceware.org (Postfix) with ESMTPS id 30ED33858D33 for ; Thu, 2 Mar 2023 16:42:16 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 30ED33858D33 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-oi1-x22c.google.com with SMTP id bp19so11261238oib.4 for ; Thu, 02 Mar 2023 08:42:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1677775335; 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=7Nvh1ppG3Pz0BvmtYlJNgpJX4baUqnIrK7w7bCIg+NY=; b=S2CjP6nU6+OR/MR03rwZ4SHMX85Qm4GFOkNhuuuasX7TtsNFMZG/8CxZSpiHQ08QRP KUqPQE3R71UPQT/96Dts2h/f7KCoIp7Z+YY/Hx4xIPTpkeTfuhFMUJ7dsT+ihoV9JjYU cUYclwYnRMI+7adNqF2RI0sERD35ddGRiM46OB6zWPDv3F9PLWGYGC8Bm2xI5tNsm9HU LTJwWOOT7VGogs7VJ2aH6z6gMXMY47aEJZ8q3saI9O1+PdoG43IsXah2ooVLOhrSK2Kl yP8bzzZJUubkctDoeEJ4M5pBWpYuJx8WxFztkWac9l93yJZFPZZRD/dzcnyXpLMNPZIO 6k4w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677775335; 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=7Nvh1ppG3Pz0BvmtYlJNgpJX4baUqnIrK7w7bCIg+NY=; b=zcaaMIgngpXEzWOzUdUliwf08dYXcIncp/my9Q/1sq99PGpqhL3aV8g4MB+XD8u5vT mL6GEZGWR+IZlTuy647NW7IA4hcWaZLYk50vbaWE4nqKzfyJhpKd0EPOrtyG8LGjynrO ry+Dhs0v8rLwWfWyJaHDJT1WI76u11QT+gKgUxrOjlhJAmU19ae9yMO6ArjdyqhJmPiC x+5QutDhqJW+2Md3SbmIYl+fFVSlDWhJJHW89ISDWtiUfH3exwfQae8PjRHAgi5kg1PT 6euQ++YECzVc8CuCIT7dYR5/37HZuk21ZZRnA7dQkW9E0wVIhp14iEgospeiusCO9hyo 4DnQ== X-Gm-Message-State: AO0yUKV7DJi/14Rv19DNpY6h0u9OYfViQOs20BpVj8JX0HLxJvpFBrYe oo+YBIsTNdi3fBT03jICNLBP/6IMbdqPSt5gBbg= X-Google-Smtp-Source: AK7set/XeHnddTAMAxYZ9rr+uNeKn8vGJD1vgghMJ7dH8AG23bXAL+EwdVB8N712CaHO0p57lizpYQ== X-Received: by 2002:aca:1c03:0:b0:383:f55f:76d9 with SMTP id c3-20020aca1c03000000b00383f55f76d9mr4554328oic.31.1677775335205; Thu, 02 Mar 2023 08:42:15 -0800 (PST) Received: from ?IPV6:2804:1b3:a7c3:d849:ed82:aa7c:6d0c:c8ba? ([2804:1b3:a7c3:d849:ed82:aa7c:6d0c:c8ba]) by smtp.gmail.com with ESMTPSA id q3-20020acac003000000b0037d74967ef6sm7289951oif.44.2023.03.02.08.42.13 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 02 Mar 2023 08:42:14 -0800 (PST) Message-ID: <588df669-00d8-ba15-8651-2a517d8cf922@linaro.org> Date: Thu, 2 Mar 2023 13:42:11 -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 v3] posix: Fix system blocks SIGCHLD erroneously [BZ #30163] Content-Language: en-US To: Adam Yi , libc-alpha@sourceware.org Cc: 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=-12.5 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 24/02/23 12:27, 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. Thanks for working on this, just some more minor comments below. > --- > stdlib/tst-system.c | 28 ++++++++++++++++++++ > support/Makefile | 2 ++ > support/dtotimespec-time64.c | 27 +++++++++++++++++++ > support/dtotimespec.c | 50 ++++++++++++++++++++++++++++++++++++ > support/shell-container.c | 13 ++++++++++ > support/timespec.h | 4 +++ > sysdeps/posix/system.c | 6 ++--- > 7 files changed, 127 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..736d2bf95b 100644 > --- a/stdlib/tst-system.c > +++ b/stdlib/tst-system.c > @@ -20,6 +20,8 @@ > #include > #include > #include > +#include > +#include > > #include > #include > @@ -71,6 +73,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 +170,18 @@ do_test (void) > xchmod (_PATH_BSHELL, st.st_mode); > } > > + { > + pthread_t long_sleep_thread, short_sleep_thread; > + > + TEST_COMPARE (pthread_create (&long_sleep_thread, NULL, > + sleep_and_check_sigchld, &(double) { 0.2 }), 0); > + TEST_COMPARE (pthread_create (&short_sleep_thread, NULL, > + sleep_and_check_sigchld, &(double) { 0.1 }), 0); > + > + TEST_COMPARE (pthread_join (short_sleep_thread, NULL), 0); > + TEST_COMPARE (pthread_join (long_sleep_thread, NULL), 0); Use xpthread_create and xpthread_join here: pthread_t short_sleep_thread = xpthread_create (NULL, sleep_and_check_sigchld, &(double) { 0.2 }); pthread_t long_sleep_thread = xpthread_create (NULL, sleep_and_check_sigchld, &(double) { 0.1 }); xpthread_join (short_sleep_thread); xpthread_join (sleep_and_check_sigchld); > + } > + > TEST_COMPARE (system (""), 0); > > return 0> diff --git a/support/Makefile b/support/Makefile > index b29b7eb505..48cd74581d 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 \ Ok. > 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 Ok. > diff --git a/support/dtotimespec.c b/support/dtotimespec.c > new file mode 100644 > index 0000000000..860403b83c > --- /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 (! (TYPE_MINIMUM (time_t) < sec)) > + 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); > + } > +} Ok. > diff --git a/support/shell-container.c b/support/shell-container.c > index e9ac9b6d04..a2be314ad9 100644 > --- a/support/shell-container.c > +++ b/support/shell-container.c > @@ -39,6 +39,7 @@ > #include > > #include > +#include > > /* Design considerations > > @@ -171,6 +172,17 @@ kill_func (char **argv) > return 0; > } > > +/* Emulate the "/bin/sleep" command. No suffix support. Options are > + ignored. */ > +static int > +sleep_func (char **argv) > +{ > + double sec = atof (argv[0]); Add some parsing check here: TEST_VERIFY_EXIT (argv[0] != NULL); char *endptr = NULL; double sec = strtod (argv[0], &endptr); TEST_VERIFY_EXIT (endptr != argv[0] && errno != ERANGE); /* No suffix support and only positive values. */ TEST_VERIFY_EXIT (sec >= 0.0); > + struct timespec ts = dtotimespec (sec); > + nanosleep (&ts, NULL); > + return 0; > +} > + > /* This is a list of all the built-in commands we understand. */ > static struct { > const char *name; > @@ -181,6 +193,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);