| Message ID | 20230803173436.4146900-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make abort AS-safe | expand |
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 >
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.
* 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
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.
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
* 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
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.
* 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
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.
* 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
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.
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)) {