From patchwork Sat Jun 18 17:25:59 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella X-Patchwork-Id: 70391 Delivered-To: patch@linaro.org Received: by 10.140.28.4 with SMTP id 4csp807072qgy; Sat, 18 Jun 2016 10:26:25 -0700 (PDT) X-Received: by 10.66.101.231 with SMTP id fj7mr10022267pab.59.1466270785232; Sat, 18 Jun 2016 10:26:25 -0700 (PDT) Return-Path: Received: from sourceware.org (server1.sourceware.org. [209.132.180.131]) by mx.google.com with ESMTPS id fa5si21941343pab.13.2016.06.18.10.26.24 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 18 Jun 2016 10:26:25 -0700 (PDT) Received-SPF: pass (google.com: domain of libc-alpha-return-70785-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; spf=pass (google.com: domain of libc-alpha-return-70785-patch=linaro.org@sourceware.org designates 209.132.180.131 as permitted sender) smtp.mailfrom=libc-alpha-return-70785-patch=linaro.org@sourceware.org; dmarc=fail (p=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:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; q=dns; s=default; b=fJh7K4VvGA4bmt7J geVdfldbVK0qtBN05kJS7SRP6oUyCbIO0Q9OrpXSc9rAqMwXD6L2LsxYBCYiZIzL tT9uDEo2e4Q7tqFc3NYLhhrQLZ48jtUniU85Ebt6/OKpUnixDtPsOVxZ7K54Js4q VqUA8Fmb0PcIivrway5rKzf1cz0= 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:subject:to:references:cc:from:message-id:date :mime-version:in-reply-to:content-type :content-transfer-encoding; s=default; bh=R9f6+C/TkerXxPWRFdC5eE Eykws=; b=ohrRb/BA2uyrD9ACkxiMSez+c+sdntDd0QkZe/Agg5S0n/etMdKOIy Uc0SbfRV9/EeoE/U8lLbqIboD2DKavBmcmwGrTf32NrkGIQa1mC6AOyY0jMLv0G0 9BWHMWoz+gjVHS+KrsI7WkHYPGhzrk7Rq6YMe5VVwzB/OUqUqC5GU= Received: (qmail 115553 invoked by alias); 18 Jun 2016 17:26:16 -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 115498 invoked by uid 89); 18 Jun 2016 17:26:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.2 spammy=H*r:sk:16.2016, H*M:2090902 X-HELO: mail-yw0-f174.google.com X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=3clLOxndtNbMdzxtBe3e5UgaEQjE9jNxVPIPIkJIkUY=; b=Oqwr6IT2mx2cUxTgd8RfRaNG/DAfL3FqDw9zGl4wsOYyzmLj4VzH29toT7aGKrp3EW gQIdBy13uP0pd/1J4t0dhlas0RBe3W3aoCgsVY+J90blfS9ULxarhmScrgS+sIjSXdLa cVXieginzANrvkBdYUyeDOdv1m6E0lWyJLumYScUO1u/8VuaDJatrzXGr35cYYagUXQX mRo93cPqBOGgpGMbP22xtihLYDva+qQR/nfJn8Hc1s2cFy6GMCHTkFWMgtOrlF4s5wfh 24ldQ3J1Fs/HvzekKbDdvSPP/bLoltPIcRO2/n2vCM+7s3TDCf1o3Z2hAdUnwqYnCY7T /BIg== X-Gm-Message-State: ALyK8tI14FT4c5a6SlOy8ozG7gG6GASDLCz12kge6dF30g5KpUXQyn7OOWOKqSFR9+9mrALD X-Received: by 10.37.118.80 with SMTP id r77mr4441100ybc.44.1466270762527; Sat, 18 Jun 2016 10:26:02 -0700 (PDT) Subject: Re: [PATCH] Refactor Linux raise implementation (BZ#15368) To: Zack Weinberg References: <1466188988-19954-1-git-send-email-adhemerval.zanella@linaro.org> Cc: GNU C Library From: Adhemerval Zanella Message-ID: <57658427.2090902@linaro.org> Date: Sat, 18 Jun 2016 14:25:59 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: Thanks for reviews, comment below: On 18/06/2016 11:09, Zack Weinberg wrote: > On Fri, Jun 17, 2016 at 2:43 PM, Adhemerval Zanella > wrote: >> This patch changes both the nptl and libc Linux raise implementation >> to avoid the issues described in BZ#15368. > > It would be nice to have comments in these files which explain why it > is necessary to block all signals and why the cached values are not > safe to use. The old comment that you deleted ("raise is async-safe > and could be called while fork/vfork temporarily invalidated the PID") > would be a good start. > Indeed, I did not remove them in this new version below. I also extended the comment a bit explaining why to block the application signals. >> The strategy used is >> summarized in bug report first comment: >> >> 1. Block all signals (including internal NPTL ones); > > The code appears to block all signals *except* the internal NPTL ones. > If I understand Rich's description of the problem correctly, that is > wrong. Right, we need to block only user defined handler that might fork/vfork. The call itself is OK, but I will change the comments describing why there is no need to block GLIBC internal handlers. > >> +static inline int >> +__libc_signal_block_all (const sigset_t *set) >> +{ > > Type-safety error here: the 'set' argument is written to and should > not be 'const'. It compiles because INTERNAL_SYSCALL() erases types, > but the compiler would be entitled to assume that 'set' is unchanged > after the call. Ack. > >> +static inline int >> +__libc_signal_block_app (const sigset_t *set) > > Same type-safety error here. Ack. > >> + sigset_t set; >> + __libc_signal_block_app (&set); >> + >> + pid_t pid = __getpid (); > > If I'm reading the code correctly, __getpid does *not* bypass the cache. > >> + pid_t tid = INLINE_SYSCALL (gettid, 0); > > INTERNAL_SYSCALL() here too? gettid() can't fail, so there's no point > generating the code to potentially set errno. > Right, I will use INTERNAL_SYSCALL. > zw > I think this version addresses all the issues you raised: -- * sysdeps/unix/sysv/linux/nptl-signals.h (__nptl_clear_internal_signals): New function. (__libc_signal_block_all): Likewise. (__libc_signal_block_app): Likewise. (__libc_signal_restore_set): Likewise. * sysdeps/unix/sysv/linux/pt-raise.c (raise): Use Linux raise.c implementation. * sysdeps/unix/sysv/linux/raise.c (raise): Reimplement to not use the cached pid/tid value in pthread structure. diff --git a/sysdeps/unix/sysv/linux/nptl-signals.h b/sysdeps/unix/sysv/linux/nptl-signals.h index 01f34c2..6525b6d 100644 --- a/sysdeps/unix/sysv/linux/nptl-signals.h +++ b/sysdeps/unix/sysv/linux/nptl-signals.h @@ -39,5 +39,42 @@ __nptl_is_internal_signal (int sig) return (sig == SIGCANCEL) || (sig == SIGTIMER) || (sig == SIGSETXID); } +static inline void +__nptl_clear_internal_signals (sigset_t *set) +{ + __sigdelset (set, SIGCANCEL); + __sigdelset (set, SIGTIMER); + __sigdelset (set, SIGSETXID); +} + +#define SIGALL_SET \ + ((__sigset_t) { .__val = {[0 ... _SIGSET_NWORDS-1 ] = -1 } }) + +static inline int +__libc_signal_block_all (sigset_t *set) +{ + INTERNAL_SYSCALL_DECL (err); + return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &SIGALL_SET, + set, _NSIG / 8); +} + +static inline int +__libc_signal_block_app (sigset_t *set) +{ + sigset_t allset = SIGALL_SET; + __nptl_clear_internal_signals (&allset); + INTERNAL_SYSCALL_DECL (err); + return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_BLOCK, &allset, set, + _NSIG / 8); +} + +static inline int +__libc_signal_restore_set (const sigset_t *set) +{ + INTERNAL_SYSCALL_DECL (err); + return INTERNAL_SYSCALL (rt_sigprocmask, err, 4, SIG_SETMASK, set, NULL, + _NSIG / 8); +} + /* Used to communicate with signal handler. */ extern struct xid_command *__xidcmd attribute_hidden; diff --git a/sysdeps/unix/sysv/linux/pt-raise.c b/sysdeps/unix/sysv/linux/pt-raise.c index 715bbe9..5f6dea1 100644 --- a/sysdeps/unix/sysv/linux/pt-raise.c +++ b/sysdeps/unix/sysv/linux/pt-raise.c @@ -1,4 +1,5 @@ -/* Copyright (C) 2002-2016 Free Software Foundation, Inc. +/* ISO C raise function for libpthread. + Copyright (C) 2002-2016 Free Software Foundation, Inc. This file is part of the GNU C Library. Contributed by Ulrich Drepper , 2002. @@ -16,22 +17,4 @@ License along with the GNU C Library; if not, see . */ -#include -#include -#include -#include - - -int -raise (int sig) -{ - /* raise is an async-safe function. It could be called while the - fork function temporarily invalidated the PID field. Adjust for - that. */ - pid_t pid = THREAD_GETMEM (THREAD_SELF, pid); - if (__glibc_unlikely (pid < 0)) - pid = -pid; - - return INLINE_SYSCALL (tgkill, 3, pid, THREAD_GETMEM (THREAD_SELF, tid), - sig); -} +#include diff --git a/sysdeps/unix/sysv/linux/raise.c b/sysdeps/unix/sysv/linux/raise.c index 3795e6e..d386426 100644 --- a/sysdeps/unix/sysv/linux/raise.c +++ b/sysdeps/unix/sysv/linux/raise.c @@ -16,42 +16,34 @@ License along with the GNU C Library; if not, see . */ -#include -#include #include #include -#include - +#include +#include +#include +#include int raise (int sig) { - struct pthread *pd = THREAD_SELF; - pid_t pid = THREAD_GETMEM (pd, pid); - pid_t selftid = THREAD_GETMEM (pd, tid); - if (selftid == 0) - { - /* This system call is not supposed to fail. */ -#ifdef INTERNAL_SYSCALL - INTERNAL_SYSCALL_DECL (err); - selftid = INTERNAL_SYSCALL (gettid, err, 0); -#else - selftid = INLINE_SYSCALL (gettid, 0); -#endif - THREAD_SETMEM (pd, tid, selftid); - - /* We do not set the PID field in the TID here since we might be - called from a signal handler while the thread executes fork. */ - pid = selftid; - } - else - /* raise is an async-safe function. It could be called while the - fork/vfork function temporarily invalidated the PID field. Adjust for - that. */ - if (__glibc_unlikely (pid <= 0)) - pid = (pid & INT_MAX) == 0 ? selftid : -pid; - - return INLINE_SYSCALL (tgkill, 3, pid, selftid, sig); + /* raise is an async-safe function so it could be called while the + fork/vfork function temporarily invalidated the PID field. To avoid + relying in the cached value we block all user-defined signal handler + (which might call fork/vfork) and issues the getpid and gettid + directly. */ + + sigset_t set; + __libc_signal_block_app (&set); + + INTERNAL_SYSCALL_DECL (err); + pid_t pid = INTERNAL_SYSCALL (getpid, err, 0); + pid_t tid = INTERNAL_SYSCALL (gettid, err, 0); + + int ret = INLINE_SYSCALL (tgkill, 3, pid, tid, sig); + + __libc_signal_restore_set (&set); + + return ret; } libc_hidden_def (raise) weak_alias (raise, gsignal)