JDK-8293466 : libjsig should ignore non-modifying sigaction calls
  • Type: Bug
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 8,11,17,20
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • Submitted: 2022-09-07
  • Updated: 2023-10-03
  • Resolved: 2022-09-19
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 11 JDK 17 JDK 20
11.0.22Fixed 17.0.7Fixed 20 b16Fixed
Related Reports
Relates :  
Description
Found during code review of 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 than that.
Comments
A pull request was submitted for review. URL: https://git.openjdk.org/jdk11u-dev/pull/2158 Date: 2023-10-02 07:42:41 +0000
02-10-2023

Fix request [11u] I backport this to fix this small issue. Risk is low, rather simple change. Clean backport. SAP nighltly Tesing passed.
02-10-2023

A pull request was submitted for review. URL: https://git.openjdk.org/jdk17u-dev/pull/1049 Date: 2023-01-10 14:11:03 +0000
10-01-2023

Fix Request: I'd like to backport this fix. The probability of this error happening is low, but it is an error nonetheless and may cause signal chaining to misbehave. Risk is low.
10-01-2023

Changeset: b1ed40a8 Author: Thomas Stuefe <stuefe@openjdk.org> Date: 2022-09-19 05:38:09 +0000 URL: https://git.openjdk.org/jdk/commit/b1ed40a87ab357d1b51ac5102bba181f21ffa9b6
19-09-2022

A pull request was submitted for review. URL: https://git.openjdk.org/jdk/pull/10236 Date: 2022-09-10 06:15:05 +0000
10-09-2022

Its probably not supported. But users do the strangest things, especially with lots of half-right information floating around in blogs and StackOverflow. But I am more worried about future bugs, since in hotspot, nobody thinks twice about querying signal handler information. E.g. in `PosixSignals::is_sig_ignored(int)`. How easy to slip in such a call into the libjsig hotspot install window. Better to fix it, especially since its easy to fix.
07-09-2022

Thanks. This bug is indeed exotic. I couldn't find any use case of -XX:+AllowUserSignalHandlers in our code repository, and I haven't seen a real use case for it yet. Is it really supported to use both libjsig and +AllowUserSignalHandlers at the same time? I'd expect the application to implement its own signal chaining mechanism if it needs +AllowUserSignalHandlers, instead of using libjsig.
07-09-2022

[~manc] you are right of course, thanks for catching this. I'll correct the bug description.
07-09-2022

Agree with most of the analysis. However, not sure about below: > And any futher non-modifying sigaction calls will return NULL instead of the original - still installed - custom handler. The first sigaction call from set_signal_handler() will lead to: res = call_os_sigaction(sig, act, &oldAct); sact[sig] = oldAct; This will save the custom handler in sact[sig] right? Then why further non-modifying sigaction would return NULL?
07-09-2022