* [PATCH v2 0/2] Make abort AS-safe @ 2023-08-03 17:34 Adhemerval Zanella 2023-08-03 17:34 ` [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp Adhemerval Zanella 2023-08-03 17:34 ` [PATCH v2 2/2] stdlib: Make abort AS-safe (BZ 26275) Adhemerval Zanella 0 siblings, 2 replies; 15+ messages in thread From: Adhemerval Zanella @ 2023-08-03 17:34 UTC (permalink / raw) To: libc-alpha, Carlos O'Donell, Florian Weimer Besides POSIX stating abort should be AS-safe, Rust also had an open PR about it [1] (it was closed with a different fix). The main issue is the recursive lock used on abort does not synchronize with 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 raising SIGABRT might create a new process with a non-expected signal mask. The requirement of SIGABRT handler being called even if the signals is blocked was changed by a POSIX defect [3] 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. The the signal mask change removal, there is no need to use a recursive lock. The lock is also used on both _Fork and posix_spawn, to avoid the spawn process to see the abort handler as SIG_DFL. The clone is also subjected to this issue, but since glibc does not do any internal metadata setup (as for fork-like function), this patch does not handle it for the symbol. I have not added a regression tests because, from previous Carlos's patch [2], hitting the code path to trigger the potential issue (fork just after abort has acquired the lock and reset SIGABRT handler) is not deterministic and it would generate a lot of development overhead. [1] https://github.com/rust-lang/rust/issues/73894#issuecomment-673478761 [2] https://sourceware.org/pipermail/libc-alpha/2020-September/117934.html [3] https://austingroupbugs.net/view.php?id=906#c5851 Changes from v1: - Change __abort_lock_lock/__abort_lock_unlock to use a internal_sigset_t. - Improve comments. - Use gettid syscall on __pthread_raise_internal to work after vfork. Adhemerval Zanella (2): setjmp: Use BSD sematic as default for setjmp stdlib: Make abort AS-safe (BZ 26275) include/bits/unistd_ext.h | 3 + include/stdlib.h | 6 + manual/setjmp.texi | 14 +-- manual/startup.texi | 3 - nptl/pthread_create.c | 3 +- nptl/pthread_kill.c | 11 ++ posix/fork.c | 2 + setjmp/setjmp.h | 5 - signal/sigaction.c | 15 ++- stdlib/abort.c | 130 ++++++++------------- sysdeps/generic/internal-signals.h | 27 ++++- sysdeps/generic/internal-sigset.h | 26 +++++ sysdeps/htl/pthreadP.h | 2 + sysdeps/nptl/_Fork.c | 9 ++ sysdeps/nptl/libc_start_call_main.h | 3 +- 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 | 6 +- 19 files changed, 167 insertions(+), 110 deletions(-) create mode 100644 sysdeps/generic/internal-sigset.h -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-03 17:34 [PATCH v2 0/2] Make abort AS-safe Adhemerval Zanella @ 2023-08-03 17:34 ` Adhemerval Zanella 2023-08-03 18:06 ` Joe Simmons-Talbott ` (2 more replies) 2023-08-03 17:34 ` [PATCH v2 2/2] stdlib: Make abort AS-safe (BZ 26275) Adhemerval Zanella 1 sibling, 3 replies; 15+ messages in thread From: Adhemerval Zanella @ 2023-08-03 17:34 UTC (permalink / raw) To: libc-alpha, Carlos O'Donell, Florian Weimer POSIX relaxed the relation of setjmp/longjmp and the signal mask save/restore, meaning that setjmp does not require to be routed to _setjmp to be standard compliant. This is done to avoid breakage of SIGABRT handlers, since to fully make abort AS-safe, it is required to remove the recurisve lock used to unblock SIGABRT prior raised the signal. Also, it allows caller to actually use setjmp, since from 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally routed to _setjmp. Checked on x86_64-linux-gnu. --- manual/setjmp.texi | 14 ++++---------- nptl/pthread_create.c | 3 ++- setjmp/setjmp.h | 5 ----- sysdeps/nptl/libc_start_call_main.h | 3 ++- 4 files changed, 8 insertions(+), 17 deletions(-) diff --git a/manual/setjmp.texi b/manual/setjmp.texi index 7092a0dde2..f2d82a2f33 100644 --- a/manual/setjmp.texi +++ b/manual/setjmp.texi @@ -189,16 +189,10 @@ them @code{volatile}. @section Non-Local Exits and Signals In BSD Unix systems, @code{setjmp} and @code{longjmp} also save and -restore the set of blocked signals; see @ref{Blocking Signals}. However, -the POSIX.1 standard requires @code{setjmp} and @code{longjmp} not to -change the set of blocked signals, and provides an additional pair of -functions (@code{sigsetjmp} and @code{siglongjmp}) to get the BSD -behavior. - -The behavior of @code{setjmp} and @code{longjmp} in @theglibc{} is -controlled by feature test macros; see @ref{Feature Test Macros}. The -default in @theglibc{} is the POSIX.1 behavior rather than the BSD -behavior. +restore the set of blocked signals; see @ref{Blocking Signals}, while +on @w{System V} they will not. POSIX does not specify the relation of +@code{setjmp} and @code{longjmp} to signal mask. The default in +@theglibc{} is the BSD behavior. The facilities in this section are declared in the header file @file{setjmp.h}. diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c index 1ac8862ed2..3d7dfac198 100644 --- a/nptl/pthread_create.c +++ b/nptl/pthread_create.c @@ -399,7 +399,8 @@ start_thread (void *arg) the saved signal mask), so that is a false positive. */ DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overflow="); #endif - not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf); + not_first_call = __sigsetjmp ( + (struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf, 0); DIAG_POP_NEEDS_COMMENT; /* No previous handlers. NB: This must be done after setjmp since the diff --git a/setjmp/setjmp.h b/setjmp/setjmp.h index 3cdc2dfcfb..53edbba92b 100644 --- a/setjmp/setjmp.h +++ b/setjmp/setjmp.h @@ -44,11 +44,6 @@ extern int __sigsetjmp (struct __jmp_buf_tag __env[1], int __savemask) __THROWNL Return 0. */ extern int _setjmp (struct __jmp_buf_tag __env[1]) __THROWNL; -/* Do not save the signal mask. This is equivalent to the `_setjmp' - BSD function. */ -#define setjmp(env) _setjmp (env) - - /* Jump to the environment saved in ENV, making the `setjmp' call there return VAL, or 1 if VAL is 0. */ extern void longjmp (struct __jmp_buf_tag __env[1], int __val) diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h index a55e3df013..984e859550 100644 --- a/sysdeps/nptl/libc_start_call_main.h +++ b/sysdeps/nptl/libc_start_call_main.h @@ -41,7 +41,8 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL), the saved signal mask), so that is a false positive. */ DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overflow="); #endif - not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf); + not_first_call = __sigsetjmp ( + (struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf, 0); DIAG_POP_NEEDS_COMMENT; if (__glibc_likely (! not_first_call)) { -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-03 17:34 ` [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp Adhemerval Zanella @ 2023-08-03 18:06 ` Joe Simmons-Talbott 2023-08-03 22:09 ` Joseph Myers 2023-08-04 8:43 ` Florian Weimer 2 siblings, 0 replies; 15+ messages in thread From: Joe Simmons-Talbott @ 2023-08-03 18:06 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: libc-alpha, Carlos O'Donell, Florian Weimer On Thu, Aug 03, 2023 at 02:34:35PM -0300, Adhemerval Zanella via Libc-alpha wrote: > POSIX relaxed the relation of setjmp/longjmp and the signal mask > save/restore, meaning that setjmp does not require to be routed to > _setjmp to be standard compliant. > > This is done to avoid breakage of SIGABRT handlers, since to fully > make abort AS-safe, it is required to remove the recurisve lock recursive > used to unblock SIGABRT prior raised the signal. > > Also, it allows caller to actually use setjmp, since from > 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally > routed to _setjmp. > > Checked on x86_64-linux-gnu. > --- > manual/setjmp.texi | 14 ++++---------- > nptl/pthread_create.c | 3 ++- > setjmp/setjmp.h | 5 ----- > sysdeps/nptl/libc_start_call_main.h | 3 ++- > 4 files changed, 8 insertions(+), 17 deletions(-) > > diff --git a/manual/setjmp.texi b/manual/setjmp.texi > index 7092a0dde2..f2d82a2f33 100644 > --- a/manual/setjmp.texi > +++ b/manual/setjmp.texi > @@ -189,16 +189,10 @@ them @code{volatile}. > @section Non-Local Exits and Signals > > In BSD Unix systems, @code{setjmp} and @code{longjmp} also save and > -restore the set of blocked signals; see @ref{Blocking Signals}. However, > -the POSIX.1 standard requires @code{setjmp} and @code{longjmp} not to > -change the set of blocked signals, and provides an additional pair of > -functions (@code{sigsetjmp} and @code{siglongjmp}) to get the BSD > -behavior. > - > -The behavior of @code{setjmp} and @code{longjmp} in @theglibc{} is > -controlled by feature test macros; see @ref{Feature Test Macros}. The > -default in @theglibc{} is the POSIX.1 behavior rather than the BSD > -behavior. > +restore the set of blocked signals; see @ref{Blocking Signals}, while > +on @w{System V} they will not. POSIX does not specify the relation of > +@code{setjmp} and @code{longjmp} to signal mask. The default in > +@theglibc{} is the BSD behavior. > > The facilities in this section are declared in the header file > @file{setjmp.h}. > diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c > index 1ac8862ed2..3d7dfac198 100644 > --- a/nptl/pthread_create.c > +++ b/nptl/pthread_create.c > @@ -399,7 +399,8 @@ start_thread (void *arg) > the saved signal mask), so that is a false positive. */ > DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overflow="); > #endif > - not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf); > + not_first_call = __sigsetjmp ( > + (struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf, 0); > DIAG_POP_NEEDS_COMMENT; > > /* No previous handlers. NB: This must be done after setjmp since the > diff --git a/setjmp/setjmp.h b/setjmp/setjmp.h > index 3cdc2dfcfb..53edbba92b 100644 > --- a/setjmp/setjmp.h > +++ b/setjmp/setjmp.h > @@ -44,11 +44,6 @@ extern int __sigsetjmp (struct __jmp_buf_tag __env[1], int __savemask) __THROWNL > Return 0. */ > extern int _setjmp (struct __jmp_buf_tag __env[1]) __THROWNL; > > -/* Do not save the signal mask. This is equivalent to the `_setjmp' > - BSD function. */ > -#define setjmp(env) _setjmp (env) > - > - > /* Jump to the environment saved in ENV, making the > `setjmp' call there return VAL, or 1 if VAL is 0. */ > extern void longjmp (struct __jmp_buf_tag __env[1], int __val) > diff --git a/sysdeps/nptl/libc_start_call_main.h b/sysdeps/nptl/libc_start_call_main.h > index a55e3df013..984e859550 100644 > --- a/sysdeps/nptl/libc_start_call_main.h > +++ b/sysdeps/nptl/libc_start_call_main.h > @@ -41,7 +41,8 @@ __libc_start_call_main (int (*main) (int, char **, char ** MAIN_AUXVEC_DECL), > the saved signal mask), so that is a false positive. */ > DIAG_IGNORE_NEEDS_COMMENT (11, "-Wstringop-overflow="); > #endif > - not_first_call = setjmp ((struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf); > + not_first_call = __sigsetjmp ( > + (struct __jmp_buf_tag *) unwind_buf.cancel_jmp_buf, 0); > DIAG_POP_NEEDS_COMMENT; > if (__glibc_likely (! not_first_call)) > { > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-03 17:34 ` [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp Adhemerval Zanella 2023-08-03 18:06 ` Joe Simmons-Talbott @ 2023-08-03 22:09 ` Joseph Myers 2023-08-04 8:43 ` Florian Weimer 2 siblings, 0 replies; 15+ messages in thread From: Joseph Myers @ 2023-08-03 22:09 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: libc-alpha, Carlos O'Donell, Florian Weimer If you're changing API-level semantics for a function (or macro), I'd definitely expect a NEWS entry (under "Deprecated and removed features, and other changes affecting compatibility") for the change. Some investigation of the extent to which setjmp users outside glibc might be affected by the change would also be a good idea. -- Joseph S. Myers joseph@codesourcery.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-03 17:34 ` [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp Adhemerval Zanella 2023-08-03 18:06 ` Joe Simmons-Talbott 2023-08-03 22:09 ` Joseph Myers @ 2023-08-04 8:43 ` Florian Weimer 2023-08-04 12:36 ` Adhemerval Zanella Netto 2 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2023-08-04 8:43 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: libc-alpha, Carlos O'Donell * Adhemerval Zanella: > POSIX relaxed the relation of setjmp/longjmp and the signal mask > save/restore, meaning that setjmp does not require to be routed to > _setjmp to be standard compliant. > > This is done to avoid breakage of SIGABRT handlers, since to fully > make abort AS-safe, it is required to remove the recurisve lock > used to unblock SIGABRT prior raised the signal. > > Also, it allows caller to actually use setjmp, since from > 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally > routed to _setjmp. I still think we shouldn't do this due to the performance implications. Thanks, Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-04 8:43 ` Florian Weimer @ 2023-08-04 12:36 ` Adhemerval Zanella Netto 2023-08-05 7:21 ` Paul Zimmermann 2023-08-07 12:54 ` Florian Weimer 0 siblings, 2 replies; 15+ messages in thread From: Adhemerval Zanella Netto @ 2023-08-04 12:36 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Carlos O'Donell On 04/08/23 05:43, Florian Weimer wrote: > * Adhemerval Zanella: > >> POSIX relaxed the relation of setjmp/longjmp and the signal mask >> save/restore, meaning that setjmp does not require to be routed to >> _setjmp to be standard compliant. >> >> This is done to avoid breakage of SIGABRT handlers, since to fully >> make abort AS-safe, it is required to remove the recurisve lock >> used to unblock SIGABRT prior raised the signal. >> >> Also, it allows caller to actually use setjmp, since from >> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally >> routed to _setjmp. > > I still think we shouldn't do this due to the performance implications. By not changing it and with the abort AS-safe fix, the following code will always abort the program: -- static jmp_buf jb; static void sigabrt_handler (int sig) { longjmp (jb, 1); } struct sigaction sa = { .sa_handler = sigabrt_handler, .sa_flags = 0 }; sigemptyset (&sa.sa_mask); assert (sigaction (SIGABRT, &sa, 0) == 0); if (setjmp (jb) == 0) abort (); if (setjmp (jb) == 0) abort (); // No reached. -- Callers will need to change to sigsetjmp (..., 1) to have the same semantic. That's the main reason I am suggesting this patch. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-04 12:36 ` Adhemerval Zanella Netto @ 2023-08-05 7:21 ` Paul Zimmermann 2023-08-07 12:54 ` Florian Weimer 1 sibling, 0 replies; 15+ messages in thread From: Paul Zimmermann @ 2023-08-05 7:21 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: fweimer, libc-alpha, carlos I see several threads currently with typos in the subject line, for example "sematic" should be "semantic". Please can people take care when writing the subject line, and fix it if needed? Paul ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-04 12:36 ` Adhemerval Zanella Netto 2023-08-05 7:21 ` Paul Zimmermann @ 2023-08-07 12:54 ` Florian Weimer 2023-08-07 12:59 ` Adhemerval Zanella Netto 1 sibling, 1 reply; 15+ messages in thread From: Florian Weimer @ 2023-08-07 12:54 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha, Carlos O'Donell * Adhemerval Zanella Netto: > On 04/08/23 05:43, Florian Weimer wrote: >> * Adhemerval Zanella: >> >>> POSIX relaxed the relation of setjmp/longjmp and the signal mask >>> save/restore, meaning that setjmp does not require to be routed to >>> _setjmp to be standard compliant. >>> >>> This is done to avoid breakage of SIGABRT handlers, since to fully >>> make abort AS-safe, it is required to remove the recurisve lock >>> used to unblock SIGABRT prior raised the signal. >>> >>> Also, it allows caller to actually use setjmp, since from >>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally >>> routed to _setjmp. >> >> I still think we shouldn't do this due to the performance implications. > > By not changing it and with the abort AS-safe fix, the following code > will always abort the program: > > -- > static jmp_buf jb; > > static void > sigabrt_handler (int sig) > { > longjmp (jb, 1); > } > > struct sigaction sa = { .sa_handler = sigabrt_handler, .sa_flags = 0 }; > sigemptyset (&sa.sa_mask); > assert (sigaction (SIGABRT, &sa, 0) == 0); > > if (setjmp (jb) == 0) > abort (); > > if (setjmp (jb) == 0) > abort (); > > // No reached. > -- > > Callers will need to change to sigsetjmp (..., 1) to have the same semantic. > That's the main reason I am suggesting this patch. Could we just unconditionally unblock SIGABRT at the start of abort, before the raise (SIGABRT) call? Other signal handlers could still observe this, but I think this change isn't really part of the core async signal safety fix. Thanks, Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-07 12:54 ` Florian Weimer @ 2023-08-07 12:59 ` Adhemerval Zanella Netto 2023-08-07 13:40 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Adhemerval Zanella Netto @ 2023-08-07 12:59 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Carlos O'Donell On 07/08/23 09:54, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 04/08/23 05:43, Florian Weimer wrote: >>> * Adhemerval Zanella: >>> >>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask >>>> save/restore, meaning that setjmp does not require to be routed to >>>> _setjmp to be standard compliant. >>>> >>>> This is done to avoid breakage of SIGABRT handlers, since to fully >>>> make abort AS-safe, it is required to remove the recurisve lock >>>> used to unblock SIGABRT prior raised the signal. >>>> >>>> Also, it allows caller to actually use setjmp, since from >>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally >>>> routed to _setjmp. >>> >>> I still think we shouldn't do this due to the performance implications. >> >> By not changing it and with the abort AS-safe fix, the following code >> will always abort the program: >> >> -- >> static jmp_buf jb; >> >> static void >> sigabrt_handler (int sig) >> { >> longjmp (jb, 1); >> } >> >> struct sigaction sa = { .sa_handler = sigabrt_handler, .sa_flags = 0 }; >> sigemptyset (&sa.sa_mask); >> assert (sigaction (SIGABRT, &sa, 0) == 0); >> >> if (setjmp (jb) == 0) >> abort (); >> >> if (setjmp (jb) == 0) >> abort (); >> >> // No reached. >> -- >> >> Callers will need to change to sigsetjmp (..., 1) to have the same semantic. >> That's the main reason I am suggesting this patch. > > Could we just unconditionally unblock SIGABRT at the start of abort, > before the raise (SIGABRT) call? Other signal handlers could still > observe this, but I think this change isn't really part of the core > async signal safety fix. My understanding is this still subject to same race condition with fork and posix_spawn signal handling setup, and that's why I have removed it. And we can't take a lock, even recursive, because it would prevent _Fork to be fully AS-safe. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-07 12:59 ` Adhemerval Zanella Netto @ 2023-08-07 13:40 ` Florian Weimer 2023-08-07 18:33 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2023-08-07 13:40 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha, Carlos O'Donell * Adhemerval Zanella Netto: > On 07/08/23 09:54, Florian Weimer wrote: >> * Adhemerval Zanella Netto: >> >>> On 04/08/23 05:43, Florian Weimer wrote: >>>> * Adhemerval Zanella: >>>> >>>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask >>>>> save/restore, meaning that setjmp does not require to be routed to >>>>> _setjmp to be standard compliant. >>>>> >>>>> This is done to avoid breakage of SIGABRT handlers, since to fully >>>>> make abort AS-safe, it is required to remove the recurisve lock >>>>> used to unblock SIGABRT prior raised the signal. >>>>> >>>>> Also, it allows caller to actually use setjmp, since from >>>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally >>>>> routed to _setjmp. >>>> >>>> I still think we shouldn't do this due to the performance implications. >>> >>> By not changing it and with the abort AS-safe fix, the following code >>> will always abort the program: >>> >>> -- >>> static jmp_buf jb; >>> >>> static void >>> sigabrt_handler (int sig) >>> { >>> longjmp (jb, 1); >>> } >>> >>> struct sigaction sa = { .sa_handler = sigabrt_handler, .sa_flags = 0 }; >>> sigemptyset (&sa.sa_mask); >>> assert (sigaction (SIGABRT, &sa, 0) == 0); >>> >>> if (setjmp (jb) == 0) >>> abort (); >>> >>> if (setjmp (jb) == 0) >>> abort (); >>> >>> // No reached. >>> -- >>> >>> Callers will need to change to sigsetjmp (..., 1) to have the same semantic. >>> That's the main reason I am suggesting this patch. >> >> Could we just unconditionally unblock SIGABRT at the start of abort, >> before the raise (SIGABRT) call? Other signal handlers could still >> observe this, but I think this change isn't really part of the core >> async signal safety fix. > > My understanding is this still subject to same race condition with fork > and posix_spawn signal handling setup, and that's why I have removed it. Concurrent fork from another thread does not matter here because that copies the signal mask from the fork-calling thread, not the abort-calling thread. Does this address your concern? I think the only gap is another signal being delivered to the abort-calling thread, and there the unblocking is sort-of implied by current POSIX anyway, so it's arguably to be observably. Thanks, Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-07 13:40 ` Florian Weimer @ 2023-08-07 18:33 ` Adhemerval Zanella Netto 2023-08-07 19:51 ` Florian Weimer 0 siblings, 1 reply; 15+ messages in thread From: Adhemerval Zanella Netto @ 2023-08-07 18:33 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Carlos O'Donell On 07/08/23 10:40, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 07/08/23 09:54, Florian Weimer wrote: >>> * Adhemerval Zanella Netto: >>> >>>> On 04/08/23 05:43, Florian Weimer wrote: >>>>> * Adhemerval Zanella: >>>>> >>>>>> POSIX relaxed the relation of setjmp/longjmp and the signal mask >>>>>> save/restore, meaning that setjmp does not require to be routed to >>>>>> _setjmp to be standard compliant. >>>>>> >>>>>> This is done to avoid breakage of SIGABRT handlers, since to fully >>>>>> make abort AS-safe, it is required to remove the recurisve lock >>>>>> used to unblock SIGABRT prior raised the signal. >>>>>> >>>>>> Also, it allows caller to actually use setjmp, since from >>>>>> 7011c2622fe3e10a29dbe74f06aaebd07710127d the symbol is unconditionally >>>>>> routed to _setjmp. >>>>> >>>>> I still think we shouldn't do this due to the performance implications. >>>> >>>> By not changing it and with the abort AS-safe fix, the following code >>>> will always abort the program: >>>> >>>> -- >>>> static jmp_buf jb; >>>> >>>> static void >>>> sigabrt_handler (int sig) >>>> { >>>> longjmp (jb, 1); >>>> } >>>> >>>> struct sigaction sa = { .sa_handler = sigabrt_handler, .sa_flags = 0 }; >>>> sigemptyset (&sa.sa_mask); >>>> assert (sigaction (SIGABRT, &sa, 0) == 0); >>>> >>>> if (setjmp (jb) == 0) >>>> abort (); >>>> >>>> if (setjmp (jb) == 0) >>>> abort (); >>>> >>>> // No reached. >>>> -- >>>> >>>> Callers will need to change to sigsetjmp (..., 1) to have the same semantic. >>>> That's the main reason I am suggesting this patch. >>> >>> Could we just unconditionally unblock SIGABRT at the start of abort, >>> before the raise (SIGABRT) call? Other signal handlers could still >>> observe this, but I think this change isn't really part of the core >>> async signal safety fix. >> >> My understanding is this still subject to same race condition with fork >> and posix_spawn signal handling setup, and that's why I have removed it. > > Concurrent fork from another thread does not matter here because that > copies the signal mask from the fork-calling thread, not the > abort-calling thread. Does this address your concern? > > I think the only gap is another signal being delivered to the > abort-calling thread, and there the unblocking is sort-of implied by > current POSIX anyway, so it's arguably to be observably. I still think unblocking the signal on raise is not fully correct, since calling abort in the SIGABRT handler itself leads to a recursive call to the handler, which will eventually lead to a stack overflow. By not messing with the signal handler, the process will be correctly terminated as by receiving a SIGABRT. ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-07 18:33 ` Adhemerval Zanella Netto @ 2023-08-07 19:51 ` Florian Weimer 2023-08-07 20:49 ` Adhemerval Zanella Netto 0 siblings, 1 reply; 15+ messages in thread From: Florian Weimer @ 2023-08-07 19:51 UTC (permalink / raw) To: Adhemerval Zanella Netto; +Cc: libc-alpha, Carlos O'Donell * Adhemerval Zanella Netto: > I still think unblocking the signal on raise is not fully correct, since > calling abort in the SIGABRT handler itself leads to a recursive call > to the handler, which will eventually lead to a stack overflow. We can't know that, it depends on how the handler is written. I think it's better to call the handler at least once (even if the signal was blocked) and risk terminating with SIGSEGV due to recursive signals. Thanks, Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp 2023-08-07 19:51 ` Florian Weimer @ 2023-08-07 20:49 ` Adhemerval Zanella Netto 0 siblings, 0 replies; 15+ messages in thread From: Adhemerval Zanella Netto @ 2023-08-07 20:49 UTC (permalink / raw) To: Florian Weimer; +Cc: libc-alpha, Carlos O'Donell On 07/08/23 16:51, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> I still think unblocking the signal on raise is not fully correct, since >> calling abort in the SIGABRT handler itself leads to a recursive call >> to the handler, which will eventually lead to a stack overflow. > > We can't know that, it depends on how the handler is written. But the idea of making abort AS-safe is also to allow it being called in SIGABRT handler. Adding UB is not really an improvement, specially if the idea to moving the interface to be AS-safe. > > I think it's better to call the handler at least once (even if the > signal was blocked) and risk terminating with SIGSEGV due to recursive > signals. > I disagree and SIGSEGV can be non deterministic depending of the defined stack size. And I am meant with default signal mask, using SA_NODEFER for SIGABR handler will require additional synchronization by the handler. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/2] stdlib: Make abort AS-safe (BZ 26275) 2023-08-03 17:34 [PATCH v2 0/2] Make abort AS-safe Adhemerval Zanella 2023-08-03 17:34 ` [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp Adhemerval Zanella @ 2023-08-03 17:34 ` Adhemerval Zanella 2023-08-03 18:05 ` Joe Simmons-Talbott 1 sibling, 1 reply; 15+ messages in thread From: Adhemerval Zanella @ 2023-08-03 17:34 UTC (permalink / raw) To: libc-alpha, Carlos O'Donell, Florian Weimer The recursive lock used on abort does not synchronize with 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. To fix the AS-safe, the raise is issues 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. The 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 posix_spawn possible issue 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. The fallback is also simplified, there is no nned to use a loop of ABORT_INSTRUCTION after _exit (if the syscall does not terminate the process, the system is really broken). Checked on x86_64-linux-gnu and aarch64-linux-gnu. --- include/bits/unistd_ext.h | 3 + include/stdlib.h | 6 + manual/startup.texi | 3 - nptl/pthread_kill.c | 11 ++ posix/fork.c | 2 + signal/sigaction.c | 15 ++- stdlib/abort.c | 130 ++++++++------------- 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 | 6 +- 15 files changed, 159 insertions(+), 93 deletions(-) create mode 100644 sysdeps/generic/internal-sigset.h diff --git a/include/bits/unistd_ext.h b/include/bits/unistd_ext.h index 277be05746..eeb07baf70 100644 --- a/include/bits/unistd_ext.h +++ b/include/bits/unistd_ext.h @@ -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 diff --git a/include/stdlib.h b/include/stdlib.h index 7deb8193d7..e46f049576 100644 --- a/include/stdlib.h +++ b/include/stdlib.h @@ -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; @@ -75,6 +76,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) 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..a8fcf497c3 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) +{ + /* 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) { 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..2eaa8f0b09 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,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) diff --git a/stdlib/abort.c b/stdlib/abort.c index 16a453459c..1fb2cee121 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,55 @@ 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 (internal_sigset_t *set) +{ + internal_signal_block_all (set); + __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 (const internal_sigset_t *set) +{ + __libc_lock_unlock (lock); + internal_signal_restore_set (set); +} - /* 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 dispositioni (it will block on __abort_lock), + reinstall the handle to abort the process, and re-raise the signal. */ + 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..1143b765e4 100644 --- a/sysdeps/generic/internal-signals.h +++ b/sysdeps/generic/internal-signals.h @@ -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 */ diff --git a/sysdeps/generic/internal-sigset.h b/sysdeps/generic/internal-sigset.h new file mode 100644 index 0000000000..80279ffc47 --- /dev/null +++ b/sysdeps/generic/internal-sigset.h @@ -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 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..3bddfde4fa 100644 --- a/sysdeps/nptl/_Fork.c +++ b/sysdeps/nptl/_Fork.c @@ -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) 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/internal-sigset.h b/sysdeps/unix/sysv/linux/internal-sigset.h index 569bab73b0..9c1deb2cde 100644 --- a/sysdeps/unix/sysv/linux/internal-sigset.h +++ b/sysdeps/unix/sysv/linux/internal-sigset.h @@ -21,7 +21,7 @@ #include <sigsetops.h> -typedef struct +typedef struct _internal_sigset_t { unsigned long int __val[__NSIG_WORDS]; } internal_sigset_t; diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c index ec687cb423..9b41f0f869 100644 --- a/sysdeps/unix/sysv/linux/spawni.c +++ b/sysdeps/unix/sysv/linux/spawni.c @@ -370,7 +370,9 @@ __spawnix (pid_t * pid, const char *file, args.envp = envp; args.xflags = xflags; - internal_signal_block_all (&args.oldmask); + /* Avoid the abort to change the SIGABRT disposition to SIG_DFL for the + case POSIX_SPAWN_SETSIGDEF is not set and SIG_IGN is current handle. */ + __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 @@ -431,7 +433,7 @@ __spawnix (pid_t * pid, const char *file, if ((ec == 0) && (pid != NULL)) *pid = new_pid; - internal_signal_restore_set (&args.oldmask); + __abort_lock_unlock (&args.oldmask); __pthread_setcancelstate (state, NULL); -- 2.34.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2 2/2] stdlib: Make abort AS-safe (BZ 26275) 2023-08-03 17:34 ` [PATCH v2 2/2] stdlib: Make abort AS-safe (BZ 26275) Adhemerval Zanella @ 2023-08-03 18:05 ` Joe Simmons-Talbott 0 siblings, 0 replies; 15+ messages in thread From: Joe Simmons-Talbott @ 2023-08-03 18:05 UTC (permalink / raw) To: Adhemerval Zanella; +Cc: libc-alpha, Carlos O'Donell, Florian Weimer On Thu, Aug 03, 2023 at 02:34:36PM -0300, Adhemerval Zanella via Libc-alpha wrote: > The recursive lock used on abort does not synchronize with 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. > > To fix the AS-safe, the raise is issues without changing the process issued > signal mask, and an AS-safe lock is used if a SIGABRT is installed or > the process is blocked or ignored. The 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 posix_spawn possible issue 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. > > The fallback is also simplified, there is no nned to use a loop of need > ABORT_INSTRUCTION after _exit (if the syscall does not terminate the > process, the system is really broken). > > Checked on x86_64-linux-gnu and aarch64-linux-gnu. > --- > include/bits/unistd_ext.h | 3 + > include/stdlib.h | 6 + > manual/startup.texi | 3 - > nptl/pthread_kill.c | 11 ++ > posix/fork.c | 2 + > signal/sigaction.c | 15 ++- > stdlib/abort.c | 130 ++++++++------------- > 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 | 6 +- > 15 files changed, 159 insertions(+), 93 deletions(-) > create mode 100644 sysdeps/generic/internal-sigset.h > > diff --git a/include/bits/unistd_ext.h b/include/bits/unistd_ext.h > index 277be05746..eeb07baf70 100644 > --- a/include/bits/unistd_ext.h > +++ b/include/bits/unistd_ext.h > @@ -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 > diff --git a/include/stdlib.h b/include/stdlib.h > index 7deb8193d7..e46f049576 100644 > --- a/include/stdlib.h > +++ b/include/stdlib.h > @@ -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; > @@ -75,6 +76,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) > 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..a8fcf497c3 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) > +{ > + /* 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) > { > 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..2eaa8f0b09 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,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) > diff --git a/stdlib/abort.c b/stdlib/abort.c > index 16a453459c..1fb2cee121 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,55 @@ > 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 (internal_sigset_t *set) > +{ > + internal_signal_block_all (set); > + __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 (const internal_sigset_t *set) > +{ > + __libc_lock_unlock (lock); > + internal_signal_restore_set (set); > +} > > - /* 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 dispositioni (it will block on __abort_lock), > + reinstall the handle to abort the process, and re-raise the signal. */ > + 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..1143b765e4 100644 > --- a/sysdeps/generic/internal-signals.h > +++ b/sysdeps/generic/internal-signals.h > @@ -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 */ > diff --git a/sysdeps/generic/internal-sigset.h b/sysdeps/generic/internal-sigset.h > new file mode 100644 > index 0000000000..80279ffc47 > --- /dev/null > +++ b/sysdeps/generic/internal-sigset.h > @@ -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 > 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..3bddfde4fa 100644 > --- a/sysdeps/nptl/_Fork.c > +++ b/sysdeps/nptl/_Fork.c > @@ -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) > 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/internal-sigset.h b/sysdeps/unix/sysv/linux/internal-sigset.h > index 569bab73b0..9c1deb2cde 100644 > --- a/sysdeps/unix/sysv/linux/internal-sigset.h > +++ b/sysdeps/unix/sysv/linux/internal-sigset.h > @@ -21,7 +21,7 @@ > > #include <sigsetops.h> > > -typedef struct > +typedef struct _internal_sigset_t > { > unsigned long int __val[__NSIG_WORDS]; > } internal_sigset_t; > diff --git a/sysdeps/unix/sysv/linux/spawni.c b/sysdeps/unix/sysv/linux/spawni.c > index ec687cb423..9b41f0f869 100644 > --- a/sysdeps/unix/sysv/linux/spawni.c > +++ b/sysdeps/unix/sysv/linux/spawni.c > @@ -370,7 +370,9 @@ __spawnix (pid_t * pid, const char *file, > args.envp = envp; > args.xflags = xflags; > > - internal_signal_block_all (&args.oldmask); > + /* Avoid the abort to change the SIGABRT disposition to SIG_DFL for the > + case POSIX_SPAWN_SETSIGDEF is not set and SIG_IGN is current handle. */ > + __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 > @@ -431,7 +433,7 @@ __spawnix (pid_t * pid, const char *file, > if ((ec == 0) && (pid != NULL)) > *pid = new_pid; > > - internal_signal_restore_set (&args.oldmask); > + __abort_lock_unlock (&args.oldmask); > > __pthread_setcancelstate (state, NULL); > > -- > 2.34.1 > ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-08-07 20:50 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-03 17:34 [PATCH v2 0/2] Make abort AS-safe Adhemerval Zanella 2023-08-03 17:34 ` [PATCH v2 1/2] setjmp: Use BSD sematic as default for setjmp Adhemerval Zanella 2023-08-03 18:06 ` Joe Simmons-Talbott 2023-08-03 22:09 ` Joseph Myers 2023-08-04 8:43 ` Florian Weimer 2023-08-04 12:36 ` Adhemerval Zanella Netto 2023-08-05 7:21 ` Paul Zimmermann 2023-08-07 12:54 ` Florian Weimer 2023-08-07 12:59 ` Adhemerval Zanella Netto 2023-08-07 13:40 ` Florian Weimer 2023-08-07 18:33 ` Adhemerval Zanella Netto 2023-08-07 19:51 ` Florian Weimer 2023-08-07 20:49 ` Adhemerval Zanella Netto 2023-08-03 17:34 ` [PATCH v2 2/2] stdlib: Make abort AS-safe (BZ 26275) Adhemerval Zanella 2023-08-03 18:05 ` Joe Simmons-Talbott
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).