@@ -27,7 +27,9 @@ Major new features:
Deprecated and removed features, and other changes affecting compatibility:
- [Add deprecations, removals and changes affecting compatibility here]
+* The async-signal-safe abort implementation makes longjmp from the SIGABRT
+ handler always abort if set up with setjmp. Use sigsetjmp to trigger old
+ behavior, where the handler does not stop process execution.
Changes to build and runtime requirements:
@@ -59,7 +59,7 @@ static int test_main (void);
#include <support/support.h>
volatile int chk_fail_ok;
-jmp_buf chk_fail_buf;
+sigjmp_buf chk_fail_buf;
static void
handler (int sig)
@@ -86,7 +86,7 @@ do_one_test (impl_t *impl, char *dst, const char *src,
return;
chk_fail_ok = 1;
- if (setjmp (chk_fail_buf) == 0)
+ if (sigsetjmp (chk_fail_buf, 1) == 0)
{
res = CALL (impl, dst, src, dlen);
printf ("*** Function %s (%zd; %zd) did not __chk_fail\n",
@@ -214,7 +214,7 @@ do_random_tests (void)
if (impl->test != 1)
{
chk_fail_ok = 1;
- if (setjmp (chk_fail_buf) == 0)
+ if (sigsetjmp (chk_fail_buf, 1) == 0)
{
res = (unsigned char *)
CALL (impl, (char *) p2 + align2,
@@ -26,7 +26,7 @@
static volatile int chk_fail_ok;
static volatile int ret;
-static jmp_buf chk_fail_buf;
+static sigjmp_buf chk_fail_buf;
static void
handler (int sig)
@@ -49,7 +49,7 @@ static wchar_t wbuf2[20] = L"%ls";
do { wprintf (L"Failure on line %d\n", __LINE__); ret = 1; } while (0)
#define CHK_FAIL_START \
chk_fail_ok = 1; \
- if (! setjmp (chk_fail_buf)) \
+ if (! sigsetjmp (chk_fail_buf, 1)) \
{
#define CHK_FAIL_END \
chk_fail_ok = 0; \
@@ -90,7 +90,7 @@ do_prepare (int argc, char *argv[])
static volatile int chk_fail_ok;
static volatile int ret;
-static jmp_buf chk_fail_buf;
+static sigjmp_buf chk_fail_buf;
static void
handler (int sig)
@@ -133,7 +133,7 @@ static int num2 = 987654;
do { printf ("Failure on line %d\n", __LINE__); ret = 1; } while (0)
#define CHK_FAIL_START \
chk_fail_ok = 1; \
- if (! setjmp (chk_fail_buf)) \
+ if (! sigsetjmp (chk_fail_buf, 1)) \
{
#define CHK_FAIL_END \
chk_fail_ok = 0; \
@@ -3,4 +3,7 @@
#ifndef _ISOMAC
extern int __close_range (unsigned int lowfd, unsigned int highfd, int flags);
libc_hidden_proto (__close_range);
+
+extern pid_t __gettid (void);
+libc_hidden_proto (__gettid);
#endif
@@ -20,6 +20,7 @@
# include <sys/stat.h>
# include <rtld-malloc.h>
+# include <internal-sigset.h>
extern __typeof (strtol_l) __strtol_l;
extern __typeof (strtoul_l) __strtoul_l;
@@ -77,6 +78,11 @@ libc_hidden_proto (__isoc23_strtoull_l)
# define strtoull_l __isoc23_strtoull_l
#endif
+extern void __abort_fork_reset_child (void) attribute_hidden;
+extern void __abort_lock_lock (internal_sigset_t *set) attribute_hidden;
+extern void __abort_lock_unlock (const internal_sigset_t *set)
+ attribute_hidden;
+
libc_hidden_proto (exit)
libc_hidden_proto (abort)
libc_hidden_proto (getenv)
@@ -1010,10 +1010,7 @@ for this function is in @file{stdlib.h}.
@deftypefun void abort (void)
@standards{ISO, stdlib.h}
-@safety{@prelim{}@mtsafe{}@asunsafe{@asucorrupt{}}@acunsafe{@aculock{} @acucorrupt{}}}
-@c The implementation takes a recursive lock and attempts to support
-@c calls from signal handlers, but if we're in the middle of flushing or
-@c using streams, we may encounter them in inconsistent states.
+@safety{@prelim{}@mtsafe{}@assafe{}@acsafe{}}
The @code{abort} function causes abnormal program termination. This
does not execute cleanup functions registered with @code{atexit} or
@code{on_exit}.
@@ -1021,6 +1018,10 @@ does not execute cleanup functions registered with @code{atexit} or
This function actually terminates the process by raising a
@code{SIGABRT} signal, and your program can include a handler to
intercept this signal; see @ref{Signal Handling}.
+
+If either the signal handler does not terminate the process, or if the
+signal is blocked, @code{abort} will reset the signal disposition to the
+default @code{SIG_DFL} action and raise the signal again.
@end deftypefun
@node Termination Internals
@@ -69,6 +69,17 @@ __pthread_kill_implementation (pthread_t threadid, int signo, int no_tid)
return ret;
}
+/* Send the signal SIGNO to the caller. Used by abort and called where the
+ signals are being already blocked and there is no need to synchronize with
+ exit_lock. */
+int
+__pthread_raise_internal (int signo)
+{
+ /* Use the gettid syscall so it works after vfork. */
+ int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), __gettid(), signo);
+ return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0;
+}
+
int
__pthread_kill_internal (pthread_t threadid, int signo)
{
@@ -84,6 +84,8 @@ __libc_fork (void)
fork_system_setup_after_fork ();
+ call_function_static_weak (__abort_fork_reset_child);
+
/* Release malloc locks. */
call_function_static_weak (__malloc_fork_unlock_child);
@@ -16,8 +16,9 @@
<https://www.gnu.org/licenses/>. */
#include <errno.h>
-#include <signal.h>
#include <internal-signals.h>
+#include <libc-lock.h>
+#include <signal.h>
/* If ACT is not NULL, change the action for SIG to *ACT.
If OACT is not NULL, put the old action for SIG in *OACT. */
@@ -30,7 +31,17 @@ __sigaction (int sig, const struct sigaction *act, struct sigaction *oact)
return -1;
}
- return __libc_sigaction (sig, act, oact);
+ internal_sigset_t set;
+
+ if (sig == SIGABRT)
+ __abort_lock_lock (&set);
+
+ int r = __libc_sigaction (sig, act, oact);
+
+ if (sig == SIGABRT)
+ __abort_lock_unlock (&set);
+
+ return r;
}
libc_hidden_def (__sigaction)
weak_alias (__sigaction, sigaction)
@@ -15,13 +15,11 @@
License along with the GNU C Library; if not, see
<https://www.gnu.org/licenses/>. */
-#include <libc-lock.h>
#include <signal.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
#include <internal-signals.h>
+#include <libc-lock.h>
+#include <pthreadP.h>
+#include <unistd.h>
/* Try to get a machine dependent instruction which will make the
program crash. This is used in case everything else fails. */
@@ -35,89 +33,56 @@
struct abort_msg_s *__abort_msg;
libc_hidden_def (__abort_msg)
-/* We must avoid to run in circles. Therefore we remember how far we
- already got. */
-static int stage;
+/* The lock is used to prevent multiple thread to change the SIGABRT
+ to SIG_IGN while abort tries to change to SIG_DFL, and to avoid
+ a new process to see a wrong disposition if there is a SIGABRT
+ handler installed. */
+__libc_lock_define_initialized (static, lock);
+
+void
+__abort_fork_reset_child (void)
+{
+ __libc_lock_init (lock);
+}
-/* We should be prepared for multiple threads trying to run abort. */
-__libc_lock_define_initialized_recursive (static, lock);
+void
+__abort_lock_lock (internal_sigset_t *set)
+{
+ internal_signal_block_all (set);
+ __libc_lock_lock (lock);
+}
+void
+__abort_lock_unlock (const internal_sigset_t *set)
+{
+ __libc_lock_unlock (lock);
+ internal_signal_restore_set (set);
+}
/* Cause an abnormal program termination with core-dump. */
-void
+_Noreturn void
abort (void)
{
- struct sigaction act;
-
- /* First acquire the lock. */
- __libc_lock_lock_recursive (lock);
-
- /* Now it's for sure we are alone. But recursive calls are possible. */
-
- /* Unblock SIGABRT. */
- if (stage == 0)
- {
- ++stage;
- internal_sigset_t sigs;
- internal_sigemptyset (&sigs);
- internal_sigaddset (&sigs, SIGABRT);
- internal_sigprocmask (SIG_UNBLOCK, &sigs, NULL);
- }
-
- /* Send signal which possibly calls a user handler. */
- if (stage == 1)
- {
- /* This stage is special: we must allow repeated calls of
- `abort' when a user defined handler for SIGABRT is installed.
- This is risky since the `raise' implementation might also
- fail but I don't see another possibility. */
- int save_stage = stage;
-
- stage = 0;
- __libc_lock_unlock_recursive (lock);
-
- raise (SIGABRT);
-
- __libc_lock_lock_recursive (lock);
- stage = save_stage + 1;
- }
-
- /* There was a handler installed. Now remove it. */
- if (stage == 2)
- {
- ++stage;
- memset (&act, '\0', sizeof (struct sigaction));
- act.sa_handler = SIG_DFL;
- __sigfillset (&act.sa_mask);
- act.sa_flags = 0;
- __sigaction (SIGABRT, &act, NULL);
- }
-
- /* Try again. */
- if (stage == 3)
- {
- ++stage;
- raise (SIGABRT);
- }
-
- /* Now try to abort using the system specific command. */
- if (stage == 4)
- {
- ++stage;
- ABORT_INSTRUCTION;
- }
-
- /* If we can't signal ourselves and the abort instruction failed, exit. */
- if (stage == 5)
- {
- ++stage;
- _exit (127);
- }
-
- /* If even this fails try to use the provided instruction to crash
- or otherwise make sure we never return. */
- while (1)
- /* Try for ever and ever. */
- ABORT_INSTRUCTION;
+ raise (SIGABRT);
+
+ /* There is a SIGABRT handle installed and it returned, or SIGABRT was
+ blocked or ignored. In this case use a AS-safe lock to prevent sigaction
+ to change the signal dispositioni (it will block on __abort_lock),
+ reinstall the handle to abort the process, and re-raise the signal.
+
+ The handle can not change again because sigaction blocks on the lock. */
+ __abort_lock_lock (NULL);
+
+ struct sigaction act = {.sa_handler = SIG_DFL, .sa_flags = 0 };
+ __sigfillset (&act.sa_mask);
+ __libc_sigaction (SIGABRT, &act, NULL);
+ __pthread_raise_internal (SIGABRT);
+ internal_signal_unblock_signal (SIGABRT);
+
+ /* This code should be unreachable, try the arch-specific code and the
+ syscall fallback. */
+ ABORT_INSTRUCTION;
+
+ _exit (127);
}
libc_hidden_def (abort)
@@ -20,6 +20,7 @@
# define __INTERNAL_SIGNALS_H
#include <signal.h>
+#include <internal-sigset.h>
#include <sigsetops.h>
#include <stdbool.h>
#include <stddef.h>
@@ -39,10 +40,32 @@ clear_internal_signals (sigset_t *set)
{
}
-typedef sigset_t internal_sigset_t;
-
#define internal_sigemptyset(__s) __sigemptyset (__s)
+#define internal_sigfillset(__s) __sigfillset (__s)
#define internal_sigaddset(__s, __i) __sigaddset (__s, __i)
#define internal_sigprocmask(__h, __s, __o) __sigprocmask (__h, __s, __o)
+static inline void
+internal_signal_block_all (internal_sigset_t *oset)
+{
+ internal_sigset_t set;
+ internal_sigfillset (&set);
+ internal_sigprocmask (SIG_BLOCK, &set, oset);
+}
+
+static inline void
+internal_signal_restore_set (const internal_sigset_t *set)
+{
+ internal_sigprocmask (SIG_SETMASK, set, NULL);
+}
+
+static inline void
+internal_signal_unblock_signal (int sig)
+{
+ internal_sigset_t set;
+ internal_sigemptyset (&set);
+ internal_sigaddset (&set, sig);
+ internal_sigprocmask (SIG_UNBLOCK, &set, NULL);
+}
+
#endif /* __INTERNAL_SIGNALS_H */
new file mode 100644
@@ -0,0 +1,26 @@
+/* Internal sigset_t definition.
+ Copyright (C) 2022-2023 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
+ <https://www.gnu.org/licenses/>. */
+
+#ifndef _INTERNAL_SIGSET_H
+#define _INTERNAL_SIGSET_H
+
+#include <signal.h>
+
+typedef sigset_t internal_sigset_t;
+
+#endif
@@ -92,6 +92,8 @@ int __pthread_attr_setstack (pthread_attr_t *__attr, void *__stackaddr,
int __pthread_attr_getstack (const pthread_attr_t *, void **, size_t *);
void __pthread_testcancel (void);
+#define __pthread_raise_internal(__sig) raise (__sig)
+
libc_hidden_proto (__pthread_self)
#if IS_IN (libpthread)
@@ -17,11 +17,17 @@
<https://www.gnu.org/licenses/>. */
#include <arch-fork.h>
+#include <libc-lock.h>
#include <pthreadP.h>
pid_t
_Fork (void)
{
+ /* The lock acquisition needs to be AS-safe to avoid deadlock if _Fork is
+ called from the signal handler that has interrupted fork itself. */
+ internal_sigset_t set;
+ __abort_lock_lock (&set);
+
pid_t pid = arch_fork (&THREAD_SELF->tid);
if (pid == 0)
{
@@ -44,6 +50,9 @@ _Fork (void)
INTERNAL_SYSCALL_CALL (set_robust_list, &self->robust_head,
sizeof (struct robust_list_head));
}
+
+ __abort_lock_unlock (&set);
+
return pid;
}
libc_hidden_def (_Fork)
@@ -508,6 +508,7 @@ libc_hidden_proto (__pthread_kill)
extern int __pthread_cancel (pthread_t th);
extern int __pthread_kill_internal (pthread_t threadid, int signo)
attribute_hidden;
+extern int __pthread_raise_internal (int signo) attribute_hidden;
extern void __pthread_exit (void *value) __attribute__ ((__noreturn__));
libc_hidden_proto (__pthread_exit)
extern int __pthread_join (pthread_t threadid, void **thread_return);
@@ -90,6 +90,15 @@ internal_signal_restore_set (const internal_sigset_t *set)
__NSIG_BYTES);
}
+static inline void
+internal_signal_unblock_signal (int sig)
+{
+ internal_sigset_t set;
+ internal_sigemptyset (&set);
+ internal_sigaddset (&set, sig);
+ INTERNAL_SYSCALL_CALL (rt_sigprocmask, SIG_UNBLOCK, &set, NULL,
+ __NSIG_BYTES);
+}
/* It is used on timer_create code directly on sigwaitinfo call, so it can not
use the internal_sigset_t definitions. */
@@ -21,7 +21,7 @@
#include <sigsetops.h>
-typedef struct
+typedef struct _internal_sigset_t
{
unsigned long int __val[__NSIG_WORDS];
} internal_sigset_t;
@@ -383,7 +383,11 @@ __spawnix (int *pid, const char *file,
args.pidfd = 0;
args.xflags = xflags;
- internal_signal_block_all (&args.oldmask);
+ /* Avoid the potential issues is if caller sets a SIG_IGN for SIGABRT, calls
+ abort, and another thread issues posix_spawn just after the sigaction
+ returns. With default options (not setting POSIX_SPAWN_SETSIGDEF), the
+ process can still see SIG_DFL for SIGABRT, where it should be SIG_IGN. */
+ __abort_lock_lock (&args.oldmask);
/* The clone flags used will create a new child that will run in the same
memory space (CLONE_VM) and the execution of calling thread will be
@@ -474,7 +478,7 @@ __spawnix (int *pid, const char *file,
if ((ec == 0) && (pid != NULL))
*pid = use_pidfd ? args.pidfd : new_pid;
- internal_signal_restore_set (&args.oldmask);
+ __abort_lock_unlock (&args.oldmask);
__pthread_setcancelstate (state, NULL);
The recursive lock used on abort does not synchronize with a new process creation (either by fork-like interfaces or posix_spawn ones), nor it is reinitialized after fork(). Also, the SIGABRT unblock before raise shows another race condition, where a fork or posix_spawn call by another thread just after the recursive lock release and before the SIGABRT raise might create programs with a non-expected signal mask. With the default option (without POSIX_SPAWN_SETSIGDEF), the process can see SIG_DFL for SIGABRT, where it should be SIG_IGN. To fix the AS-safe, the raise() is issued without changing the process signal mask, and an AS-safe lock is used if a SIGABRT is installed or the process is blocked or ignored. With the signal mask change removal, there is no need to use a recursive lock. The lock is also on both _Fork and posix_spawn, to avoid the spawn process to see the abort handler as SIG_DFL. The fallback is also simplified, there is no need to use a loop of ABORT_INSTRUCTION after _exit (if the syscall does not terminate the process, the system is broken). The proposed fix changes how setjmp works on a SIGABRT handler, where glibc does not save the signal mask. So usage like the below will now always abort. static volatile int chk_fail_ok; static jmp_buf chk_fail_buf; static void handler (int sig) { if (chk_fail_ok) { chk_fail_ok = 0; longjmp (chk_fail_buf, 1); } else _exit (127); } [...] signal (SIGABRT, handler); [....] chk_fail_ok = 1; if (! setjmp (chk_fail_buf)) { // Something that can calls abort, like a failed fortify function. chk_fail_ok = 0; printf ("FAIL\n"); } Such cases will need to use sigsetjmp instead. Checked on x86_64-linux-gnu and aarch64-linux-gnu. -- Changes from v2: - Dropped the setjmp to follow BSD semantic and adjusted the tests to use sigsetjmp. - Improve the abort documentation on why the lock is required as a QoI, and why it is still async-signal-safe. Changes from v1: - Rename de signal block and lock to __abort_lock_lock. - Improve comments on both abort, where the signal disposition can not be changed, and on posix_spawn on why it needs to take the abort lock. - Use gettid() on __pthread_raise_internal. - Added a NEWS entry for the setjmp fix. --- NEWS | 4 +- debug/test-strcpy_chk.c | 6 +- debug/tst-fortify-wide.c | 4 +- debug/tst-fortify.c | 4 +- include/bits/unistd_ext.h | 3 + include/stdlib.h | 6 + manual/startup.texi | 9 +- nptl/pthread_kill.c | 11 ++ posix/fork.c | 2 + signal/sigaction.c | 15 ++- stdlib/abort.c | 131 ++++++++------------- sysdeps/generic/internal-signals.h | 27 ++++- sysdeps/generic/internal-sigset.h | 26 ++++ sysdeps/htl/pthreadP.h | 2 + sysdeps/nptl/_Fork.c | 9 ++ sysdeps/nptl/pthreadP.h | 1 + sysdeps/unix/sysv/linux/internal-signals.h | 9 ++ sysdeps/unix/sysv/linux/internal-sigset.h | 2 +- sysdeps/unix/sysv/linux/spawni.c | 8 +- 19 files changed, 177 insertions(+), 102 deletions(-) create mode 100644 sysdeps/generic/internal-sigset.h