| Message ID | 20230731171900.4065501-2-adhemerval.zanella@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Make abort AS-safe | expand |
* Adhemerval Zanella via Libc-alpha: > 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. Doesn't this have non-trivial performance impact? Thanks, Florian
On 01/08/23 05:35, Florian Weimer wrote: > * Adhemerval Zanella via Libc-alpha: > >> 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. > > Doesn't this have non-trivial performance impact? Yes, it is two extra sigprocmask to get/set the signal mask. This is not *strictly* required, but the SIGABRT on abort generates racy conditions on process creation and. This patch can be dropped, but it would mean that to get expected semantic for abort handlers will need to use sigsetjmp (..., 1) instead of setjmp.
* Adhemerval Zanella Netto: > On 01/08/23 05:35, Florian Weimer wrote: >> * Adhemerval Zanella via Libc-alpha: >> >>> 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. >> >> Doesn't this have non-trivial performance impact? > > Yes, it is two extra sigprocmask to get/set the signal mask. This is > not *strictly* required, but the SIGABRT on abort generates racy > conditions on process creation and. This patch can be dropped, but it > would mean that to get expected semantic for abort handlers will need > to use sigsetjmp (..., 1) instead of setjmp. Sorry, I don't understand? With the current locking, this change should really not be required because the user SIGABRT handler does not run with the signal mask changed. Thanks, Florian
On 02/08/23 04:59, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 01/08/23 05:35, Florian Weimer wrote: >>> * Adhemerval Zanella via Libc-alpha: >>> >>>> 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. >>> >>> Doesn't this have non-trivial performance impact? >> >> Yes, it is two extra sigprocmask to get/set the signal mask. This is >> not *strictly* required, but the SIGABRT on abort generates racy >> conditions on process creation and. This patch can be dropped, but it >> would mean that to get expected semantic for abort handlers will need >> to use sigsetjmp (..., 1) instead of setjmp. > > Sorry, I don't understand? With the current locking, this change should > really not be required because the user SIGABRT handler does not run > with the signal mask changed. > This change is only required to keep the same semantic of setjmp/longjmp regarding SIGABRT handler, where current code keeps subsequent SIGABRT installed with default flags to not keep the signal masked. Otherwise, users that callers that handle SIGABRT will need to either a different sigaction mask that do no change the blocked signals while handling the signal, call sigprocmask after SIGABRT returns from longjmp, or use sigsetjmp.
* Adhemerval Zanella Netto: > On 02/08/23 04:59, Florian Weimer wrote: >> * Adhemerval Zanella Netto: >> >>> On 01/08/23 05:35, Florian Weimer wrote: >>>> * Adhemerval Zanella via Libc-alpha: >>>> >>>>> 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. >>>> >>>> Doesn't this have non-trivial performance impact? >>> >>> Yes, it is two extra sigprocmask to get/set the signal mask. This is >>> not *strictly* required, but the SIGABRT on abort generates racy >>> conditions on process creation and. This patch can be dropped, but it >>> would mean that to get expected semantic for abort handlers will need >>> to use sigsetjmp (..., 1) instead of setjmp. >> >> Sorry, I don't understand? With the current locking, this change should >> really not be required because the user SIGABRT handler does not run >> with the signal mask changed. >> > > This change is only required to keep the same semantic of setjmp/longjmp > regarding SIGABRT handler, where current code keeps subsequent SIGABRT > installed with default flags to not keep the signal masked. Otherwise, > users that callers that handle SIGABRT will need to either a different > sigaction mask that do no change the blocked signals while handling > the signal, call sigprocmask after SIGABRT returns from longjmp, or > use sigsetjmp. Sorry, I still don't see it. The new code switches the handler to SIG_DFL atomically and blocks further sigaction calls. This extends to subprocesses because creating them is inhibited, too. I think this means that the difference in signal handler masking is not observable. Thanks, Florian
On 02/08/23 09:42, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 02/08/23 04:59, Florian Weimer wrote: >>> * Adhemerval Zanella Netto: >>> >>>> On 01/08/23 05:35, Florian Weimer wrote: >>>>> * Adhemerval Zanella via Libc-alpha: >>>>> >>>>>> 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. >>>>> >>>>> Doesn't this have non-trivial performance impact? >>>> >>>> Yes, it is two extra sigprocmask to get/set the signal mask. This is >>>> not *strictly* required, but the SIGABRT on abort generates racy >>>> conditions on process creation and. This patch can be dropped, but it >>>> would mean that to get expected semantic for abort handlers will need >>>> to use sigsetjmp (..., 1) instead of setjmp. >>> >>> Sorry, I don't understand? With the current locking, this change should >>> really not be required because the user SIGABRT handler does not run >>> with the signal mask changed. >>> >> >> This change is only required to keep the same semantic of setjmp/longjmp >> regarding SIGABRT handler, where current code keeps subsequent SIGABRT >> installed with default flags to not keep the signal masked. Otherwise, >> users that callers that handle SIGABRT will need to either a different >> sigaction mask that do no change the blocked signals while handling >> the signal, call sigprocmask after SIGABRT returns from longjmp, or >> use sigsetjmp. > > Sorry, I still don't see it. The new code switches the handler to > SIG_DFL atomically and blocks further sigaction calls. This extends to > subprocesses because creating them is inhibited, too. I think this > means that the difference in signal handler masking is not observable. But this change it no to handle if raise returns, but rather if you have a SIGABRT handler that does not (like the fortify tests) installed with default flags. In this case, the kernel will add SIGABRT on the masked signal, longjmp will return to setjmp with the SIGBRT handler set mask, and the next SIGABRT won't trigger the handler
* Adhemerval Zanella Netto: > On 02/08/23 09:42, Florian Weimer wrote: >> * Adhemerval Zanella Netto: >> >>> On 02/08/23 04:59, Florian Weimer wrote: >>>> * Adhemerval Zanella Netto: >>>> >>>>> On 01/08/23 05:35, Florian Weimer wrote: >>>>>> * Adhemerval Zanella via Libc-alpha: >>>>>> >>>>>>> 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. >>>>>> >>>>>> Doesn't this have non-trivial performance impact? >>>>> >>>>> Yes, it is two extra sigprocmask to get/set the signal mask. This is >>>>> not *strictly* required, but the SIGABRT on abort generates racy >>>>> conditions on process creation and. This patch can be dropped, but it >>>>> would mean that to get expected semantic for abort handlers will need >>>>> to use sigsetjmp (..., 1) instead of setjmp. >>>> >>>> Sorry, I don't understand? With the current locking, this change should >>>> really not be required because the user SIGABRT handler does not run >>>> with the signal mask changed. >>>> >>> >>> This change is only required to keep the same semantic of setjmp/longjmp >>> regarding SIGABRT handler, where current code keeps subsequent SIGABRT >>> installed with default flags to not keep the signal masked. Otherwise, >>> users that callers that handle SIGABRT will need to either a different >>> sigaction mask that do no change the blocked signals while handling >>> the signal, call sigprocmask after SIGABRT returns from longjmp, or >>> use sigsetjmp. >> >> Sorry, I still don't see it. The new code switches the handler to >> SIG_DFL atomically and blocks further sigaction calls. This extends to >> subprocesses because creating them is inhibited, too. I think this >> means that the difference in signal handler masking is not observable. > > But this change it no to handle if raise returns, but rather if you have > a SIGABRT handler that does not (like the fortify tests) installed with > default flags. In this case, the kernel will add SIGABRT on the masked > signal, longjmp will return to setjmp with the SIGBRT handler set mask, > and the next SIGABRT won't trigger the handler Ahh, you mean because use removed the signal unblocking from abort? If the signal is blocked, it is not delivered before it is unblocked. This means that the handler will not observe it blocked. But POSIX says this: | The abort() function shall override blocking or ignoring the SIGABRT | signal. It also says: | The SIGABRT signal shall be sent to the calling process as if by means | of raise() with the argument SIGABRT. Strictly speaking, it is impossible to comply with both requirements, but I think the handler is expected to run even if SIGABRT is blocked. As far as I understand it, the new code terminates the process in this case, without ever running the handler. Thanks, Florian
On 02/08/23 10:17, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >> On 02/08/23 09:42, Florian Weimer wrote: >>> * Adhemerval Zanella Netto: >>> >>>> On 02/08/23 04:59, Florian Weimer wrote: >>>>> * Adhemerval Zanella Netto: >>>>> >>>>>> On 01/08/23 05:35, Florian Weimer wrote: >>>>>>> * Adhemerval Zanella via Libc-alpha: >>>>>>> >>>>>>>> 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. >>>>>>> >>>>>>> Doesn't this have non-trivial performance impact? >>>>>> >>>>>> Yes, it is two extra sigprocmask to get/set the signal mask. This is >>>>>> not *strictly* required, but the SIGABRT on abort generates racy >>>>>> conditions on process creation and. This patch can be dropped, but it >>>>>> would mean that to get expected semantic for abort handlers will need >>>>>> to use sigsetjmp (..., 1) instead of setjmp. >>>>> >>>>> Sorry, I don't understand? With the current locking, this change should >>>>> really not be required because the user SIGABRT handler does not run >>>>> with the signal mask changed. >>>>> >>>> >>>> This change is only required to keep the same semantic of setjmp/longjmp >>>> regarding SIGABRT handler, where current code keeps subsequent SIGABRT >>>> installed with default flags to not keep the signal masked. Otherwise, >>>> users that callers that handle SIGABRT will need to either a different >>>> sigaction mask that do no change the blocked signals while handling >>>> the signal, call sigprocmask after SIGABRT returns from longjmp, or >>>> use sigsetjmp. >>> >>> Sorry, I still don't see it. The new code switches the handler to >>> SIG_DFL atomically and blocks further sigaction calls. This extends to >>> subprocesses because creating them is inhibited, too. I think this >>> means that the difference in signal handler masking is not observable. >> >> But this change it no to handle if raise returns, but rather if you have >> a SIGABRT handler that does not (like the fortify tests) installed with >> default flags. In this case, the kernel will add SIGABRT on the masked >> signal, longjmp will return to setjmp with the SIGBRT handler set mask, >> and the next SIGABRT won't trigger the handler > > Ahh, you mean because use removed the signal unblocking from abort? > > If the signal is blocked, it is not delivered before it is unblocked. > This means that the handler will not observe it blocked. > > But POSIX says this: > > | The abort() function shall override blocking or ignoring the SIGABRT > | signal. > > It also says: > > | The SIGABRT signal shall be sent to the calling process as if by means > | of raise() with the argument SIGABRT. > > Strictly speaking, it is impossible to comply with both requirements, > but I think the handler is expected to run even if SIGABRT is blocked. > As far as I understand it, the new code terminates the process in this > case, without ever running the handler. The later has been changed with a new clarification [1]: The SIGABRT signal shall be sent to the calling [CX]thread[/CX] as if by means of raise() with the argument SIGABRT. [CX]If this signal does not terminate the process (for example, if the signal is caught and the handler returns), abort() may change the disposition of SIGABRT to SIG_DFL and send the signal (in the same way) again. If a second signal is sent and it does not terminate the process, the behavior is unspecified, except that the abort() call shall not return.[/CX] [1] https://austingroupbugs.net/view.php?id=906#c5851
* Adhemerval Zanella Netto: >> Ahh, you mean because use removed the signal unblocking from abort? >> >> If the signal is blocked, it is not delivered before it is unblocked. >> This means that the handler will not observe it blocked. >> >> But POSIX says this: >> >> | The abort() function shall override blocking or ignoring the SIGABRT >> | signal. >> >> It also says: >> >> | The SIGABRT signal shall be sent to the calling process as if by means >> | of raise() with the argument SIGABRT. >> >> Strictly speaking, it is impossible to comply with both requirements, >> but I think the handler is expected to run even if SIGABRT is blocked. >> As far as I understand it, the new code terminates the process in this >> case, without ever running the handler. > > The later has been changed with a new clarification [1]: > > The SIGABRT signal shall be sent to the calling [CX]thread[/CX] as if by > means of raise() with the argument SIGABRT. [CX]If this signal does not > terminate the process (for example, if the signal is caught and the handler > returns), abort() may change the disposition of SIGABRT to SIG_DFL and send > the signal (in the same way) again. If a second signal is sent and it does > not terminate the process, the behavior is unspecified, except that the > abort() call shall not return.[/CX] > > [1] https://austingroupbugs.net/view.php?id=906#c5851 Okay, I missed that change. So removing the unblocking should be okay after this specification change. I still don't see how the removal of unblocking changes the signal mask observed by the signal handler, though. Thanks, Florian
On 02/08/23 11:43, Florian Weimer wrote: > * Adhemerval Zanella Netto: > >>> Ahh, you mean because use removed the signal unblocking from abort? >>> >>> If the signal is blocked, it is not delivered before it is unblocked. >>> This means that the handler will not observe it blocked. >>> >>> But POSIX says this: >>> >>> | The abort() function shall override blocking or ignoring the SIGABRT >>> | signal. >>> >>> It also says: >>> >>> | The SIGABRT signal shall be sent to the calling process as if by means >>> | of raise() with the argument SIGABRT. >>> >>> Strictly speaking, it is impossible to comply with both requirements, >>> but I think the handler is expected to run even if SIGABRT is blocked. >>> As far as I understand it, the new code terminates the process in this >>> case, without ever running the handler. >> >> The later has been changed with a new clarification [1]: >> >> The SIGABRT signal shall be sent to the calling [CX]thread[/CX] as if by >> means of raise() with the argument SIGABRT. [CX]If this signal does not >> terminate the process (for example, if the signal is caught and the handler >> returns), abort() may change the disposition of SIGABRT to SIG_DFL and send >> the signal (in the same way) again. If a second signal is sent and it does >> not terminate the process, the behavior is unspecified, except that the >> abort() call shall not return.[/CX] >> >> [1] https://austingroupbugs.net/view.php?id=906#c5851 > > Okay, I missed that change. So removing the unblocking should be okay > after this specification change. I still don't see how the removal of > unblocking changes the signal mask observed by the signal handler, > though. It is not by the signal handler, but rather after a SIGABRT handler issues longjmp. With default flags (0), SIGABRT will continue to be blocked: $ cat test.c #include <assert.h> #include <setjmp.h> #include <signal.h> #include <stdio.h> #include <stdlib.h> static jmp_buf jb; static void sigabrt_handler (int sig) { longjmp (jb, 1); } int main (int argc, char *argv[]) { sigset_t set; assert (sigprocmask (SIG_BLOCK, NULL, &set) == 0); printf ("SIGABRT blocked: %d\n", sigismember (&set, SIGABRT)); 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 (); printf ("first abort did not terminated\n"); assert (sigprocmask (SIG_BLOCK, NULL, &set) == 0); printf ("SIGABRT blocked: %d\n", sigismember (&set, SIGABRT)); if (setjmp (jb) == 0) abort (); printf ("second abort did not terminated\n"); return 0; } $ gcc test.c -o test && ./test SIGABRT blocked: 0 first abort did not terminated SIGABRT blocked: 1 second abort did not terminated By continuing to use _setjmp as 'setjmp' and not unblocking SIGABRT on abort call, the process will be aborted regardless. This is due the difference historically between BSD and SysV regarding whether setjmp/longjmp should save/restore the signal mask (POSIX current allows both semantics).
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)) {