Modified: trunk/Source/_javascript_Core/ChangeLog (211827 => 211828)
--- trunk/Source/_javascript_Core/ChangeLog 2017-02-07 19:51:55 UTC (rev 211827)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-02-07 20:01:35 UTC (rev 211828)
@@ -1,3 +1,56 @@
+2017-02-05 Mark Lam <mark....@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....@gmail.com>
Unreviewed, manual roll out of r211777
Modified: trunk/Source/_javascript_Core/tools/SigillCrashAnalyzer.cpp (211827 => 211828)
--- trunk/Source/_javascript_Core/tools/SigillCrashAnalyzer.cpp 2017-02-07 19:51:55 UTC (rev 211827)
+++ trunk/Source/_javascript_Core/tools/SigillCrashAnalyzer.cpp 2017-02-07 20:01:35 UTC (rev 211828)
@@ -47,8 +47,14 @@
class SigillCrashAnalyzer {
public:
static SigillCrashAnalyzer& instance();
- void analyze(SignalContext&);
+ enum class CrashSource {
+ Unknown,
+ _javascript_Core,
+ Other,
+ };
+ CrashSource analyze(SignalContext&);
+
private:
SigillCrashAnalyzer() { }
void dumpCodeBlock(CodeBlock*, void* machinePC);
@@ -165,15 +171,48 @@
#endif
-struct sigaction oldSigIllAction;
+struct sigaction originalSigIllAction;
-static void handleCrash(int, siginfo_t*, void* uap)
+static void handleCrash(int signalNumber, siginfo_t* info, void* uap)
{
- sigaction(SIGILL, &oldSigIllAction, nullptr);
-
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::_javascript_Core) {
+ // 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, ¤tAction);
+
+ 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 _javascript_Core source. Hence, restore the client handler
+ // and keep going.
+ sigaction(SIGILL, ¤tAction, nullptr);
+ }
+ }
}
static void installCrashHandler()
@@ -183,7 +222,7 @@
action.sa_sigaction = reinterpret_cast<void (*)(int, siginfo_t *, void *)>(handleCrash);
sigfillset(&action.sa_mask);
action.sa_flags = SA_SIGINFO;
- sigaction(SIGILL, &action, &oldSigIllAction);
+ sigaction(SIGILL, &action, &originalSigIllAction);
#else
UNUSED_PARAM(handleCrash);
#endif
@@ -227,8 +266,9 @@
SigillCrashAnalyzer::instance();
}
-void SigillCrashAnalyzer::analyze(SignalContext& context)
+auto SigillCrashAnalyzer::analyze(SignalContext& context) -> CrashSource
{
+ CrashSource crashSource = CrashSource::Unknown;
log("BEGIN SIGILL analysis");
[&] () {
@@ -257,9 +297,11 @@
}
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::_javascript_Core;
#if CPU(ARM64)
size_t pcAsSize = reinterpret_cast<size_t>(pc);
@@ -294,6 +336,7 @@
} ();
log("END SIGILL analysis");
+ return crashSource;
}
void SigillCrashAnalyzer::dumpCodeBlock(CodeBlock* codeBlock, void* machinePC)
