8285792: Posix signal handler modification checking issues.

Reviewed-by: dholmes, erikj

make/test/JtregNativeHotspot.gmk

@@ -874,7 +874,7 @@ BUILD_HOTSPOT_JTREG_EXECUTABLES_LIBS_exesigtest := -ljvm
 
 ifeq ($(call isTargetOs, windows), true)
     BUILD_HOTSPOT_JTREG_EXECUTABLES_CFLAGS_exeFPRegs := -MT
-    BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c libTestJNI.c libCompleteExit.c
+    BUILD_HOTSPOT_JTREG_EXCLUDE += exesigtest.c libterminatedThread.c libTestJNI.c libCompleteExit.c libTestPsig.c
     BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libatExit := jvm.lib
 else
     BUILD_HOTSPOT_JTREG_LIBRARIES_LIBS_libbootclssearch_agent += -lpthread

src/hotspot/os/posix/signals_posix.cpp

@@ -803,7 +803,7 @@ static void SR_handler(int sig, siginfo_t* siginfo, ucontext_t* context);
 
 // Semantically compare two sigaction structures. Return true if they are referring to
 // the same handler, using the same flags.
-static bool are_handlers_equal(const struct sigaction* sa,
+static bool are_actions_equal(const struct sigaction* sa,
                               const struct sigaction* expected_sa) {
   address this_handler = get_signal_handler(sa);
   address expected_handler = get_signal_handler(expected_sa);
@@ -814,13 +814,14 @@ static bool are_handlers_equal(const struct sigaction* sa,
 }
 
 // If we installed one of our signal handlers for sig, check that the current
-//  setup matches what we originally installed.
+// setup matches what we originally installed.  Return true if signal handler
-static void check_signal_handler(int sig) {
+// is different.  Otherwise, return false;
+static bool check_signal_handler(int sig) {
   char buf[O_BUFLEN];
   bool mismatch = false;
 
   if (!do_check_signal_periodically[sig]) {
-    return;
+    return false;
   }
 
   const struct sigaction* expected_act = vm_handlers.get(sig);
@@ -832,28 +833,28 @@ static void check_signal_handler(int sig) {
   if (os_sigaction == NULL) {
     // only trust the default sigaction, in case it has been interposed
     os_sigaction = (os_sigaction_t)dlsym(RTLD_DEFAULT, "sigaction");
-    if (os_sigaction == NULL) return;
+    if (os_sigaction == NULL) return false;
   }
 
   os_sigaction(sig, (struct sigaction*)NULL, &act);
 
   // Compare both sigaction structures (intelligently; only the members we care about).
-  if (!are_handlers_equal(&act, expected_act)) {
+  // Ignore if the handler is our own crash handler.
+  if (!are_actions_equal(&act, expected_act) &&
+      !(HANDLER_IS(get_signal_handler(&act), VMError::crash_handler_address))) {
     tty->print_cr("Warning: %s handler modified!", os::exception_name(sig, buf, sizeof(buf)));
     // If we had a mismatch:
-    // - print all signal handlers. As part of that printout, details will be printed
-    //   about any modified handlers.
     // - Disable any further checks for this signal - we do not want to flood stdout. Though
     //   depending on which signal had been overwritten, we may die very soon anyway.
-    os::print_signal_handlers(tty, buf, O_BUFLEN);
     do_check_signal_periodically[sig] = false;
-    tty->print_cr("Consider using jsig library.");
     // Running under non-interactive shell, SHUTDOWN2_SIGNAL will be reassigned SIG_IGN
     if (sig == SHUTDOWN2_SIGNAL && !isatty(fileno(stdin))) {
       tty->print_cr("Note: Running in non-interactive shell, %s handler is replaced by shell",
                     os::exception_name(sig, buf, O_BUFLEN));
     }
+    return true;
   }
+  return false;
 }
 
 void* os::user_handler() {
@@ -896,24 +897,35 @@ void os::run_periodic_checks() {
   // generation of hs*.log in the event of a crash, debugging
   // such a case can be very challenging, so we absolutely
   // check the following for a good measure:
-  check_signal_handler(SIGSEGV);
+  bool print_handlers = false;
-  check_signal_handler(SIGILL);
+
-  check_signal_handler(SIGFPE);
+  print_handlers |= check_signal_handler(SIGSEGV);
-  check_signal_handler(SIGBUS);
+  print_handlers |= check_signal_handler(SIGILL);
-  check_signal_handler(SIGPIPE);
+  print_handlers |= check_signal_handler(SIGFPE);
-  check_signal_handler(SIGXFSZ);
+  print_handlers |= check_signal_handler(SIGBUS);
-  PPC64_ONLY(check_signal_handler(SIGTRAP);)
+  print_handlers |= check_signal_handler(SIGPIPE);
+  print_handlers |= check_signal_handler(SIGXFSZ);
+  PPC64_ONLY(print_handlers |= check_signal_handler(SIGTRAP);)
 
   // ReduceSignalUsage allows the user to override these handlers
   // see comments at the very top and jvm_md.h
   if (!ReduceSignalUsage) {
-    check_signal_handler(SHUTDOWN1_SIGNAL);
+    print_handlers |= check_signal_handler(SHUTDOWN1_SIGNAL);
-    check_signal_handler(SHUTDOWN2_SIGNAL);
+    print_handlers |= check_signal_handler(SHUTDOWN2_SIGNAL);
-    check_signal_handler(SHUTDOWN3_SIGNAL);
+    print_handlers |= check_signal_handler(SHUTDOWN3_SIGNAL);
-    check_signal_handler(BREAK_SIGNAL);
+    print_handlers |= check_signal_handler(BREAK_SIGNAL);
   }
 
-  check_signal_handler(PosixSignals::SR_signum);
+  print_handlers |= check_signal_handler(PosixSignals::SR_signum);
+
+  if (print_handlers) {
+    // If we had a mismatch:
+    // - print all signal handlers. As part of that printout, details will be printed
+    //   about any modified handlers.
+    char buf[O_BUFLEN];
+    os::print_signal_handlers(tty, buf, O_BUFLEN);
+    tty->print_cr("Consider using jsig library.");
+  }
 }
 
 // Helper function for PosixSignals::print_siginfo_...():
@@ -1410,7 +1422,7 @@ void PosixSignals::print_signal_handler(outputStream* st, int sig,
   if (expected_act != NULL) {
     const address current_handler = get_signal_handler(&current_act);
     if (!(HANDLER_IS(current_handler, VMError::crash_handler_address))) {
-      if (!are_handlers_equal(&current_act, expected_act)) {
+      if (!are_actions_equal(&current_act, expected_act)) {
         st->print_cr("  *** Handler was modified!");
         st->print   ("  *** Expected: ");
         print_single_signal_handler(st, expected_act, buf, buflen);

test/hotspot/jtreg/runtime/posixSig/TestPosixSig.java

@@ -0,0 +1,76 @@
+/*
+ * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+/*
+ * @test TestPosixSig.java
+ * @bug 8285792
+ * @summary fix issues with signal handler modification checks
+ * @requires os.family != "windows"
+ * @library /test/lib
+ * @run driver TestPosixSig
+ */
+
+import jdk.test.lib.process.ProcessTools;
+import jdk.test.lib.process.OutputAnalyzer;
+
+public class TestPosixSig {
+
+    // Check that a substring occurs exactly once.
+    public static boolean occursOnce(String source, String substring) {
+        int index = source.indexOf(substring);
+        if (index == -1) return false;
+        return (source.indexOf(substring, index + 1) == -1);
+    }
+
+    private static native void changeSigActionFor(int val);
+
+    public static void main(String[] args) throws Throwable {
+        // Get the library path property.
+        String libpath = System.getProperty("java.library.path");
+
+        if (args.length == 0) {
+
+            // Create a new java process for the TestPsig Java/JNI test
+            ProcessBuilder pb = ProcessTools.createJavaProcessBuilder(
+                "-XX:+CheckJNICalls",
+                "-Djava.library.path=" + libpath + ":.",
+                "TestPosixSig", "dummy");
+
+            // Start the process and check the output
+            OutputAnalyzer output = new OutputAnalyzer(pb.start());
+            String outputString = output.getOutput();
+            if (!occursOnce(outputString, "SIGFPE: sig_handler in ") ||
+                !occursOnce(outputString, "SIGILL: sig_handler in ")) {
+                System.out.println("output: " + outputString);
+                throw new RuntimeException("Test failed, bad output.");
+            }
+            output.shouldHaveExitValue(0);
+
+        } else {
+            System.loadLibrary("TestPsig");
+            TestPosixSig.changeSigActionFor(8); // SIGFPE
+            TestPosixSig.changeSigActionFor(4); // SIGILL
+            Thread.sleep(600);
+        }
+    }
+}

test/hotspot/jtreg/runtime/posixSig/libTestPsig.c

@@ -0,0 +1,54 @@
+/*
+ * Copyright (c) 2022, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+#include <stdio.h>
+#include <jni.h>
+#include <signal.h>
+#include <sys/ucontext.h>
+#include <errno.h>
+#include <string.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+void sig_handler(int sig, siginfo_t *info, ucontext_t *context) {
+
+    printf( " HANDLER (1) " );
+}
+
+JNIEXPORT void JNICALL Java_TestPosixSig_changeSigActionFor(JNIEnv *env, jclass klass, jint val) {
+    struct sigaction act;
+    act.sa_handler = (void (*)())sig_handler;
+    sigemptyset(&act.sa_mask);
+    act.sa_flags = 0;
+    int retval = sigaction(val, &act, 0);
+    if (retval != 0) {
+        printf("ERROR: failed to set %d signal handler error=%s\n", val, strerror(errno));
+    }
+}
+
+#ifdef __cplusplus
+}
+#endif
+