From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x232.google.com (mail-oi1-x232.google.com [IPv6:2607:f8b0:4864:20::232]) by sourceware.org (Postfix) with ESMTPS id 28D15385840F for ; Fri, 24 Feb 2023 20:18:15 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 28D15385840F 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-x232.google.com with SMTP id be35so259655oib.4 for ; Fri, 24 Feb 2023 12:18:15 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1677269894; 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=uH9YlgxRj2BwpN3VsO3JCzjFoWevQaRC6+3sSs5pPAE=; b=ZNt5a45dUBRxy1qhj5/F88PFOsT3YkFfKCUL7Vziq/5/HCpG3mYGnD/yVurVbcdnjS UwF07vxCVvwLP9Mzc4Qsm7OMroDJmrXVaBMMtzPoize7WifiCTYStgOM8vLb5G0rkM4r ZEzgOptTdcamSL1wYXMmnR6DBSRBTp6NFNvRBcXQMFrosyr4dP8UqfrK7eikrSUHcJIq w6+Py7TTLcUmQL2ASNK+VQ0ZHZ19Vg8crXmKl17nUCabDeU6lzJvBi9V8xn4aSwDIXZp 7axhJxmf84gNP+QrAX8tTO3g0cHvAWYv5eakhKv7843bZnU/YOYiidWXSSUE1ihfNGCT n6Ow== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1677269894; 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=uH9YlgxRj2BwpN3VsO3JCzjFoWevQaRC6+3sSs5pPAE=; b=tkr1kdILOAPGTSgsM5M17Tgy1wQL+bbiGPoz4L95zvRkBCDxYheTFyDGk56PgeAG9o p+rNzl0zGkjGzzPjxhaH4kTmNJy2XKRi57o/L55BSOiOB/6pY/Mt5Q3x8EkjXPHaxVR7 3N4WTc4wURZZhGY09uGR+wJIsrhVZYUOv6F2IBYvJiTdFroiZNCFMQuzDtFIpo8BvQRn e//tnvSsL7tzu+TTnqZ5yEQ4R5be5SLjNl/3P3FxY7LTENIxwnBgLAqgKJ873vIjHrJ4 k1oM5gt2uKK0Ufl+FwKHYA2esp0fT7ZJ2v4nK2DDkTWZXTfmYdPmtJ/Bcbj9gBtj6Y4y jZUA== X-Gm-Message-State: AO0yUKV15o3j/7aXzRPDjOlTjlSjk0pZFSeVdKs300B29tzweOxGHPX3 ZcKUF1aZC0rGZGBPI1REb108wQ== X-Google-Smtp-Source: AK7set8QRO+p/P1/jjqEeWUKghl+AqSWZ2YaA0uolVEP3lfK+bTcYakvHI3s8shLEhlBBziQj1G6vw== X-Received: by 2002:a05:6808:18a9:b0:36e:ce03:ac1e with SMTP id bi41-20020a05680818a900b0036ece03ac1emr434596oib.27.1677269894245; Fri, 24 Feb 2023 12:18:14 -0800 (PST) Received: from ?IPV6:2804:18:1014:c5c:94c2:5fcd:d836:cd2c? ([2804:18:1014:c5c:94c2:5fcd:d836:cd2c]) by smtp.gmail.com with ESMTPSA id e4-20020acab504000000b00383dd160a1dsm118686oif.30.2023.02.24.12.18.12 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 24 Feb 2023 12:18:13 -0800 (PST) Message-ID: <9427e9a2-cf6c-c086-0350-c35c3063cb66@linaro.org> Date: Fri, 24 Feb 2023 17:18:10 -0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0) Gecko/20100101 Thunderbird/102.7.2 Subject: Re: [PATCH] posix: Fix system blocks SIGCHLD erroneously [BZ #30163] Content-Language: en-US To: Adam Yi , libc-alpha@sourceware.org Cc: Adam Yi 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.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,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 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/ > > 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() 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. Patch looks ok, I would recommend a v3 to enhance the testing a bit. >> --- >> 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 = {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 = 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 = 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); } } static int sleep_func (char **argv) { 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. */ TEST_VERIFY_EXIT (sec >= 0.0); struct timespec ts = dtotimespec (sec); /* We don't expect any signal here. */ TEST_VERIFY_EXIT (nanosleep (&ts, NULL) == 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 = 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); Ok, thanks for spotting it. >> -- >> 2.31.1