Message ID | 20230731171900.4065501-3-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | Make abort AS-safe | expand |
* Adhemerval Zanella via Libc-alpha: > diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h > index e2e9f9fd49..1c0f7b2e6c 100644 > --- a/sysdeps/generic/internal-signals.h > +++ b/sysdeps/generic/internal-signals.h > @@ -42,7 +42,31 @@ 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 */ This should probably go into a separate patch. I recall writing a similar patch, can't find it right now. Thanks, Florian
* Adhemerval Zanella via Libc-alpha: > +void > +__abort_lock_lock (void) > +{ > + __libc_lock_lock (lock); > +} I think __abort_lock_lock and __abort_lock_unlock should take a signal mask argument, so that they are more like a async-signal-safe mutex. > +/* Cause an abnormal program termination with core-dump. */ > +_Noreturn void > +abort (void) > +{ > + raise (SIGABRT); > + > + /* There is a SIGABRT handler 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 disposition, reinstall the handle to abort the > + process, and raise the signal again. */ > + internal_signal_block_all (NULL); > + __libc_lock_lock (lock); This could call __abort_lock_lock. Thanks, Florian
On Mon, Jul 31, 2023 at 1:19 PM Adhemerval Zanella via Libc-alpha < libc-alpha@sourceware.org> wrote: > > ABORT_INSTRUCTION after _exit (if the syscall does not terminate the > process, the system is really broken). > Yes, at least the linux kernel ultimately BUGs out if it cannot exit. the whole system crashes.
On 01/08/23 05:10, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h >> index e2e9f9fd49..1c0f7b2e6c 100644 >> --- a/sysdeps/generic/internal-signals.h >> +++ b/sysdeps/generic/internal-signals.h >> @@ -42,7 +42,31 @@ 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 */ > > This should probably go into a separate patch. I recall writing a > similar patch, can't find it right now. These function are not really used and only required for Hurd for the abort change. I think it is more logical to add them when they are actually used instead of a separated patch.
On 01/08/23 05:26, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> +void >> +__abort_lock_lock (void) >> +{ >> + __libc_lock_lock (lock); >> +} > > I think __abort_lock_lock and __abort_lock_unlock should take a signal > mask argument, so that they are more like a async-signal-safe mutex. That was my first approach, but I changed it due __spawnix usage. The lock is really only required on clone call, so we can release it after clone returns. However we need to only umask signals after the execve call in helper process. We can move the abort unlock later, it should be ok as well (it would add a slight more latency on highly multithead programs that spawns a lot of thread and try to abort(), but it should be ok). > >> +/* Cause an abnormal program termination with core-dump. */ >> +_Noreturn void >> +abort (void) >> +{ >> + raise (SIGABRT); >> + >> + /* There is a SIGABRT handler 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 disposition, reinstall the handle to abort the >> + process, and raise the signal again. */ >> + internal_signal_block_all (NULL); >> + __libc_lock_lock (lock); > > This could call __abort_lock_lock. Ack.
* Adhemerval Zanella via Libc-alpha: > + /* There is a SIGABRT handler 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 disposition, reinstall the handle to abort the typo: handle[r] Maybe mention here that the handler cannot change again because sigaction blocks on the lock? I also don't quite understand why we need to take the abort lock in posix_spawn. There isn't any user code that can run in the same address space after the vfork. Thanks, Florian
* Adhemerval Zanella via Libc-alpha: > diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c > index 44e45a4e23..e3364fb5d1 100644 > --- a/nptl/pthread_kill.c > +++ b/nptl/pthread_kill.c > @@ -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) > +{ > + struct pthread *pd = THREAD_SELF; > + int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), pd->tid, signo); > + return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0; > +} This needs to use gettid (the system call) so that it works after vfork, which may have an incorrect pd->tid. There should be a comment to this effect in pthread_kill implementation already. Thanks, Florian
On 02/08/23 04:57, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> + /* There is a SIGABRT handler 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 disposition, reinstall the handle to abort the > > typo: handle[r] > > Maybe mention here that the handler cannot change again because > sigaction blocks on the lock? Ack. > > I also don't quite understand why we need to take the abort lock in > posix_spawn. There isn't any user code that can run in the same address > space after the vfork. My understanding is 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.
On 02/08/23 09:38, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c >> index 44e45a4e23..e3364fb5d1 100644 >> --- a/nptl/pthread_kill.c >> +++ b/nptl/pthread_kill.c >> @@ -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) >> +{ >> + struct pthread *pd = THREAD_SELF; >> + int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), pd->tid, signo); >> + return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0; >> +} > > This needs to use gettid (the system call) so that it works after vfork, > which may have an incorrect pd->tid. > > There should be a comment to this effect in pthread_kill implementation > already. > Ack.
* Adhemerval Zanella Netto: >> I also don't quite understand why we need to take the abort lock in >> posix_spawn. There isn't any user code that can run in the same address >> space after the vfork. > > My understanding is 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. Right, I missed that. Maybe add a comment? Thanks, Florian
On 02/08/23 11:44, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >>> I also don't quite understand why we need to take the abort lock in >>> posix_spawn. There isn't any user code that can run in the same address >>> space after the vfork. >> >> My understanding is 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. > > Right, I missed that. Maybe add a comment? Ack.
diff --git a/include/stdlib.h b/include/stdlib.h index 7deb8193d7..b87a37e7e2 100644 --- a/include/stdlib.h +++ b/include/stdlib.h @@ -75,6 +75,10 @@ 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 (void) attribute_hidden; +extern void __abort_lock_unlock (void) attribute_hidden; + libc_hidden_proto (exit) libc_hidden_proto (abort) libc_hidden_proto (getenv) diff --git a/manual/startup.texi b/manual/startup.texi index 9bf24123f5..236a480a74 100644 --- a/manual/startup.texi +++ b/manual/startup.texi @@ -993,9 +993,6 @@ 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. The @code{abort} function causes abnormal program termination. This does not execute cleanup functions registered with @code{atexit} or @code{on_exit}. diff --git a/nptl/pthread_kill.c b/nptl/pthread_kill.c index 44e45a4e23..e3364fb5d1 100644 --- a/nptl/pthread_kill.c +++ b/nptl/pthread_kill.c @@ -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) +{ + struct pthread *pd = THREAD_SELF; + int ret = INTERNAL_SYSCALL_CALL (tgkill, __getpid (), pd->tid, signo); + return INTERNAL_SYSCALL_ERROR_P (ret) ? INTERNAL_SYSCALL_ERRNO (ret) : 0; +} + int __pthread_kill_internal (pthread_t threadid, int signo) { diff --git a/posix/fork.c b/posix/fork.c index b4aaa9fa6d..1640d0750c 100644 --- a/posix/fork.c +++ b/posix/fork.c @@ -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); diff --git a/signal/sigaction.c b/signal/sigaction.c index 3a49597c61..0d1358d720 100644 --- a/signal/sigaction.c +++ b/signal/sigaction.c @@ -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,23 @@ __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) + { + internal_signal_block_all (&set); + __abort_lock_lock (); + } + + int r = __libc_sigaction (sig, act, oact); + + if (sig == SIGABRT) + { + __abort_lock_unlock (); + internal_signal_restore_set (&set); + } + + return r; } libc_hidden_def (__sigaction) weak_alias (__sigaction, sigaction) diff --git a/stdlib/abort.c b/stdlib/abort.c index 16a453459c..e19ad730cd 100644 --- a/stdlib/abort.c +++ b/stdlib/abort.c @@ -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,53 @@ 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); -/* We should be prepared for multiple threads trying to run abort. */ -__libc_lock_define_initialized_recursive (static, lock); - - -/* Cause an abnormal program termination with core-dump. */ void -abort (void) +__abort_fork_reset_child (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); - } + __libc_lock_init (lock); +} - /* Now try to abort using the system specific command. */ - if (stage == 4) - { - ++stage; - ABORT_INSTRUCTION; - } +void +__abort_lock_lock (void) +{ + __libc_lock_lock (lock); +} - /* If we can't signal ourselves and the abort instruction failed, exit. */ - if (stage == 5) - { - ++stage; - _exit (127); - } +void +__abort_lock_unlock (void) +{ + __libc_lock_unlock (lock); +} - /* 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; +/* Cause an abnormal program termination with core-dump. */ +_Noreturn void +abort (void) +{ + raise (SIGABRT); + + /* There is a SIGABRT handler 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 disposition, reinstall the handle to abort the + process, and raise the signal again. */ + internal_signal_block_all (NULL); + __libc_lock_lock (lock); + + 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) diff --git a/sysdeps/generic/internal-signals.h b/sysdeps/generic/internal-signals.h index e2e9f9fd49..1c0f7b2e6c 100644 --- a/sysdeps/generic/internal-signals.h +++ b/sysdeps/generic/internal-signals.h @@ -42,7 +42,31 @@ 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 */ diff --git a/sysdeps/htl/pthreadP.h b/sysdeps/htl/pthreadP.h index 3f052f0e53..7410261d1a 100644 --- a/sysdeps/htl/pthreadP.h +++ b/sysdeps/htl/pthreadP.h @@ -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) diff --git a/sysdeps/nptl/_Fork.c b/sysdeps/nptl/_Fork.c index f8322ae557..f6da952ce0 100644 --- a/sysdeps/nptl/_Fork.c +++ b/sysdeps/nptl/_Fork.c @@ -17,11 +17,19 @@ <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; + + internal_signal_block_all (&set); + __abort_lock_lock (); + pid_t pid = arch_fork (&THREAD_SELF->tid); if (pid == 0) { @@ -44,6 +52,10 @@ _Fork (void) INTERNAL_SYSCALL_CALL (set_robust_list, &self->robust_head, sizeof (struct robust_list_head)); } + + __abort_lock_unlock (); + internal_signal_restore_set (&set); + return pid; } libc_hidden_def (_Fork) diff --git a/sysdeps/nptl/pthreadP.h b/sysdeps/nptl/pthreadP.h index 54f9198681..6cc675c5a5 100644 --- a/sysdeps/nptl/pthreadP.h +++ b/sysdeps/nptl/pthreadP.h @@ -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); diff --git a/sysdeps/unix/sysv/linux/internal-signals.h b/sysdeps/unix/sysv/linux/internal-signals.h index 43c3c0b4a0..c58f568dda 100644 --- a/sysdeps/unix/sysv/linux/internal-signals.h +++ b/sysdeps/unix/sysv/linux/internal-signals.h @@ -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. */ diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index ec687cb423..3ef07ec633 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -371,6 +371,7 @@ __spawnix (pid_t * pid, const char *file, args.xflags = xflags; internal_signal_block_all (&args.oldmask); + __abort_lock_lock (); /* 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 @@ -402,6 +403,8 @@ __spawnix (pid_t * pid, const char *file, &args); } + __abort_lock_unlock (); + /* It needs to collect the case where the auxiliary process was created but failed to execute the file (due either any preparation step or for execve itself). */