Fix signal handling-related races between isolates.

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixSubstrateSegfaultHandler.java

@@ -30,6 +30,7 @@
 import org.graalvm.nativeimage.c.type.VoidPointer;
 import org.graalvm.word.PointerBase;
 
+import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.SubstrateSegfaultHandler;
 import com.oracle.svm.core.Uninterruptible;
 import com.oracle.svm.core.c.function.CEntryPointOptions;
@@ -46,8 +47,11 @@
 import com.oracle.svm.core.posix.headers.Signal.ucontext_t;
 import com.oracle.svm.core.util.VMError;
 
-@AutomaticallyRegisteredImageSingleton(SubstrateSegfaultHandler.class)
+@AutomaticallyRegisteredImageSingleton({SubstrateSegfaultHandler.class, PosixSubstrateSegfaultHandler.class})
 class PosixSubstrateSegfaultHandler extends SubstrateSegfaultHandler {
+    static final CEntryPointLiteral<AdvancedSignalDispatcher> SIGNAL_HANDLER = CEntryPointLiteral.create(PosixSubstrateSegfaultHandler.class,
+                    "dispatch", int.class, siginfo_t.class, ucontext_t.class);
+
     @CEntryPoint(include = CEntryPoint.NotIncludedAutomatically.class, publishAs = Publish.NotPublished)
     @CEntryPointOptions(prologue = NoPrologue.class, epilogue = NoEpilogue.class)
     @RestrictHeapAccess(access = RestrictHeapAccess.Access.NO_ALLOCATION, reason = "Must not allocate in segfault signal handler.")
@@ -58,7 +62,7 @@ private static void dispatch(@SuppressWarnings("unused") int signalNumber, @Supp
         }
 
         if (tryEnterIsolate(uContext)) {
-            dump(sigInfo, uContext);
+            dump(sigInfo, uContext, true);
             throw VMError.shouldNotReachHereAtRuntime();
         }
 
@@ -84,13 +88,10 @@ protected void printSignalInfo(Log log, PointerBase signalInfo) {
         }
     }
 
-    /** The address of the signal handler for signals handled by Java code, above. */
-    private static final CEntryPointLiteral<AdvancedSignalDispatcher> advancedSignalDispatcher = CEntryPointLiteral.create(PosixSubstrateSegfaultHandler.class,
-                    "dispatch", int.class, siginfo_t.class, ucontext_t.class);
-
     @Override
-    protected void installInternal() {
-        PosixUtils.installSignalHandler(Signal.SignalEnum.SIGSEGV, advancedSignalDispatcher.getFunctionPointer(), Signal.SA_NODEFER());
-        PosixUtils.installSignalHandler(Signal.SignalEnum.SIGBUS, advancedSignalDispatcher.getFunctionPointer(), Signal.SA_NODEFER());
+    public void install() {
+        boolean isSignalHandlingAllowed = SubstrateOptions.EnableSignalHandling.getValue();
+        PosixSignalHandlerSupport.installNativeSignalHandler(Signal.SignalEnum.SIGSEGV, SIGNAL_HANDLER.getFunctionPointer(), Signal.SA_NODEFER(), isSignalHandlingAllowed);
+        PosixSignalHandlerSupport.installNativeSignalHandler(Signal.SignalEnum.SIGBUS, SIGNAL_HANDLER.getFunctionPointer(), Signal.SA_NODEFER(), isSignalHandlingAllowed);
     }
 }

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixSubstrateSigprofHandler.java

