Re: [PATCH 1/2] setjmp: Use BSD sematic as default for setjmp

[Adhemerval Zanella Netto <adhemerval.zanella@linaro.org>]

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