Closed Bug 952022 Opened 10 years ago Closed 10 years ago

Crash [@ js::AsmJSModule::detachIonCompilation] or Assertion failure: exit.interpCodeOffset_, at jit/AsmJSModule.h

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla29
Tracking Status
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 + wontfix
firefox29 --- fixed
firefox-esr24 29+ fixed
b2g18 --- unaffected
b2g-v1.2 --- fixed
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: gkw, Assigned: luke)

References

Details

(5 keywords, Whiteboard: [jsbugmon:][adv-main29+][adv-esr24.5+])

Crash Data

Attachments

(3 files, 1 obsolete file)

function g(f) {
    for (var j = 0; j < 9; ++j) {
        try {
            f()
        } catch (e) {}
    }
}
function h(code) {
    Function(code)();
}
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
g([]);
h("\
    m = (function(stdlib, foreign) { \
        \"use asm\";\
        var ff=foreign.ff;\
        function f(){\
            ff(0);\
        } \
        return f \
    })(this , { \
        ff: arguments.callee.caller\
    });\
    g(m , []);\
");
h("\
    m = undefined;\
    gc();\
")

asserts js debug shell on m-c changeset 7d120481a6ae with --ion-eager --ion-parallel-compile=off at Assertion failure: exit.interpCodeOffset_, at jit/AsmJSModule.h but sometimes crashes instead, at js::AsmJSModule::detachIonCompilation

s-s because gc seems to be involved.

My configure flags are:

CC="clang -Qunused-arguments" AR=ar CXX="clang++ -Qunused-arguments" sh ./configure --target=x86_64-apple-darwin12.5.0 --enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --with-ccache --enable-threadsafe <other NSPR options>


autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/dbeea0e93b56
user:        Brian Hackett
date:        Mon Dec 16 10:53:02 2013 -0800
summary:     Bug 785905 - Build Ion MIR graph off thread, r=jandem.

Brian, is bug 785905 a likely regressor?
Flags: needinfo?(bhackett1024)
Whiteboard: [jsbugmon:update] → [jsbugmon:]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Attached patch fix-detach-asmjsmodule (obsolete) — Splinter Review
I think the Brian's patch just tickled an existing bug.  The sequence leading to the crash is:
 1. create AsmJSModule
 2. it gets linked to IonScript A of JSScript B
 3. GC calls discardJitCode calls B->setIonScript(null)
 4. AsmJSModule is destroyed.  It sees that B->ion == null, so it doesn't detachDependentAsmJSModule
 5. InvalidateBailout calls A->decref, which now tries to unlink itself from the destroyed AsmJSModule

It seems like the best place to detach is whenever script->ion is repointed, since this is how ~AsmJSModule finds the IonScript.  This removes one manual detach call.  The call in Destroy should stay though, I think, because it is called when the JSScript is finalized and we don't call setIonScript() in that case.
Attachment #8350290 - Flags: review?(hv1989)
Flags: needinfo?(bhackett1024)
Luke, which branches does this bug affect? (along with its severity)
Flags: needinfo?(luke)
FWIW we already run into a similar issues with the list of IonScript backedges threaded through the runtime, and call destroyBackedges (idempotently) both on destroying the IonScript and in InvalidateActivation.
To add to that, I think the only time that script->ion will be repointed without destroying the IonScript (aka the IonScript has references on it) is when there are invalidated frames on the stack.  These are the references added in InvalidateActivation immediately after we destroyBackedges.  Adding a detachDependentAsmJSModules call after the destroyBackedges() in InvalidateActivation fixes this testcase.
(In reply to Brian Hackett (:bhackett) from comment #5)
Ah, good point.  IIUC, then, a better fix would be to factor out both calls into a single IonScript::detach (or something if you have a better name) that gets called in Destroy/InvalidateActivation.  Is that right?
Flags: needinfo?(luke)
Gary: I assume this is on all branches.
Attachment #8350290 - Attachment is obsolete: true
Attachment #8350290 - Flags: review?(hv1989)
Attachment #8350768 - Flags: review?(bhackett1024)
Comment on attachment 8350768 [details] [diff] [review]
fix-detach-asmjsmodule

Review of attachment 8350768 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/Ion.cpp
@@ +1111,5 @@
>  
>  void
> +IonScript::unlinkFromRuntime(FreeOp *fop)
> +{
> +    if (dependentAsmJSModules) {

Add a comment explaining why the modules may need to be detached prematurely?

@@ +2435,5 @@
>  
>          // The writes to the executable buffer below may clobber backedge
>          // jumps, so make sure that those backedges are unlinked from the
>          // runtime and not reclobbered with garbage if an interrupt is
>          // triggered.

This comment could get moved into unlinkFromRuntime, and maybe add a different comment here explaining this call.
Attachment #8350768 - Flags: review?(bhackett1024) → review+
https://hg.mozilla.org/mozilla-central/rev/397788955043
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Thanks Luke!
Can we get a security rating here to determine if this needs to be tracked for landing on ESR branch?
Flags: needinfo?(luke)
Since I can't demonstrate otherwise, I expect the bug is exploitable.  It seems like this bug would be difficult to hit in practice.
Flags: needinfo?(luke)
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Luke, it is possible to have a backport of this fix for ESR24? Thanks
Flags: needinfo?(luke)
[Approval Request Comment]
Fix Landed on Version: 29
Risk to taking this patch (and alternatives if risky): low
Attachment #8389827 - Flags: approval-mozilla-esr24?
Flags: needinfo?(luke)
Dan can you help with a sec rating here?
Flags: needinfo?(dveditz)
This is the kind of bug for which sec-approval was invented, https://wiki.mozilla.org/Security/Bug_Approval_Process

Given the chance we would have wanted this to ship with Firefox 28, or even 27 given we checked in a testcase that shows how to reproduce the bug last December. Or not check in the test until after the fix made it to release.
Flags: needinfo?(dveditz)
Keywords: sec-critical
Assignee: nobody → luke
Attachment #8389827 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
I have no idea what branch that patch was made against, but it wasn't esr24.
Flags: needinfo?(luke)
Oops, I had a rebased patch in my mq, but not qref'd.
Flags: needinfo?(luke)
Flags: in-testsuite+
Whiteboard: [jsbugmon:] → [jsbugmon:][adv-main29+][adv-esr24.5+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.