From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mxout5.mail.janestreet.com (mxout5.mail.janestreet.com [64.215.233.18]) by sourceware.org (Postfix) with ESMTPS id 6F78C3858D33 for ; Fri, 3 Mar 2023 05:11:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6F78C3858D33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=janestreet.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=janestreet.com Received: from mail-qv1-f72.google.com ([209.85.219.72]) by mxgoog2.mail.janestreet.com with esmtps (TLS1.3:TLS_AES_128_GCM_SHA256:128) (Exim 4.96) id 1pXxhy-0014II-2B for libc-alpha@sourceware.org; Fri, 03 Mar 2023 00:11:55 -0500 Received: by mail-qv1-f72.google.com with SMTP id lt7-20020a056214570700b0057290f3623eso859804qvb.3 for ; Thu, 02 Mar 2023 21:11:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=janestreet.com; s=google; t=1677820315; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=IupXwh4WPSwWetwLJPdeHvbF0vI+Gf1CmKpYTW6xIRw=; b=r8zSoZtJCFL2IipjopKzxfEJjjRpuPM3Ah0Nyhoe7kgS0lxwyz+AnXFwZ01mmRz29d 2cAssuMGSZQLXsLSvBoqWcmqiU/qwv7QbSdAGqMyus11G0YmdUX1rjjGOVqhvB0JfTwE QJUKxsBhzvPagTPl/mbduHfqoyESao1poq/yE= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677820315; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=IupXwh4WPSwWetwLJPdeHvbF0vI+Gf1CmKpYTW6xIRw=; b=8DcARsAikijgP/1W7uU/M7OIMFvlExoaQ9t4HIuIr05uM3RacXKprOANgEYk9g2RI4 u1ypymQ+++HaVm+6SlOo0Z1vJpM6CMY0UDgW9FkAguneBXqO5qC+bFyPC/Hsf/Aw4gwX 2kZ1pbo+tFOzutCmBiAaOjzG/Rpi8GBfIKA1qd4XxDtdgabhMTzWfuAJjVQarCAxxOvh iLJ+OBWJHgxwqdnrAy1/hA7CsgZ2zp7dU9j9xI+25reI8ULowZeJ/dCjQTFNgmVk4uut RDjHOkPqTm26s+krIqewtwWFe1PRZqKpcO/V3TTLDmXnvEr3yXO28yeeL/fdwPxqzW96 +aIQ== X-Gm-Message-State: AO0yUKVbaHPxIiVM6hlvlAeWzoMEIDqQf7W5D5RjtzmtpaTUkWC2Gjj/ n2V9JKisCDehTkhVoXMFCnhH3bAdOY+z0T0opGH15BQj38Y3Xd3+u7bLoGJeF3irj6xPKvfnjsC IgIC3vRortavuFo56+Y1UZ9IFlwWBBvMGrpI= X-Received: by 2002:ac8:4581:0:b0:3b7:fda5:262d with SMTP id l1-20020ac84581000000b003b7fda5262dmr200212qtn.8.1677820315263; Thu, 02 Mar 2023 21:11:55 -0800 (PST) X-Google-Smtp-Source: AK7set/6wEbn+MZI2LDuRF/8S1MFI0xnpl6GHBcYXTci7e6ORKaIREzXaPjNjBKVE08THqQlCoFy1jUQCfK1l8T3vZQ= X-Received: by 2002:ac8:4581:0:b0:3b7:fda5:262d with SMTP id l1-20020ac84581000000b003b7fda5262dmr200203qtn.8.1677820314871; Thu, 02 Mar 2023 21:11:54 -0800 (PST) MIME-Version: 1.0 References: <588df669-00d8-ba15-8651-2a517d8cf922@linaro.org> In-Reply-To: <588df669-00d8-ba15-8651-2a517d8cf922@linaro.org> From: Adam Yi Date: Fri, 3 Mar 2023 13:11:38 +0800 Message-ID: Subject: Re: [PATCH v3] posix: Fix system blocks SIGCHLD erroneously [BZ #30163] To: Adhemerval Zanella Netto Cc: libc-alpha@sourceware.org, i@adamyi.com Content-Type: multipart/alternative; boundary="000000000000dd832905f5f7fbbe" 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,HTML_MESSAGE,KAM_SHORT,SPF_HELO_PASS,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: --000000000000dd832905f5f7fbbe Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Thanks for reviewing! On Fri, Mar 3, 2023 at 12:42=E2=80=AFAM Adhemerval Zanella Netto < adhemerval.zanella@linaro.org> wrote: > > > 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 =3D (double *) closure; > > + char cmd[namemax]; > > + sprintf (cmd, "sleep %lf" , *seconds); > > + TEST_COMPARE (system (cmd), 0); > > + > > + sigset_t blocked =3D {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 =3D xpthread_create (NULL, > sleep_and_check_sigchld, &(double) { 0.2 }); > pthread_t long_sleep_thread =3D xpthread_create (NULL, > sleep_and_check_sigchld, &(double) { 0.1 }); > xpthread_join (short_sleep_thread); > xpthread_join (sleep_and_check_sigchld); > Will do. Thanks! > > > + } > > + > > 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 =3D \ > > 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 !=3D 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 =3D sec; > > + double frac =3D TIMESPEC_HZ * (sec - s); > > + long ns =3D frac; > > + ns +=3D ns < frac; > > + s +=3D ns / TIMESPEC_HZ; > > + ns %=3D TIMESPEC_HZ; > > + > > + if (ns < 0) > > + { > > + s--; > > + ns +=3D 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 =3D atof (argv[0]); > > Add some parsing check here: > > TEST_VERIFY_EXIT (argv[0] !=3D NULL); > char *endptr =3D NULL; > double sec =3D strtod (argv[0], &endptr); > TEST_VERIFY_EXIT (endptr !=3D argv[0] && errno !=3D ERANGE); > /* No suffix support and only positive values. */ > TEST_VERIFY_EXIT (sec >=3D 0.0); > I'm not sure if TEST_VERIFY_EXIT is the correct thing to do here, since it's the mocked /bin/sh, not directly part of the test. It's weird that providing invalid argument to `sleep` would crash the shell. Would it make more sense to print an error to stderr and return 1? FWIW, I didn't add parsing check here because I saw that other functions here do not have any sanity checks. exit and kill are just calling atoi without checking for errors and they don't check return values either. There are some checks in copy_func but it doesn't even check that there are two arguments before referencing them. > > + struct timespec ts =3D 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 =3D W_EXITCODE (127, 0); > > > > + /* sigaction can not fail with SIGINT/SIGQUIT used with old > > + disposition. Same applies for sigprocmask. */ > > DO_LOCK (); > > if (SUB_REF () =3D=3D 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 !=3D 0) > > __set_errno (ret); > --000000000000dd832905f5f7fbbe--