Changeset 211828 in webkit

Timestamp:

Feb 7, 2017, 12:01:35 PM (8 years ago)

Author:

mark.lam@apple.com

Message:

The SigillCrashAnalyzer should play nicer with client code that may install its own SIGILL handler.
​https://bugs.webkit.org/show_bug.cgi?id=167858

Reviewed by Michael Saboff.

Here are the scenarios that may come up:

1. Client code did not install a SIGILL handler.
   • In this case, once we're done analyzing the SIGILL, we can just restore the default handler and return to let the OS do the default action i.e. capture a core dump.

2. Client code installed a SIGILL handler before JSC does.
   • In this case, we will see a non-null handler returned as the old signal handler when we install ours.
   • In our signal handler, after doing our crash analysis, we should invoke the client handler to let it do its work.
   • Our analyzer can also tell us if the SIGILL source is from JSC code in general (right now, this would just mean JIT code).
   • If the SIGILL source is not from JSC, we'll just let the client handler decided how to proceed. We assume that the client handler will do the right thing (which is how the old behavior is before the SigillCrashAnalyzer was introduced).
   • If the SIGILL source is from JSC, then we know the SIGILL is an unrecoverable condition. Hence, after we have given the client handler a chance to run, we should restore the default handler and let the OS capture a core dump. This intentionally overrides whatever signal settings the client handler may have set.

3. Client code installed a SIGILL handler after JSC does.
   • In this case, we are dependent on the client handler to call our handler after it does its work. This is compatible with the old behavior before SigillCrashAnalyzer was introduced.
   • In our signal handler, if we determine that the SIGILL source is from JSC code, then the SIGILL is not recoverable. We should then restore the default handler and get a core dump.
   • If the SIGILL source is not from JSC, we check to see if there's a client handler installed after us.
   • If we detect a client handler installed after us, we defer judgement on what to do to the client handler. Since the client handler did not uninstall itself, it must have considered itself to have recovered from the SIGILL. We'll trust the client handler and take no restore action of our own (which is compatible with old code behavior).
   • If we detect no client handler and we have no previous handler, then we should restore the default handler and get a core dump.

• tools/SigillCrashAnalyzer.cpp:

(JSC::handleCrash):
(JSC::installCrashHandler):
(JSC::SigillCrashAnalyzer::analyze): Deleted.

Location:

trunk/Source/JavaScriptCore

Files:

• ChangeLog (modified) (1 diff)
• tools/SigillCrashAnalyzer.cpp (modified) (6 diffs)

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+2017-02-05  Mark Lam  <mark.lam@apple.com>
+
+        The SigillCrashAnalyzer should play nicer with client code that may install its own SIGILL handler.
+        https://bugs.webkit.org/show_bug.cgi?id=167858
+
+        Reviewed by Michael Saboff.
+
+        Here are the scenarios that may come up:
+
+        1. Client code did not install a SIGILL handler.
+           - In this case, once we're done analyzing the SIGILL, we can just restore the
+             default handler and return to let the OS do the default action i.e. capture
+             a core dump.
+
+        2. Client code installed a SIGILL handler before JSC does.
+           - In this case, we will see a non-null handler returned as the old signal
+             handler when we install ours.
+           - In our signal handler, after doing our crash analysis, we should invoke the
+             client handler to let it do its work.
+           - Our analyzer can also tell us if the SIGILL source is from JSC code in
+             general (right now, this would just mean JIT code).
+           - If the SIGILL source is not from JSC, we'll just let the client handler
+             decided how to proceed.  We assume that the client handler will do the right
+             thing (which is how the old behavior is before the SigillCrashAnalyzer was
+             introduced).
+           - If the SIGILL source is from JSC, then we know the SIGILL is an unrecoverable
+             condition.  Hence, after we have given the client handler a chance to run,
+             we should restore the default handler and let the OS capture a core dump.
+             This intentionally overrides whatever signal settings the client handler may
+             have set.
+
+        3. Client code installed a SIGILL handler after JSC does.
+           - In this case, we are dependent on the client handler to call our handler
+             after it does its work.  This is compatible with the old behavior before
+             SigillCrashAnalyzer was introduced.
+           - In our signal handler, if we determine that the SIGILL source is from JSC
+             code, then the SIGILL is not recoverable.  We should then restore the
+             default handler and get a core dump.
+           - If the SIGILL source is not from JSC, we check to see if there's a client
+             handler installed after us.
+           - If we detect a client handler installed after us, we defer judgement on what
+             to do to the client handler.  Since the client handler did not uninstall
+             itself, it must have considered itself to have recovered from the SIGILL.
+             We'll trust the client handler and take no restore action of our own (which
+             is compatible with old code behavior).
+           - If we detect no client handler and we have no previous handler, then we
+             should restore the default handler and get a core dump.
+
+        * tools/SigillCrashAnalyzer.cpp:
+        (JSC::handleCrash):
+        (JSC::installCrashHandler):
+        (JSC::SigillCrashAnalyzer::analyze): Deleted.
+
 2017-02-07  Yusuke Suzuki  <utatane.tea@gmail.com>
 

