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

[tstuefe]

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

---

Tests: Ran the runtime/jsig and the runtime/Thread tests manually.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

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

Reviewers

• Man Cao (@caoman - Committer)
• David Holmes (@dholmes-ora - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10236/head:pull/10236
$ git checkout pull/10236

Update a local copy of the PR:
$ git checkout pull/10236
$ git pull https://git.openjdk.org/jdk pull/10236/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10236

View PR using the GUI difftool:
$ git pr show -t 10236

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10236.diff

[bridgekeeper]

👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@tstuefe The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[mlbridge]

Webrevs

• 00: Full (68b9c74e)

[caoman]

LGTM. Not an official OpenJDK Reviewer though.

[AlanBateman]

/label add hotspot-runtime
/label remove core-libs

[openjdk]

@AlanBateman
The hotspot-runtime label was successfully added.

[openjdk]

@AlanBateman
The core-libs label was successfully removed.

[tstuefe]

Friendly ping.

[dholmes-ora]

This seems quite reasonable.

Thanks.

[openjdk]

@tstuefe This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8293466: libjsig should ignore non-modifying sigaction calls

Reviewed-by: manc, dholmes

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 70 new commits pushed to the master branch:

• b6ff8fa: 8292073: NMT: remove unused constructor parameter from MallocHeader
• cfd44bb: 8293218: serviceability/tmtools/jstat/GcNewTest.java fails with "Error in the percent calculation"
• 01e7b88: 8290917: x86: Memory-operand arithmetic instructions have too low costs
• 4b8399b: 8293251: Use stringStream::base() instead of as_string() when applicable
• a8f0f57: 8278165: Clarify that ZipInputStream does not access the CEN fields for a ZipEntry
• 746f5f5: 8293816: CI: ciBytecodeStream::get_klass() is not consistent
• 4b297c1: 8293892: Add links to JVMS 19 and 20 from ClassFileFormatVersion enum constants
• dfb9c06: 8293535: jdk/javadoc/doclet/testJavaFX/TestJavaFxMode.java fail with jfx
• f42caef: 8293550: Optionally add get-task-allow entitlement to macos binaries
• 5feca68: 8293840: RISC-V: Remove cbuf parameter from far_call/far_jump/trampoline_call
• ... and 60 more: https://git.openjdk.org/jdk/compare/68da02c7b536799ccca49e889c22f3e9a2691fb7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[tstuefe]

This seems quite reasonable.

Thanks.

Thank you David!

[tstuefe]

/integrate

[openjdk]

Going to push as commit b1ed40a.
Since your change was applied there have been 70 commits pushed to the master branch:

• b6ff8fa: 8292073: NMT: remove unused constructor parameter from MallocHeader
• cfd44bb: 8293218: serviceability/tmtools/jstat/GcNewTest.java fails with "Error in the percent calculation"
• 01e7b88: 8290917: x86: Memory-operand arithmetic instructions have too low costs
• 4b8399b: 8293251: Use stringStream::base() instead of as_string() when applicable
• a8f0f57: 8278165: Clarify that ZipInputStream does not access the CEN fields for a ZipEntry
• 746f5f5: 8293816: CI: ciBytecodeStream::get_klass() is not consistent
• 4b297c1: 8293892: Add links to JVMS 19 and 20 from ClassFileFormatVersion enum constants
• dfb9c06: 8293535: jdk/javadoc/doclet/testJavaFX/TestJavaFxMode.java fail with jfx
• f42caef: 8293550: Optionally add get-task-allow entitlement to macos binaries
• 5feca68: 8293840: RISC-V: Remove cbuf parameter from far_call/far_jump/trampoline_call
• ... and 60 more: https://git.openjdk.org/jdk/compare/68da02c7b536799ccca49e889c22f3e9a2691fb7...master

Your commit was automatically rebased without conflicts.

[openjdk]

@tstuefe Pushed as commit b1ed40a.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.