From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mxout1.mail.janestreet.com (mxout1.mail.janestreet.com [38.105.200.78]) by sourceware.org (Postfix) with ESMTPS id 61C593858C83 for ; Wed, 1 Mar 2023 02:36:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 61C593858C83 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-qt1-f200.google.com ([209.85.160.200]) by mxgoog2.mail.janestreet.com with esmtps (TLS1.3:TLS_AES_128_GCM_SHA256:128) (Exim 4.96) id 1pXCKL-008ssY-0p for libc-alpha@sourceware.org; Tue, 28 Feb 2023 21:36:21 -0500 Received: by mail-qt1-f200.google.com with SMTP id g20-20020ac870d4000000b003b9c1013018so5778858qtp.18 for ; Tue, 28 Feb 2023 18:36:21 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=janestreet.com; s=google; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=EVVos1V3y6z0Tvd8uqUVPGvMSebumjoWLRPF3v2slII=; b=OC73X9xkDeFAj4wU36VDaySbSz3ElGcm7Of1WXVSDkv28Wp85Y3ZFb0CSDRnnW/Gdd j4wUCdtlIkoEMoxWyOsk05iprDUvz1q4arCJZgk6UQeFn3KXxZ4TUM3RmYsSt8jeWNtq jWhGivz6Qa520TedYOnYcl7+MB5f67eBOM5dU= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding: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=EVVos1V3y6z0Tvd8uqUVPGvMSebumjoWLRPF3v2slII=; b=Mp8qtDcpUIos7rOvGjSRTw3i7092elF6rHDP+Eni1oeWnvS3Tel1j0Tf2fO+9SSps5 Yec6BBh94QbOo6rYOmsO0gq4McGJgMAQX+i5gPSjvUfPztsJCbitTJ3+56loWgYwEzYh xP4biBjeKSbVQLVjYrHA2e7FuysU6elfnsM8Cju1R2e0RJUOpm9lfklKPe3U8sPjnBNW WXAr+MojC6DnyfYJ/Ids15N3j0dYpS4XPyFqPoBxJVYrWYFV+dJsxoftORirXjc5s9WX MQ1f0+USLu1Yxa1/A+gRGN5rYJhq8GIMu/E+V2Ut62flBEiB2PJ9j8b0YloPDp2m8+rw 6xew== X-Gm-Message-State: AO0yUKXaonqhFqa9SwMhV2c600Y3dHSkhRGVFQjNJJ5ohnIWiJS+xNQw 9BLFQ4jRFSCmkOCwwcpNPVrsaNrCQBXNn3Xi6LJlCvKn20wKhnMe6aDvraXiujyMWyvN/Egs/yb cf4mEuadzKxah7e6+RSQ0cAPynlJMRx8HXeSAeooDb3o= X-Received: by 2002:ae9:e219:0:b0:742:32aa:92be with SMTP id c25-20020ae9e219000000b0074232aa92bemr955372qkc.13.1677638180679; Tue, 28 Feb 2023 18:36:20 -0800 (PST) X-Google-Smtp-Source: AK7set/xuaRRCxGrDnfryV+xdmhb2+v4JRI+4IdqT6/NLjWrxiyJh+3Anch17C3qmqGg7EuQ+CZ76s9PIDSMFWCOexs= X-Received: by 2002:ae9:e219:0:b0:742:32aa:92be with SMTP id c25-20020ae9e219000000b0074232aa92bemr955367qkc.13.1677638180271; Tue, 28 Feb 2023 18:36:20 -0800 (PST) MIME-Version: 1.0 References: <9427e9a2-cf6c-c086-0350-c35c3063cb66@linaro.org> In-Reply-To: <9427e9a2-cf6c-c086-0350-c35c3063cb66@linaro.org> From: Adam Yi Date: Wed, 1 Mar 2023 10:36:04 +0800 Message-ID: Subject: Re: [PATCH] posix: Fix system blocks SIGCHLD erroneously [BZ #30163] To: Adhemerval Zanella Netto Cc: libc-alpha@sourceware.org, Adam Yi Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-11.2 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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: On Sat, Feb 25, 2023 at 4:18=E2=80=AFAM Adhemerval Zanella Netto wrote: > > > > On 24/02/23 15:00, Adam Yi wrote: > > Looks like some spaces/tabs got mixed up in the email. Please use the > > attachment instead. > > Next time just send a v2, it is better to keep track with you patchwork > instance [1]. > > [1] https://patchwork.sourceware.org/project/glibc/list/ Got it, thanks! > > > > > On Sat, Feb 25, 2023 at 1:31 AM 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() return= s, > >> even after the system() in thread A returns. > >> > >> Although POSIX does not require, glibc system implementation aims to b= e > >> thread and cancellation safe. This bug was introduced in > >> 5fb7fc96350575c9adb1316833e48ca11553be49 when we moved reverting signa= l > >> 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. > > Patch looks ok, I would recommend a v3 to enhance the testing a bit. Thanks for the suggestion! I've sent over a v3 to add split second support. > > >> --- > >> stdlib/tst-system.c | 27 +++++++++++++++++++++++++++ > >> support/shell-container.c | 10 ++++++++++ > >> sysdeps/posix/system.c | 6 +++--- > >> 3 files changed, 40 insertions(+), 3 deletions(-) > >> > >> diff --git a/stdlib/tst-system.c b/stdlib/tst-system.c > >> index 634acfe264..be95c2de44 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,19 @@ call_system (void *closure) > >> } > >> } > >> > >> +static void * > >> +sleep_and_check_sigchld (void *seconds) > >> +{ > >> + char cmd[namemax]; > >> + sprintf(cmd, "sleep %" PRIdPTR, (intptr_t) 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 +169,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, (void *) 2), 0); > >> + TEST_COMPARE (pthread_create (&short_sleep_thread, NULL, > >> + sleep_and_check_sigchld, (void *) 1), 0); > >> + > >> + TEST_COMPARE (pthread_join (short_sleep_thread, NULL), 0); > >> + TEST_COMPARE (pthread_join (long_sleep_thread, NULL), 0); > >> + } > >> + > >> TEST_COMPARE (system (""), 0); > >> > >> return 0; > > Do we really need to sleep for 2 seconds here? It adds more latency on > testcase run. > > >> diff --git a/support/shell-container.c b/support/shell-container.c > >> index e9ac9b6d04..083426550b 100644 > >> --- a/support/shell-container.c > >> +++ b/support/shell-container.c > >> @@ -171,6 +171,15 @@ kill_func (char **argv) > >> return 0; > >> } > >> > >> +/* Emulate the "/bin/sleep" command. Options are ignored. */ > >> +static int > >> +sleep_func (char **argv) > >> +{ > >> + int secs =3D atoi (argv[0]); > > I think it would be better add support for split second, similar to > coreutils. Something like: > > #include > #include > > /* I shamelessly copied it from gnulib, move it to support/dtotimespec.c.= */ > 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); > } > } > > static int > sleep_func (char **argv) > { > 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. */ > TEST_VERIFY_EXIT (sec >=3D 0.0); > > struct timespec ts =3D dtotimespec (sec); > /* We don't expect any signal here. */ > TEST_VERIFY_EXIT (nanosleep (&ts, NULL) =3D=3D 0); > } > > >> + sleep (secs); > >> + return 0; > >> +} > >> + > >> /* This is a list of all the built-in commands we understand. */ > >> static struct { > >> const char *name; > >> @@ -181,6 +190,7 @@ static struct { > >> { "cp", copy_func }, > >> { "exit", exit_func }, > >> { "kill", kill_func }, > >> + { "sleep", sleep_func }, > >> { NULL, NULL } > >> }; > >> > >> 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); > > Ok, thanks for spotting it. > > >> -- > >> 2.31.1