trunk/Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp

@@ -48 +48 @@
 public:
     static SigillCrashAnalyzer& instance();
-    void analyze(SignalContext&);
+
+    enum class CrashSource {
+        Unknown,
+        JavaScriptCore,
+        Other,
+    };
+    CrashSource analyze(SignalContext&);
 
 private:
@@ -166 +172 @@
 #endif
 
-struct sigaction oldSigIllAction;
-
-static void handleCrash(int, siginfo_t*, void* uap)
-{
-    sigaction(SIGILL, &oldSigIllAction, nullptr);
-
+struct sigaction originalSigIllAction;
+
+static void handleCrash(int signalNumber, siginfo_t* info, void* uap)
+{
     SignalContext context(static_cast<ucontext_t*>(uap)->uc_mcontext);
     SigillCrashAnalyzer& analyzer = SigillCrashAnalyzer::instance();
-    analyzer.analyze(context);
+    auto crashSource = analyzer.analyze(context);
+
+    auto originalAction = originalSigIllAction.sa_sigaction;
+    if (originalAction) {
+        // It is always safe to just invoke the original handler using the sa_sigaction form
+        // without checking for the SA_SIGINFO flag. If the original handler is of the
+        // sa_handler form, it will just ignore the 2nd and 3rd arguments since sa_handler is a
+        // subset of sa_sigaction. This is what the man pages says the OS does anyway.
+        originalAction(signalNumber, info, uap);
+    }
+
+    if (crashSource == SigillCrashAnalyzer::CrashSource::JavaScriptCore) {
+        // Restore the default handler so that we can get a core dump.
+        struct sigaction defaultAction;
+        defaultAction.sa_handler = SIG_DFL;
+        sigfillset(&defaultAction.sa_mask);
+        defaultAction.sa_flags = 0;
+        sigaction(SIGILL, &defaultAction, nullptr);
+    } else if (!originalAction) {
+        // Pre-emptively restore the default handler but we may roll it back below.
+        struct sigaction currentAction;
+        struct sigaction defaultAction;
+        defaultAction.sa_handler = SIG_DFL;
+        sigfillset(&defaultAction.sa_mask);
+        defaultAction.sa_flags = 0;
+        sigaction(SIGILL, &defaultAction, &currentAction);
+
+        if (currentAction.sa_sigaction != handleCrash) {
+            // This means that there's a client handler installed after us. This also means
+            // that the client handler thinks it was able to recover from the SIGILL, and
+            // did not uninstall itself. We can't argue with this because the crash isn't
+            // known to be from a JavaScriptCore source. Hence, restore the client handler
+            // and keep going.
+            sigaction(SIGILL, &currentAction, nullptr);
+        }
+    }
 }
 
@@ -184 +223 @@
     sigfillset(&action.sa_mask);
     action.sa_flags = SA_SIGINFO;
-    sigaction(SIGILL, &action, &oldSigIllAction);
+    sigaction(SIGILL, &action, &originalSigIllAction);
 #else
     UNUSED_PARAM(handleCrash);
@@ -228 +267 @@
 }
 
-void SigillCrashAnalyzer::analyze(SignalContext& context)
-{
+auto SigillCrashAnalyzer::analyze(SignalContext& context) -> CrashSource
+{
+    CrashSource crashSource = CrashSource::Unknown;
     log("BEGIN SIGILL analysis");
 
@@ -258 +298 @@
         if (!isInJITMemory.value()) {
             log("pc %p is NOT in valid JIT executable memory", pc);
+            crashSource = CrashSource::Other;
             return;
         }
         log("pc %p is in valid JIT executable memory", pc);
+        crashSource = CrashSource::JavaScriptCore;
 
 #if CPU(ARM64)
@@ -295 +337 @@
 
     log("END SIGILL analysis");
+    return crashSource;
 }