@@ -92,7 +92,8 @@ private static void dispatch(@SuppressWarnings("unused") int signalNumber, @Supp
 
     @Override
     protected void installSignalHandler() {
-        PosixUtils.installSignalHandlerUnsafe(Signal.SignalEnum.SIGPROF, advancedSignalDispatcher.getFunctionPointer(), Signal.SA_RESTART(), SubstrateOptions.EnableSignalHandling.getValue());
+        PosixSignalHandlerSupport.installNativeSignalHandler(Signal.SignalEnum.SIGPROF, advancedSignalDispatcher.getFunctionPointer(), Signal.SA_RESTART(),
+                        SubstrateOptions.EnableSignalHandling.getValue());
     }
 
     static boolean isSignalHandlerBasedExecutionSamplerEnabled() {

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/PosixUtils.java

@@ -24,15 +24,13 @@
  */
 package com.oracle.svm.core.posix;
 
-import static com.oracle.svm.core.Uninterruptible.CALLED_FROM_UNINTERRUPTIBLE_CODE;
 import static com.oracle.svm.core.posix.headers.Unistd._SC_GETPW_R_SIZE_MAX;
 
 import java.io.FileDescriptor;
 import java.io.IOException;
 
 import org.graalvm.nativeimage.Platform;
 import org.graalvm.nativeimage.StackValue;
-import org.graalvm.nativeimage.c.struct.SizeOf;
 import org.graalvm.nativeimage.c.type.CCharPointer;
 import org.graalvm.nativeimage.c.type.CIntPointer;
 import org.graalvm.nativeimage.c.type.CTypeConversion;
@@ -42,7 +40,6 @@
 import org.graalvm.word.UnsignedWord;
 import org.graalvm.word.WordFactory;
 
-import com.oracle.svm.core.SubstrateOptions;
 import com.oracle.svm.core.SubstrateUtil;
 import com.oracle.svm.core.Uninterruptible;
 import com.oracle.svm.core.annotate.Alias;
@@ -51,7 +48,6 @@
 import com.oracle.svm.core.c.libc.LibCBase;
 import com.oracle.svm.core.graal.stackvalue.UnsafeStackValue;
 import com.oracle.svm.core.headers.LibC;
-import com.oracle.svm.core.jdk.Target_jdk_internal_misc_Signal;
 import com.oracle.svm.core.memory.NullableNativeMemory;
 import com.oracle.svm.core.nmt.NmtCategory;
 import com.oracle.svm.core.posix.headers.Dlfcn;
@@ -60,7 +56,6 @@
 import com.oracle.svm.core.posix.headers.Pwd;
 import com.oracle.svm.core.posix.headers.Pwd.passwd;
 import com.oracle.svm.core.posix.headers.Pwd.passwdPointer;
-import com.oracle.svm.core.posix.headers.Signal;
 import com.oracle.svm.core.posix.headers.Time;
 import com.oracle.svm.core.posix.headers.Unistd;
 import com.oracle.svm.core.posix.headers.Wait;
@@ -70,7 +65,6 @@
 import com.oracle.svm.core.util.VMError;
 
 public class PosixUtils {
-
     static String setLocale(String category, String locale) {
         int intCategory = getCategory(category);
 
@@ -266,77 +260,6 @@ public static int readBytes(int fd, CCharPointer buffer, int bufferLen, int read
         return readBytes;
     }
 
-    /**
-     * Registers a native signal handler. This method ensures that no races can happen with the Java
-     * signal handling. However, there can still be races with native code that registers signals.
-     *
-     * Note that signal handlers must be written in a way that they do NOT destroy the libc
-     * {@code errno} value (i.e., typically, each signal needs to save and restore errno).
-     *
-     * WARNING: methods related to signal handling must not be called from an initialization hook
-     * because the runtime option {@code EnableSignalHandling} is not necessarily initialized yet
-     * when initialization hooks run. So, startup hooks are currently the earliest point where
-     * execution is possible.
-     */
-    public static Signal.SignalDispatcher installSignalHandler(int signum, Signal.SignalDispatcher handler, int flags) {
-        synchronized (Target_jdk_internal_misc_Signal.class) {
-            return installSignalHandlerUnsafe(signum, handler, flags, SubstrateOptions.EnableSignalHandling.getValue());
-        }
-    }
-
-    /** See {@link #installSignalHandler(int, Signal.SignalDispatcher, int)}. */
-    public static void installSignalHandler(Signal.SignalEnum signum, Signal.AdvancedSignalDispatcher handler, int flags) {
-        synchronized (Target_jdk_internal_misc_Signal.class) {
-            installSignalHandlerUnsafe(signum, handler, flags, SubstrateOptions.EnableSignalHandling.getValue());
-        }
-    }
-
-    /**
-     * See {@link #installSignalHandler(int, Signal.SignalDispatcher, int)}, except that this method
-     * does not do any synchronization, so races with the Java signal handling may occur.
-     */
-    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
-    public static Signal.SignalDispatcher installSignalHandlerUnsafe(int signum, Signal.SignalDispatcher handler, int flags, boolean isSignalHandlingAllowed) {
-        int structSigActionSize = SizeOf.get(Signal.sigaction.class);
-        Signal.sigaction act = UnsafeStackValue.get(structSigActionSize);
-        LibC.memset(act, WordFactory.signed(0), WordFactory.unsigned(structSigActionSize));
-        act.sa_flags(flags);
-        act.sa_handler(handler);
-
-        Signal.sigaction old = UnsafeStackValue.get(Signal.sigaction.class);
-
-        int result = sigaction(signum, act, old, isSignalHandlingAllowed);
-        if (result != 0) {
-            return Signal.SIG_ERR();
-        }
-        return old.sa_handler();
-    }
-
-    /** See {@link #installSignalHandlerUnsafe(int, Signal.SignalDispatcher, int, boolean)}. */
-    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
-    public static void installSignalHandlerUnsafe(Signal.SignalEnum signum, Signal.AdvancedSignalDispatcher handler, int flags, boolean isSignalHandlingAllowed) {
-        installSignalHandlerUnsafe(signum.getCValue(), handler, flags, isSignalHandlingAllowed);
-    }
-
-    /** See {@link #installSignalHandlerUnsafe(int, Signal.SignalDispatcher, int, boolean)}. */
-    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
-    public static void installSignalHandlerUnsafe(int signum, Signal.AdvancedSignalDispatcher handler, int flags, boolean isSignalHandlingAllowed) {
-        int structSigActionSize = SizeOf.get(Signal.sigaction.class);
-        Signal.sigaction act = UnsafeStackValue.get(structSigActionSize);
-        LibC.memset(act, WordFactory.signed(0), WordFactory.unsigned(structSigActionSize));
-        act.sa_flags(Signal.SA_SIGINFO() | flags);
-        act.sa_sigaction(handler);
-
-        int result = sigaction(signum, act, WordFactory.nullPointer(), isSignalHandlingAllowed);
-        PosixUtils.checkStatusIs0(result, "sigaction failed in installSignalHandler().");
-    }
-
-    @Uninterruptible(reason = CALLED_FROM_UNINTERRUPTIBLE_CODE, mayBeInlined = true)
-    private static int sigaction(int signum, Signal.sigaction structSigAction, Signal.sigaction old, boolean isSignalHandlingAllowed) {
-        VMError.guarantee(isSignalHandlingAllowed, "Trying to install a signal handler while signal handling is disabled.");
-        return Signal.sigaction(signum, structSigAction, old);
-    }
-
     // Checkstyle: stop
     @Uninterruptible(reason = "Called from uninterruptible code.", mayBeInlined = true)
     public static int clock_gettime(int clock_id, Time.timespec ts) {

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/headers/CSunMiscSignal.java

@@ -32,11 +32,21 @@
 /** See cSunMiscSignal.c for the C implementation. */
 @CLibrary(value = "libchelper", requireStatic = true)
 public class CSunMiscSignal {
-    /** Open the C signal handler mechanism. */
+    /**
+     * Open the Java signal handler mechanism. Multiple isolates may execute this method in parallel
+     * but only a single isolate may claim ownership.
+     *
+     * @return 0 on success, 1 if the signal handler mechanism was already claimed by another
+     *         isolate, or some other value if an error occurred during initialization.
+     */
     @CFunction("cSunMiscSignal_open")
     public static native int open();
 
-    /** Close the C signal handler mechanism. */
+    /**
+     * Close the Java signal handler mechanism.
+     *
+     * @return 0 on success, or some non-zero value if an error occurred.
+     */
     @CFunction(value = "cSunMiscSignal_close", transition = NO_TRANSITION)
     public static native int close();
 

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/headers/Signal.java

@@ -49,7 +49,7 @@
 import com.oracle.svm.core.SubstrateSegfaultHandler;
 import com.oracle.svm.core.SubstrateUtil;
 import com.oracle.svm.core.Uninterruptible;
-import com.oracle.svm.core.posix.PosixUtils;
+import com.oracle.svm.core.posix.PosixSignalHandlerSupport;
 
 // Checkstyle: stop
 
@@ -74,15 +74,6 @@ public class Signal {
     @CConstant
     public static native int SIGEV_SIGNAL();
 
-    @CFunction
-    public static native int sigprocmask(int how, sigset_tPointer set, sigset_tPointer oldset);
-
-    @CFunction
-    public static native int sigemptyset(sigset_tPointer set);
-
-    @CFunction
-    public static native int sigaddset(sigset_tPointer set, int signum);
-
     @CPointerTo(nameOfCType = "sigset_t")
     public interface sigset_tPointer extends PointerBase {
     }
@@ -199,7 +190,7 @@ public interface sigevent extends PointerBase {
         void sigev_signo(int value);
     }
 
-    /** Don't call this function directly, use {@link PosixUtils#sigaction} instead. */
+    /** Don't call this function directly, use {@link PosixSignalHandlerSupport} instead. */
     @CFunction(transition = NO_TRANSITION)
     public static native int sigaction(int signum, sigaction act, sigaction oldact);
 
@@ -495,5 +486,15 @@ public interface AArch64DarwinMContext64 extends PointerBase {
     public static class NoTransitions {
         @CFunction(transition = NO_TRANSITION)
         public static native int kill(int pid, int sig);
+
+        @CFunction(transition = NO_TRANSITION)
+        public static native int sigprocmask(int how, sigset_tPointer set, sigset_tPointer oldset);
+
+        @CFunction(transition = NO_TRANSITION)
+        public static native int sigemptyset(sigset_tPointer set);
+
+        @CFunction(transition = NO_TRANSITION)
+        public static native int sigaddset(sigset_tPointer set, int signum);
+
     }
 }

substratevm/src/com.oracle.svm.core.posix/src/com/oracle/svm/core/posix/jdk/Target_jdk_internal_misc_Signal.java

@@ -33,7 +33,7 @@
 final class Target_jdk_internal_misc_Signal {
     @Substitute
     private static int findSignal0(String signalName) {
-        return PosixSignalHandlerSupport.singleton().findSignal0(signalName);
+        return PosixSignalHandlerSupport.singleton().findSignal(signalName);
     }
 
     @Substitute

substratevm/src/com.oracle.svm.core.windows/src/com/oracle/svm/core/windows/WindowsSignalHandlerSupport.java

@@ -56,7 +56,7 @@ public WindowsSignalHandlerSupport() {
     }
 
     @Override
-    public long installSignalHandler(int sig, long nativeH) {
+    public long installJavaSignalHandler(int sig, long nativeH) {
         assert MonitorSupport.singleton().isLockedByCurrentThread(Target_jdk_internal_misc_Signal.class);
         ensureInitialized();
 

substratevm/src/com.oracle.svm.core.windows/src/com/oracle/svm/core/windows/WindowsSubstrateSegfaultHandler.java

@@ -53,7 +53,7 @@ class WindowsSubstrateSegfaultHandler extends SubstrateSegfaultHandler {
     private static final int EX_EXECUTE = 8;
 
     @Override
-    protected void installInternal() {
+    public void install() {
         /*
          * Normally we would use SEH (Structured Exception Handling) for this. However, in order for
          * SEH to work, the OS must be able to perform stack walking. On x64, this requires the
@@ -90,7 +90,7 @@ private static int handler(ErrHandlingAPI.EXCEPTION_POINTERS exceptionInfo) {
 
         ErrHandlingAPI.CONTEXT context = exceptionInfo.ContextRecord();
         if (tryEnterIsolate(context)) {
-            dump(exceptionInfo, context);
+            dump(exceptionInfo, context, true);
             throw shouldNotReachHere();
         }
 

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/SubstrateSegfaultHandler.java

@@ -69,6 +69,7 @@
 import jdk.graal.compiler.api.replacements.Fold;
 import jdk.graal.compiler.core.common.NumUtil;
 import jdk.graal.compiler.options.Option;
+import jdk.graal.compiler.word.Word;
 
 @AutomaticallyRegisteredFeature
 class SubstrateSegfaultHandlerFeature implements InternalFeature {
@@ -108,15 +109,21 @@ private static Field getStaticFieldWithWellKnownValue() {
 }
 
 final class SubstrateSegfaultHandlerStartupHook implements RuntimeSupport.Hook {
+    private static final CGlobalData<Pointer> SEGFAULT_HANDLER_INSTALLED = CGlobalDataFactory.createWord();
+
     @Override
     public void execute(boolean isFirstIsolate) {
-        if (isFirstIsolate) {
-            Boolean optionValue = SubstrateSegfaultHandler.Options.InstallSegfaultHandler.getValue();
-            if (SubstrateOptions.EnableSignalHandling.getValue() && optionValue != Boolean.FALSE) {
-                ImageSingletons.lookup(SubstrateSegfaultHandler.class).install();
-            }
+        Boolean optionValue = SubstrateSegfaultHandler.Options.InstallSegfaultHandler.getValue();
+        if (SubstrateOptions.EnableSignalHandling.getValue() && optionValue != Boolean.FALSE && isFirst()) {
+            ImageSingletons.lookup(SubstrateSegfaultHandler.class).install();
         }
     }
+
+    private static boolean isFirst() {
+        Word expected = WordFactory.zero();
+        Word actual = SEGFAULT_HANDLER_INSTALLED.get().compareAndSwapWord(0, expected, WordFactory.unsigned(1), LocationIdentity.ANY_LOCATION);
+        return expected == actual;
+    }
 }
 
 public abstract class SubstrateSegfaultHandler {
@@ -130,24 +137,13 @@ public static class Options {
     static long offsetOfStaticFieldWithWellKnownValue;
     @SuppressWarnings("unused") private static long staticFieldWithWellKnownValue = MARKER_VALUE;
 
-    private boolean installed;
-
     @Fold
     public static SubstrateSegfaultHandler singleton() {
         return ImageSingletons.lookup(SubstrateSegfaultHandler.class);
     }
 
-    public static boolean isInstalled() {
-        return singleton().installed;
-    }
-
     /** Installs the platform dependent segfault handler. */
-    public void install() {
-        installInternal();
-        installed = true;
-    }
-
-    protected abstract void installInternal();
+    public abstract void install();
 
     protected abstract void printSignalInfo(Log log, PointerBase signalInfo);
 
@@ -255,23 +251,28 @@ public static void enterIsolateAsyncSignalSafe(Isolate isolate) {
         }
     }
 
-    /** Called from the platform dependent segfault handler to print diagnostics. */
+    /** Called in certain embedding use-cases. */
     @Uninterruptible(reason = "Must be uninterruptible until we get immune to safepoints.")
     public static void dump(PointerBase signalInfo, RegisterDumper.Context context) {
+        dump(signalInfo, context, false);
+    }
+
+    @Uninterruptible(reason = "Must be uninterruptible until we get immune to safepoints.")
+    public static void dump(PointerBase signalInfo, RegisterDumper.Context context, boolean inSVMSegfaultHandler) {
         Pointer sp = (Pointer) RegisterDumper.singleton().getSP(context);
         CodePointer ip = (CodePointer) RegisterDumper.singleton().getIP(context);
-        dump(sp, ip, signalInfo, context);
+        dump(sp, ip, signalInfo, context, inSVMSegfaultHandler);
     }
 
     @Uninterruptible(reason = "Must be uninterruptible until we get immune to safepoints.", calleeMustBe = false)
     @RestrictHeapAccess(access = NO_ALLOCATION, reason = "Must not allocate in segfault handler.")
-    public static void dump(Pointer sp, CodePointer ip, PointerBase signalInfo, RegisterDumper.Context context) {
+    private static void dump(Pointer sp, CodePointer ip, PointerBase signalInfo, RegisterDumper.Context context, boolean inSVMSegfaultHandler) {
         SafepointBehavior.preventSafepoints();
         StackOverflowCheck.singleton().disableStackOverflowChecksForFatalError();
-        dumpInterruptibly(sp, ip, signalInfo, context);
+        dumpInterruptibly(sp, ip, signalInfo, context, inSVMSegfaultHandler);
     }
 
-    private static void dumpInterruptibly(Pointer sp, CodePointer ip, PointerBase signalInfo, RegisterDumper.Context context) {
+    private static void dumpInterruptibly(Pointer sp, CodePointer ip, PointerBase signalInfo, RegisterDumper.Context context, boolean inSVMSegfaultHandler) {
         LogHandler logHandler = ImageSingletons.lookup(LogHandler.class);
         Log log = Log.enterFatalContext(logHandler, ip, "[ [ SegfaultHandler caught a segfault. ] ]", null);
         if (log != null) {
@@ -282,7 +283,7 @@ private static void dumpInterruptibly(Pointer sp, CodePointer ip, PointerBase si
             }
 
             boolean printedDiagnostics = SubstrateDiagnostics.printFatalError(log, sp, ip, context, false);
-            if (SubstrateSegfaultHandler.isInstalled() && printedDiagnostics) {
+            if (printedDiagnostics && inSVMSegfaultHandler) {
                 log.string("Segfault detected, aborting process. ") //
                                 .string("Use '-XX:-InstallSegfaultHandler' to disable the segfault handler at run time and create a core dump instead. ") //
                                 .string("Rebuild with '-R:-InstallSegfaultHandler' to disable the handler permanently at build time.") //