From patchwork Tue Feb 18 17:19:46 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 183601 Delivered-To: patch@linaro.org Received: by 2002:a92:1f12:0:0:0:0:0 with SMTP id i18csp6357814ile; Tue, 18 Feb 2020 09:20:09 -0800 (PST) X-Google-Smtp-Source: APXvYqy5yZoI4N6eGXVe93aXn7oJcKjWI1/2PgG+F6fuclEXIXqw+0UGBRd4Ob/IjTCxqFHW/tOz X-Received: by 2002:a9d:7e90:: with SMTP id m16mr15574240otp.227.1582046409369; Tue, 18 Feb 2020 09:20:09 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1582046409; cv=none; d=google.com; s=arc-20160816; b=vtWlD85e3EMy5StrDufnogFD/1GPibDBHytosU1YQJiOUz54D8yrOIqZjTI5EBJh3o Ltr0s4Wc0ZJqKiYtLK3PAylACAXKSMXTOTlDewdBXdCptCl0XYegVmPmR2F9NWowWc5M PJkEsNGbMvVHJgeFzRizb4+PcoovEp1ULhDdl8nSn0AcsC46I3QIUH9WORjVjoG6BVbr Ooo/35flnc/7mR+QXo5I7COIndPLVj403YAFIDCv/ZqkigMyPDe2yMi86ntKF/Kj75BF 4p6XOyA1oX/xTI9bOZwP9UuETlJ9ATsVvjlMpIVonJarNlBZnt3IjArcdfeIE1CWIsV7 hljw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=message-id:date:subject:to:from:dkim-signature:delivered-to:sender :list-help:list-post:list-archive:list-subscribe:list-unsubscribe :list-id:precedence:mailing-list:dkim-signature:domainkey-signature; bh=WIeT8dsCkbhENVRklaLFfnRHWl21h4dZQZVv2Szp/IE=; b=ji6sZB1oJPGU12w5EFVHIT7Nn6EJIDS28odycdgwGpfv7zttpXXRz84VwQ1Z0PyAST 21Q/hcCe4M7FRmqlA/GAutEmt3OMV5TEmjAn10+04p5iUCJhFxLa5/p0fSlwoU+RlGVf DU8iASIPgNwdZ5K7NYJ7gs6z69WvRQHESgOLrwOupT5zKX5dU17ngccIz6PpLGcdHAzC zilE8KC8dTNZEmqX8BQjN7oh3s+SHO3NnVlBkTFay+mfAQXZISyy0NZ6liIa8EreIRdh rWzvzPCCT82PX6pXcAP2Xj/Rn5xqJLEfLT6yk3XagoZo7FKXa4g5FAY0LLf5Xcn/qoqh VL2A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=GUSAM77b; dkim=pass header.i=@linaro.org header.s=google header.b=lxS5sYEF; spf=pass (google.com: domain of libc-alpha-return-109890-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-109890-patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id g9si2038256otq.68.2020.02.18.09.20.08 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 18 Feb 2020 09:20:09 -0800 (PST) Received-SPF: pass (google.com: domain of libc-alpha-return-109890-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) client-ip=209.132.180.131; Authentication-Results: mx.google.com; dkim=pass header.i=@sourceware.org header.s=default header.b=GUSAM77b; dkim=pass header.i=@linaro.org header.s=google header.b=lxS5sYEF; spf=pass (google.com: domain of libc-alpha-return-109890-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom="libc-alpha-return-109890-patch=linaro.org@sourceware.org"; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org DomainKey-Signature: a=rsa-sha1; c=nofws; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; q=dns; s= default; b=YOkq2Z97H7/2U+SG9OPvvZwpqWWVMlFDSlrz8iY/kaXr7Vlr47iIP kugdgp9/pvIfE6riNXYL8opF0EE6k0xj0tHlQUt8UTn4WgKjnPddd/LsnHC8psqt hg3Ng3pqhNEef6i9vVV1AeJvarxi4lzDwcImT+KPJC9oE76EdDPV04= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=sourceware.org; h=list-id :list-unsubscribe:list-subscribe:list-archive:list-post :list-help:sender:from:to:subject:date:message-id; s=default; bh=L1UMH/Q9VIeOpErPV2Ti0Aa3YA8=; b=GUSAM77bgLcKNKnf1zhbfidnrr6l u+oK64jYEhre3GN6VyS1O03i4YGHWYANGfQ/qNsRljUrx4IMCEyp7SaBAy1r4yx7 iuK+6H5VpMBBgE93lXnS4Kj/Sh5YSRlph7YEIJzZvm1gml4SqJiLQwX36+HmhMix cBgu+/gdJD3fCVM= Received: (qmail 109753 invoked by alias); 18 Feb 2020 17:19:58 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 109745 invoked by uid 89); 18 Feb 2020 17:19:57 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-22.0 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=1000000 X-HELO: mail-qt1-f170.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=from:to:subject:date:message-id; bh=WIeT8dsCkbhENVRklaLFfnRHWl21h4dZQZVv2Szp/IE=; b=lxS5sYEFI232UysDzZc0L6d5He42/WtKBfmqYnd2eotmPhnvNt8SBlFr365tb/hPfL ckcZG8dBi4zpa7/elaHCL2FJ/QkhJAi5EvV9Chutgc/mkbJTSWaXfXVo8uVqZAuYs8JQ IBUXbb+3nRibfYCgoO2LPFi7ok1L9skES0Roxm5SQOMh3eKkXfBaDe6WJrFtyJepiZFu XI1j2mTu+N23MgkKL7uTeblpatKMRHj3IeFIhhFsMPCv/Qr3C8s3ifDAio5e7KJXpy7z 46ts/ZF0ti6sTlesCl4uUsh4V8FB8v8Ax3INDf5USxLRv+G03wEOTYoGVRJ9hthZHbDH 1ryw== Return-Path: From: Adhemerval Zanella To: libc-alpha@sourceware.org Subject: [PATCH v4] Block all signals on timer_create thread (BZ#10815) Date: Tue, 18 Feb 2020 14:19:46 -0300 Message-Id: <20200218171946.12426-1-adhemerval.zanella@linaro.org> Changes from previous version: - Fixed sigtimer_set declaration. - Remove INTERNAL_SYSCALL_DECL usage. - Update copyright years. -- The behavior of the signal mask on threads created by timer_create for SIGEV_THREAD timers are implementation-defined and glibc explicit unblocks all signals before calling the user-defined function. This behavior, although not incorrect standard-wise, opens a race if a program using a blocked rt-signal plus sigwaitinfo (and without an installed signal handler for the rt-signal) receives a signal while executing the used-defined function for SIGEV_THREAD. A better alternative discussed in bug report is to rather block all signals (besides the internal ones not available to application usage). This patch fixes this issue by only unblocking SIGSETXID (used on set*uid function) and SIGCANCEL (used for thread cancellation). Checked on x86_64-linux-gnu and i686-linux-gnu. --- nptl/Makefile | 5 +- nptl/tst-cancel28.c | 80 ++++++++++++++++++ rt/Makefile | 7 +- rt/tst-timer-sigmask.c | 85 +++++++++++++++++++ sysdeps/unix/sysv/linux/internal-signals.h | 21 +++++ sysdeps/unix/sysv/linux/timer_routines.c | 94 ++++++++-------------- 6 files changed, 229 insertions(+), 63 deletions(-) create mode 100644 nptl/tst-cancel28.c create mode 100644 rt/tst-timer-sigmask.c -- 2.17.1 diff --git a/nptl/Makefile b/nptl/Makefile index 9c90af78f1..2911a3de37 100644 --- a/nptl/Makefile +++ b/nptl/Makefile @@ -266,7 +266,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \ tst-cancel11 tst-cancel12 tst-cancel13 tst-cancel14 tst-cancel15 \ tst-cancel16 tst-cancel17 tst-cancel18 tst-cancel19 tst-cancel20 \ tst-cancel21 tst-cancel22 tst-cancel23 tst-cancel24 \ - tst-cancel26 tst-cancel27 \ + tst-cancel26 tst-cancel27 tst-cancel28 \ tst-cancel-self tst-cancel-self-cancelstate \ tst-cancel-self-canceltype tst-cancel-self-testcancel \ tst-cleanup0 tst-cleanup1 tst-cleanup2 tst-cleanup3 tst-cleanup4 \ @@ -574,6 +574,9 @@ $(objpfx)tst-tls6.out: tst-tls6.sh $(objpfx)tst-tls5 \ $(BASH) $< $(common-objpfx) '$(test-via-rtld-prefix)' \ '$(test-wrapper-env)' '$(run-program-env)' > $@; \ $(evaluate-test) +$(objpfx)tst-cancel28: $(common-objpfx)rt/librt.so +else +$(objpfx)tst-cancel28: $(common-objpfx)rt/librt.a endif $(objpfx)tst-dlsym1: $(libdl) $(shared-thread-library) diff --git a/nptl/tst-cancel28.c b/nptl/tst-cancel28.c new file mode 100644 index 0000000000..f126dd8bcb --- /dev/null +++ b/nptl/tst-cancel28.c @@ -0,0 +1,80 @@ +/* Check if the thread created by POSIX timer using SIGEV_THREAD is + cancellable. + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + 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 +#include +#include +#include +#include + +#include +#include +#include + +static pthread_barrier_t barrier; +static pthread_t timer_thread; + +static void +cl (void *arg) +{ + xpthread_barrier_wait (&barrier); +} + +static void +thread_handler (union sigval sv) +{ + timer_thread = pthread_self (); + + xpthread_barrier_wait (&barrier); + + pthread_cleanup_push (cl, NULL); + while (1) + clock_nanosleep (CLOCK_REALTIME, 0, &(struct timespec) { 1, 0 }, NULL); + pthread_cleanup_pop (0); +} + +static int +do_test (void) +{ + struct sigevent sev = { 0 }; + sev.sigev_notify = SIGEV_THREAD; + sev.sigev_notify_function = &thread_handler; + + timer_t timerid; + TEST_COMPARE (timer_create (CLOCK_REALTIME, &sev, &timerid), 0); + + xpthread_barrier_init (&barrier, NULL, 2); + + struct itimerspec trigger = { 0 }; + trigger.it_value.tv_nsec = 1000000; + TEST_COMPARE (timer_settime (timerid, 0, &trigger, NULL), 0); + + xpthread_barrier_wait (&barrier); + + xpthread_cancel (timer_thread); + + xpthread_barrier_init (&barrier, NULL, 2); + xpthread_barrier_wait (&barrier); + + return 0; +} + +/* A stall in cancellation is a regression. */ +#define TIMEOUT 5 +#include diff --git a/rt/Makefile b/rt/Makefile index 935d968716..dab5d62a57 100644 --- a/rt/Makefile +++ b/rt/Makefile @@ -47,6 +47,7 @@ tests := tst-shm tst-timer tst-timer2 \ tst-timer3 tst-timer4 tst-timer5 \ tst-cpuclock2 tst-cputimer1 tst-cputimer2 tst-cputimer3 \ tst-shm-cancel +tests-internal := tst-timer-sigmask extra-libs := librt extra-libs-others := $(extra-libs) @@ -63,9 +64,11 @@ LDFLAGS-rt.so = -Wl,--enable-new-dtags,-z,nodelete $(objpfx)librt.so: $(shared-thread-library) ifeq (yes,$(build-shared)) -$(addprefix $(objpfx),$(tests)): $(objpfx)librt.so $(shared-thread-library) +$(addprefix $(objpfx),$(tests) $(tests-internal)): \ + $(objpfx)librt.so $(shared-thread-library) else -$(addprefix $(objpfx),$(tests)): $(objpfx)librt.a $(static-thread-library) +$(addprefix $(objpfx),$(tests)) $(tests-internal): \ + $(objpfx)librt.a $(static-thread-library) endif tst-mqueue7-ARGS = -- $(host-test-program-cmd) diff --git a/rt/tst-timer-sigmask.c b/rt/tst-timer-sigmask.c new file mode 100644 index 0000000000..1ed60e9d85 --- /dev/null +++ b/rt/tst-timer-sigmask.c @@ -0,0 +1,85 @@ +/* Check resulting signal mask from POSIX timer using SIGEV_THREAD. + Copyright (C) 2020 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + 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 +#include +#include +#include + +#include +#include +#include + +#include + +static pthread_barrier_t barrier; +static bool thread_handler_failure = false; + +static void +thread_handler (union sigval sv) +{ + sigset_t ss; + sigprocmask (SIG_BLOCK, NULL, &ss); + if (test_verbose > 0) + printf ("%s: blocked signal mask = { ", __func__); + for (int sig = 1; sig < NSIG; sig++) + { +#ifdef __linux__ + /* POSIX timers threads created to handle SIGEV_THREAD block all + signals except SIGKILL, SIGSTOP and glibc internals one except + SIGTIMER. */ + if (!sigismember (&ss, sig) + && (sig != SIGSETXID && sig != SIGCANCEL + && sig != SIGKILL && sig != SIGSTOP)) + { + thread_handler_failure = true; + } +#endif + if (test_verbose && sigismember (&ss, sig)) + printf ("%d, ", sig); + } + if (test_verbose > 0) + printf ("}\n"); + + xpthread_barrier_wait (&barrier); +} + +static int +do_test (void) +{ + struct sigevent sev = { 0 }; + sev.sigev_notify = SIGEV_THREAD; + sev.sigev_notify_function = &thread_handler; + + timer_t timerid; + TEST_COMPARE (timer_create (CLOCK_REALTIME, &sev, &timerid), 0); + + xpthread_barrier_init (&barrier, NULL, 2); + + struct itimerspec trigger = { 0 }; + trigger.it_value.tv_nsec = 1000000; + TEST_COMPARE (timer_settime (timerid, 0, &trigger, NULL), 0); + + xpthread_barrier_wait (&barrier); + + TEST_COMPARE (thread_handler_failure, false); + + return 0; +} + +#include diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h index e958522a07..3fbd4807d1 100644 --- a/sysdeps/unix/sysv/linux/internal-signals.h +++ b/sysdeps/unix/sysv/linux/internal-signals.h @@ -58,6 +58,11 @@ static const sigset_t sigall_set = { .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 } }; +static const sigset_t sigtimer_set = { + .__val = { [0] = __sigmask (SIGTIMER), + [1 ... _SIGSET_NWORDS-1] = 0 } +}; + /* Block all signals, including internal glibc ones. */ static inline void __libc_signal_block_all (sigset_t *set) @@ -76,6 +81,22 @@ __libc_signal_block_app (sigset_t *set) _NSIG / 8); } +/* Block only SIGTIMER and return the previous set on SET. */ +static inline void +__libc_signal_block_sigtimer (sigset_t *set) +{ + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_BLOCK, &sigtimer_set, set, + _NSIG / 8); +} + +/* Unblock only SIGTIMER and return the previous set on SET. */ +static inline void +__libc_signal_unblock_sigtimer (sigset_t *set) +{ + INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &sigtimer_set, set, + _NSIG / 8); +} + /* Restore current process signal mask. */ static inline void __libc_signal_restore_set (const sigset_t *set) diff --git a/sysdeps/unix/sysv/linux/timer_routines.c b/sysdeps/unix/sysv/linux/timer_routines.c index 00b7e018ba..63083f6f91 100644 --- a/sysdeps/unix/sysv/linux/timer_routines.c +++ b/sysdeps/unix/sysv/linux/timer_routines.c @@ -42,15 +42,9 @@ struct thread_start_data static void * timer_sigev_thread (void *arg) { - /* The parent thread has all signals blocked. This is a bit - surprising for user code, although valid. We unblock all - signals. */ - sigset_t ss; - sigemptyset (&ss); - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &ss, NULL, _NSIG / 8); + __libc_signal_unblock_sigtimer (NULL); struct thread_start_data *td = (struct thread_start_data *) arg; - void (*thrfunc) (sigval_t) = td->thrfunc; sigval_t sival = td->sival; @@ -68,65 +62,49 @@ timer_sigev_thread (void *arg) static void * timer_helper_thread (void *arg) { - /* Wait for the SIGTIMER signal, allowing the setXid signal, and - none else. */ - sigset_t ss; - sigemptyset (&ss); - __sigaddset (&ss, SIGTIMER); - /* Endless loop of waiting for signals. The loop is only ended when the thread is canceled. */ while (1) { siginfo_t si; - /* sigwaitinfo cannot be used here, since it deletes - SIGCANCEL == SIGTIMER from the set. */ - - /* XXX The size argument hopefully will have to be changed to the - real size of the user-level sigset_t. */ - int result = SYSCALL_CANCEL (rt_sigtimedwait, &ss, &si, NULL, _NSIG / 8); - - if (result > 0) + while (sigwaitinfo (&sigtimer_set, &si) < 0); + if (si.si_code == SI_TIMER) { - if (si.si_code == SI_TIMER) - { - struct timer *tk = (struct timer *) si.si_ptr; + struct timer *tk = (struct timer *) si.si_ptr; + + /* Check the timer is still used and will not go away + while we are reading the values here. */ + pthread_mutex_lock (&__active_timer_sigev_thread_lock); - /* Check the timer is still used and will not go away - while we are reading the values here. */ - pthread_mutex_lock (&__active_timer_sigev_thread_lock); + struct timer *runp = __active_timer_sigev_thread; + while (runp != NULL) + if (runp == tk) + break; + else + runp = runp->next; - struct timer *runp = __active_timer_sigev_thread; - while (runp != NULL) - if (runp == tk) - break; - else - runp = runp->next; + if (runp != NULL) + { + struct thread_start_data *td = malloc (sizeof (*td)); - if (runp != NULL) + /* There is not much we can do if the allocation fails. */ + if (td != NULL) { - struct thread_start_data *td = malloc (sizeof (*td)); - - /* There is not much we can do if the allocation fails. */ - if (td != NULL) - { - /* This is the signal we are waiting for. */ - td->thrfunc = tk->thrfunc; - td->sival = tk->sival; - - pthread_t th; - (void) pthread_create (&th, &tk->attr, - timer_sigev_thread, td); - } - } + /* This is the signal we are waiting for. */ + td->thrfunc = tk->thrfunc; + td->sival = tk->sival; - pthread_mutex_unlock (&__active_timer_sigev_thread_lock); + pthread_t th; + pthread_create (&th, &tk->attr, timer_sigev_thread, td); + } } - else if (si.si_code == SI_TKILL) - /* The thread is canceled. */ - pthread_exit (NULL); + + pthread_mutex_unlock (&__active_timer_sigev_thread_lock); } + else if (si.si_code == SI_TKILL) + /* The thread is canceled. */ + pthread_exit (NULL); } } @@ -160,14 +138,10 @@ __start_helper_thread (void) /* Block all signals in the helper thread but SIGSETXID. To do this thoroughly we temporarily have to block all signals here. The - helper can lose wakeups if SIGCANCEL is not blocked throughout, - but sigfillset omits it SIGSETXID. So, we add SIGCANCEL back - explicitly here. */ + helper can lose wakeups if SIGTIMER is not blocked throughout. */ sigset_t ss; - sigset_t oss; - sigfillset (&ss); - __sigaddset (&ss, SIGCANCEL); - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &ss, &oss, _NSIG / 8); + __libc_signal_block_app (&ss); + __libc_signal_block_sigtimer (NULL); /* Create the helper thread for this timer. */ pthread_t th; @@ -177,7 +151,7 @@ __start_helper_thread (void) __helper_tid = ((struct pthread *) th)->tid; /* Restore the signal mask. */ - INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_SETMASK, &oss, NULL, _NSIG / 8); + __libc_signal_restore_set (&ss); /* No need for the attribute anymore. */ (void) pthread_attr_destroy (&attr);