RFR: JDK-8252533: Signal handlers should run with synchronous error signals unblocked

[Gerard Ziemski]

On Mon, 26 Oct 2020 11:43:54 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi Thomas,
>> 
>> I think this has highlighted a number of bugs with our signal handling code in general. It makes no sense to ever block these synchronous signals, so any code that does so seems flawed. You also point out the semantics of sa_sigaction.sa_mask which highlights that call_chained_handler is incorrect as it tries to emulate the conditions under which the original handler would have been called, but it explicitly sets a sigmask from sa_mask instead of combining it with the existing sigmask.
>> 
>> Aside: I also noticed the SR_sigset is unused, so unclear if we have a bug lurking in PosixSignals::SR_initialize. There is some commentary about the blocked/unblocked status of SR_signum that also seems wrong.
>> 
>> That all said ... AFAICS the synchronous signals are all explicitly unblocked via PosixSignals::hotspot_sigmask which is set when a new thread starts executing, or an existing thread attaches. So from that point we should only need to ensure those signals are not explicitly blocked - which means that in os::signal we unblock them in sa_mask for all platforms. Does that not suffice? Or are you concerned about user native code changing the signal masks arbitrarily? In which case re-setting in each of the VM's signal handlers would be reasonable.
>> 
>> Note that SIGTRAP is currently mostly only processed under #if defined(PPC64)and/or defined(AIX).
>> 
>> Thanks,
>> David
>
>> Hi Thomas,
>> 
>> I think this has highlighted a number of bugs with our signal handling code in general. It makes no sense to ever block these synchronous signals, so any code that does so seems flawed. You also point out the semantics of sa_sigaction.sa_mask which highlights that call_chained_handler is incorrect as it tries to emulate the conditions under which the original handler would have been called, but it explicitly sets a sigmask from sa_mask instead of combining it with the existing sigmask.
>> 
> 
> Yes, this coding may benefit from a bit more grooming (but preferably in another RFE).
> 
>> Aside: I also noticed the SR_sigset is unused, so unclear if we have a bug lurking in PosixSignals::SR_initialize. There is some commentary about the blocked/unblocked status of SR_signum that also seems wrong.
>> 
>> That all said ... AFAICS the synchronous signals are all explicitly unblocked via PosixSignals::hotspot_sigmask which is set when a new thread starts executing, or an existing thread attaches. So from that point we should only need to ensure those signals are not explicitly blocked - which means that in os::signal we unblock them in sa_mask for all platforms. Does that not suffice? Or are you concerned about user native code changing the signal masks arbitrarily? In which case re-setting in each of the VM's signal handlers would be reasonable.
>> 
> 
> You are right. This is what I tried to say with "sa_sigaction.sa_mask vs pthread_sigmask". 
> 
> I chose explicit unblocking over setting the signal mask when establishing the handler because I was concerned about someone changing the thread signal mask after handler is installed and before the signal is triggered. Especially I am not sure what happens if some native code calls sigprocmask() - does that affect all threads, only this thread? Posix says its undefined. Because I did not want to think about all this, and because I found it slightly more obvious, I unblock explicitly.
> 
> That said, it was a bit of a coin toss, and if you dislike this, we can do it via sigaction.sa_mask instead. The danger of someone blocking synchronous error signals under our nose is probably remote :)
> 
>> Note that SIGTRAP is currently mostly only processed under #if defined(PPC64)and/or defined(AIX).
> 
> Yes, we use it for implicit null checks. But this has only meaning outside of signal handling. I unblocked it inside signal handlers because I found vague reports of processes crashing with blocked SIGTRAPs and thought that the same reasoning may apply to this signal wrt to blocking in signal handlers.
> 
> Blocking SIGTRAP has been handled inconsistently, too. It had been unblocked at the entrance of the secondary error handler for all platforms (see vmError_posix.cpp) but not in general error handling. I think that is an oversight.
> 
>> 
>> Thanks,
>> David
> 
> Thanks, Thomas

hi Thomas,

Looks good as far as I can tell.

I'm still learning the POSIX signal code, so excuse the question, but if our signal handlers are capable of handling the synchronous signals while doing fatal error handling, then why don't we unblock it for all the signals, so that we can handle any kind of a situation inside the fatal error handling?

Also, since we're touching the "VMError::reset_signal_handlers()" any chance we can improve on the name here, as you yourself suggested in other review?

cheers

-------------

PR: https://git.openjdk.java.net/jdk/pull/839