Refactor Linux raise implementation (BZ#15368)
Commit Message
This patch changes both the nptl and libc Linux raise implementation
to avoid the issues described in BZ#15368. The strategy used is
summarized in bug report first comment:
1. Block all signals (including internal NPTL ones);
2. Get pid and tid directly from syscall (not relying on cached
values);
3. Call tgkill;
4. Restore old signal mask.
Tested on x86_64 and i686.
* 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.
---
ChangeLog | 12 +++++++++
sysdeps/unix/sysv/linux/nptl-signals.h | 37 ++++++++++++++++++++++++++++
sysdeps/unix/sysv/linux/pt-raise.c | 23 +++--------------
sysdeps/unix/sysv/linux/raise.c | 45 ++++++++++++----------------------
4 files changed, 67 insertions(+), 50 deletions(-)
Comments
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.
> 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.
> +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.
> +static inline int
> +__libc_signal_block_app (const sigset_t *set)
Same type-safety error here.
> + 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.
zw
On 2016-06-17 15:43, Adhemerval Zanella wrote:
> This patch changes both the nptl and libc Linux raise implementation
> to avoid the issues described in BZ#15368. The strategy used is
> summarized in bug report first comment:
>
> 1. Block all signals (including internal NPTL ones);
> 2. Get pid and tid directly from syscall (not relying on cached
> values);
> 3. Call tgkill;
> 4. Restore old signal mask.
This new implementation introduces a behaviour change when a process is
run under ptrace:
1) The process call raise(SIGSTOP)
2) The parent process run ptrace (PTRACE_CONT, pid, NULL, SOME_SIGNAL)
3) The process runs some code generating a ptrace event
With the old implementation, the ptrace event captured after the process
is restarted is the one from 3). With the new implementation, given the
signals are blocked, they are only delivered when raise unblocks them.
This generates an additional ptrace event for the delivery of
SOME_SIGNAL before 3).
For reference this breaks the libnih testsuite. I believe that it is a
corner case and that the testsuite has too precise expectations. Still
I think it is worth mentioning the behavior change here in case someone
ends up debugging the same issue.
Aurelien
@@ -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 (const 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 (const 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;
@@ -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>
@@ -16,42 +16,27 @@
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);
+ sigset_t set;
+ __libc_signal_block_app (&set);
+
+ pid_t pid = __getpid ();
+ pid_t tid = INLINE_SYSCALL (gettid, 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)