This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] Refactor Linux raise implementation (BZ#15368)


Ping x2 (I would like to push it before hard freeze).

On 29/06/2016 09:57, Adhemerval Zanella wrote:
> Ping.
> 
> On 18/06/2016 14:25, Adhemerval Zanella wrote:
>> 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
>>> <adhemerval.zanella@linaro.org> 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 <drepper@redhat.com>, 2002.
>>  
>> @@ -16,22 +17,4 @@
>>     License along with the GNU C Library; if not, see
>>     <http://www.gnu.org/licenses/>.  */
>>  
>> -#include <errno.h>
>> -#include <signal.h>
>> -#include <sysdep.h>
>> -#include <tls.h>
>> -
>> -
>> -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 <sysdeps/unix/sysv/linux/raise.c>
>> 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
>>     <http://www.gnu.org/licenses/>.  */
>>  
>> -#include <errno.h>
>> -#include <limits.h>
>>  #include <signal.h>
>>  #include <sysdep.h>
>> -#include <nptl/pthreadP.h>
>> -
>> +#include <errno.h>
>> +#include <sys/types.h>
>> +#include <unistd.h>
>> +#include <nptl-signals.h>
>>  
>>  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)
>>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]