From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oa1-x2c.google.com (mail-oa1-x2c.google.com [IPv6:2001:4860:4864:20::2c]) by sourceware.org (Postfix) with ESMTPS id D0ADD3858D39 for ; Tue, 7 Mar 2023 12:55:34 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org D0ADD3858D39 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-oa1-x2c.google.com with SMTP id 586e51a60fabf-1755e639b65so14968271fac.3 for ; Tue, 07 Mar 2023 04:55:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1678193734; 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=QP7rjAuvXFWo3CRoGTkac9D0e8Oj3mxRhpJ3JRG5omI=; b=Ogvj3GRnk+HcwfwEi/zylTW/I8/4qRqIfEdZc/yRF5s7Lk7riHdY2aM6fz/WXStt9h FoeH6A64w/tx9MCJ7rK9NNU/Q6o9rwfr4zvjB4qoqLPbj2tN9luhlc2C9fs2x/sb95zV H+81evt79iWfh/54pCiIV6HnFyREceJf8yf0cvzh09n+Kr79uaO/N6W6OlmhROySCsOt BEohoKGTgWIvrfQHawMTJuZ7t97Y36qniruC9+UlCF9Bd1gL/8UihObRo01Yx/yRwrhh tAb30ys8ATMvR3izQNE5fH3GqSnSGTcvjhcmQfocvwXiyyzbrDyCtt46t8NKiYFhUVm3 1drw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678193734; 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=QP7rjAuvXFWo3CRoGTkac9D0e8Oj3mxRhpJ3JRG5omI=; b=FIoOZqKIKvNtJ7q0XmpJcaMTFR8WRO4qt5aMP9LDrkPkhmDGw12qy5RU4sTlrR6bQC 8o/diqRaOUnu6RmKDM3EWHSS1s35AG1ZMmQ4d+/xYB5stKA5UJMpCXYNExpSmMAb5vOe R1vEfUQxpa8Jm0JJduxD1vxseasDjhDgGktDWmeP6Met1NczdaPMOHPW3Y/xmzl8CtQn DbwexlrYRZjjwjIV6gWjEX65VKXsYgnDOwpe+tLg3tFVnksVkfvxJbeUZ7bpajQZhU4V z7tGEKA20BiIW9ZvYRXcr2OPyq8cU7n5F/d3/jjM2X3e0Yrtq5/muUIHZ/cII/r5N3B6 hUfQ== X-Gm-Message-State: AO0yUKUpirDQWVqEAxAiJmKTdRzqZifiQ8vkwqo3YtGbsstz6Is0Fgnp BAYa4bQvKBhNUs1o1O3/fZAYbQ== X-Google-Smtp-Source: AK7set902GXhY7rqhGu2FKZVIYjKG8iw4GPEKLOzj2dJ1XtML3yu+YzFI3JsaZx6jvCJWWfhxbu96A== X-Received: by 2002:a05:6870:808d:b0:176:4bc1:e5b5 with SMTP id q13-20020a056870808d00b001764bc1e5b5mr8612025oab.7.1678193733994; Tue, 07 Mar 2023 04:55: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 c22-20020a056870a59600b0017197629658sm5028584oam.56.2023.03.07.04.55.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 07 Mar 2023 04:55:33 -0800 (PST) Message-ID: Date: Tue, 7 Mar 2023 09:55:31 -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 v6] posix: Fix system blocks SIGCHLD erroneously [BZ #30163] Content-Language: en-US To: Adam Yi , libc-alpha@sourceware.org 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.9 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 07/03/23 09:30, 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. > > Signed-off-by: Adam Yi LGTM, thanks. Reviewed-by: Adhemerval Zanella > --- > 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);