<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[211828] trunk/Source/JavaScriptCore</title>
</head>
<body>
<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; }
#msg dl a { font-weight: bold}
#msg dl a:link { color:#fc3; }
#msg dl a:active { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/211828">211828</a></dd>
<dt>Author</dt> <dd>mark.lam@apple.com</dd>
<dt>Date</dt> <dd>2017-02-07 12:01:35 -0800 (Tue, 07 Feb 2017)</dd>
</dl>
<h3>Log Message</h3>
<pre>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.</pre>
<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceJavaScriptCoreChangeLog">trunk/Source/JavaScriptCore/ChangeLog</a></li>
<li><a href="#trunkSourceJavaScriptCoretoolsSigillCrashAnalyzercpp">trunk/Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp</a></li>
</ul>
</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceJavaScriptCoreChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/ChangeLog (211827 => 211828)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/ChangeLog 2017-02-07 19:51:55 UTC (rev 211827)
+++ trunk/Source/JavaScriptCore/ChangeLog 2017-02-07 20:01:35 UTC (rev 211828)
</span><span class="lines">@@ -1,3 +1,56 @@
</span><ins>+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.
+
</ins><span class="cx"> 2017-02-07 Yusuke Suzuki <utatane.tea@gmail.com>
</span><span class="cx">
</span><span class="cx"> Unreviewed, manual roll out of r211777
</span></span></pre></div>
<a id="trunkSourceJavaScriptCoretoolsSigillCrashAnalyzercpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp (211827 => 211828)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp 2017-02-07 19:51:55 UTC (rev 211827)
+++ trunk/Source/JavaScriptCore/tools/SigillCrashAnalyzer.cpp 2017-02-07 20:01:35 UTC (rev 211828)
</span><span class="lines">@@ -47,8 +47,14 @@
</span><span class="cx"> class SigillCrashAnalyzer {
</span><span class="cx"> public:
</span><span class="cx"> static SigillCrashAnalyzer& instance();
</span><del>- void analyze(SignalContext&);
</del><span class="cx">
</span><ins>+ enum class CrashSource {
+ Unknown,
+ JavaScriptCore,
+ Other,
+ };
+ CrashSource analyze(SignalContext&);
+
</ins><span class="cx"> private:
</span><span class="cx"> SigillCrashAnalyzer() { }
</span><span class="cx"> void dumpCodeBlock(CodeBlock*, void* machinePC);
</span><span class="lines">@@ -165,15 +171,48 @@
</span><span class="cx">
</span><span class="cx"> #endif
</span><span class="cx">
</span><del>-struct sigaction oldSigIllAction;
</del><ins>+struct sigaction originalSigIllAction;
</ins><span class="cx">
</span><del>-static void handleCrash(int, siginfo_t*, void* uap)
</del><ins>+static void handleCrash(int signalNumber, siginfo_t* info, void* uap)
</ins><span class="cx"> {
</span><del>- sigaction(SIGILL, &oldSigIllAction, nullptr);
-
</del><span class="cx"> SignalContext context(static_cast<ucontext_t*>(uap)->uc_mcontext);
</span><span class="cx"> SigillCrashAnalyzer& analyzer = SigillCrashAnalyzer::instance();
</span><del>- analyzer.analyze(context);
</del><ins>+ 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);
+ }
+ }
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> static void installCrashHandler()
</span><span class="lines">@@ -183,7 +222,7 @@
</span><span class="cx"> action.sa_sigaction = reinterpret_cast<void (*)(int, siginfo_t *, void *)>(handleCrash);
</span><span class="cx"> sigfillset(&action.sa_mask);
</span><span class="cx"> action.sa_flags = SA_SIGINFO;
</span><del>- sigaction(SIGILL, &action, &oldSigIllAction);
</del><ins>+ sigaction(SIGILL, &action, &originalSigIllAction);
</ins><span class="cx"> #else
</span><span class="cx"> UNUSED_PARAM(handleCrash);
</span><span class="cx"> #endif
</span><span class="lines">@@ -227,8 +266,9 @@
</span><span class="cx"> SigillCrashAnalyzer::instance();
</span><span class="cx"> }
</span><span class="cx">
</span><del>-void SigillCrashAnalyzer::analyze(SignalContext& context)
</del><ins>+auto SigillCrashAnalyzer::analyze(SignalContext& context) -> CrashSource
</ins><span class="cx"> {
</span><ins>+ CrashSource crashSource = CrashSource::Unknown;
</ins><span class="cx"> log("BEGIN SIGILL analysis");
</span><span class="cx">
</span><span class="cx"> [&] () {
</span><span class="lines">@@ -257,9 +297,11 @@
</span><span class="cx"> }
</span><span class="cx"> if (!isInJITMemory.value()) {
</span><span class="cx"> log("pc %p is NOT in valid JIT executable memory", pc);
</span><ins>+ crashSource = CrashSource::Other;
</ins><span class="cx"> return;
</span><span class="cx"> }
</span><span class="cx"> log("pc %p is in valid JIT executable memory", pc);
</span><ins>+ crashSource = CrashSource::JavaScriptCore;
</ins><span class="cx">
</span><span class="cx"> #if CPU(ARM64)
</span><span class="cx"> size_t pcAsSize = reinterpret_cast<size_t>(pc);
</span><span class="lines">@@ -294,6 +336,7 @@
</span><span class="cx"> } ();
</span><span class="cx">
</span><span class="cx"> log("END SIGILL analysis");
</span><ins>+ return crashSource;
</ins><span class="cx"> }
</span><span class="cx">
</span><span class="cx"> void SigillCrashAnalyzer::dumpCodeBlock(CodeBlock* codeBlock, void* machinePC)
</span></span></pre>
</div>
</div>
</body>
</html>