RFR: JDK-8293466: libjsig should ignore non-modifying sigaction calls

[Man Cao]

On Sat, 10 Sep 2022 06:15:05 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

> Found during code review of [JDK-8292695](https://bugs.openjdk.org/browse/JDK-8292695).
> 
> We have two bugs in libjsig when we install hotspot signal handlers. Relevant code in libjsig:
> 
> 
> int sigaction(int sig, const struct sigaction *act, struct sigaction *oact) {
> <snip>
> 
>   sigused = sigismember(&jvmsigs, sig);
>   if (jvm_signal_installed && sigused) {
>     /* jvm has installed its signal handler for this signal. */
>     /* Save the handler. Don't really install it. */
>     if (oact != NULL) {
>       *oact = sact[sig];
>     }
>     if (act != NULL) {
>       sact[sig] = *act;
>     }
> 
>     signal_unlock();
>     return 0;
>   } else if (jvm_signal_installing) {
>     /* jvm is installing its signal handlers. Install the new
>      * handlers and save the old ones. */
>     res = call_os_sigaction(sig, act, &oldAct);
>     sact[sig] = oldAct;
>     if (oact != NULL) {
>       *oact = oldAct;
>     }
> 
>     /* Record the signals used by jvm. */
>     sigaddset(&jvmsigs, sig);
> 
>     signal_unlock();
>     return res;
>   }
> <snip>
> }
> 
> 
> Bug 1: we change state even if the sigaction call failed
> Bug 2: we change state even if the sigaction call was a non-modifying one (act == NULL)
> 
> The latter is usually no problem since hotspot always calls `sigaction()` in pairs when installing a signal: first with NULL to get the old handler, then with the real handler. But this is not always true. If `AllowUserSignalHandlers` is set, and we find a custom handler is present, we will not override it:
> 
> 
> void set_signal_handler(int sig, bool do_check = true) {
>   // Check for overwrite.
>   struct sigaction oldAct;
>   sigaction(sig, (struct sigaction*)NULL, &oldAct); <<<<< first sigaction call, libjsig now remembers signal as set
> 
>   // Query the current signal handler. Needs to be a separate operation
>   // from installing a new handler since we need to honor AllowUserSignalHandlers.
>   void* oldhand = get_signal_handler(&oldAct);
>   if (!HANDLER_IS_IGN_OR_DFL(oldhand) &&
>       !HANDLER_IS(oldhand, javaSignalHandler)) {
>     if (AllowUserSignalHandlers) {
>       // Do not overwrite; user takes responsibility to forward to us.
>       return;
> 
> 
> That means:
> - we still have the original custom handler in place
> - but we already called sigaction, albeit with NULL, but libjsig now assumes that hotspot installed a handler itself.
> 
> The result is that any further attempts to change the signal handler, whether by hotspot or by user code, will be prevented by libjsig. Any further non-modifying sigaction calls will return the original - still installed - custom handler.
> 
> Admittedly, the error is very exotic. Users would have to set AllowUserSignalHandlers and preload libjsig, and *then* attempt to modify signal handlers after JVM initialization. But it is confusing, and a potential source for other errors. In hotspot, nobody counts on a non-modifying sigaction query changing program state somewhere.
> 
> This seems to be an old bug, I see it in at least JDK 8. Did not look further into the past
> 
> ---
> 
> Tests: Ran the runtime/jsig and the runtime/Thread tests manually.

LGTM. Not an official OpenJDK Reviewer though.

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

Marked as reviewed by manc (Committer).

PR: https://git.openjdk.org/jdk/pull/10236