Closed Bug 730806 Opened 9 years ago Closed 9 years ago

Crash [@ js::mjit::Compiler::scanInlineCalls]

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
Tracking Status
firefox11 --- unaffected
firefox12 + fixed
firefox13 + fixed
firefox-esr10 --- unaffected

People

(Reporter: decoder, Assigned: bhackett1024)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [sg:critical] js-triage-needed)

Crash Data

Attachments

(2 files, 1 obsolete file)

The attached test crashes on mozilla-central revision 2dc40eb83023 (options -m -n -a). The test is very fragile, I even had whitespace sensitivity in there :( If you cannot reproduce the crash, check in Valgrind.

In Valgrind, the test doesn't crash but I see errors like this:

==13954== Invalid read of size 4
==13954==    at 0x82CB7F2: js::mjit::Compiler::scanInlineCalls(unsigned int, unsigned int) (Compiler.cpp:239)
==13954==    by 0x82CC42C: js::mjit::Compiler::performCompilation() (Compiler.cpp:537)
==13954==    by 0x82CB403: js::mjit::Compiler::compile() (Compiler.cpp:151)
==13954==    by 0x82CE2FA: js::mjit::CanMethodJIT(JSContext*, JSScript*, unsigned char*, bool, js::mjit::CompileRequest) (Compiler.cpp:986)
==13954==    by 0x813E31A: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:1705)
==13954==    by 0x82C6838: js::mjit::EnterMethodJIT(JSContext*, js::StackFrame*, void*, JS::Value*, bool) (MethodJIT.cpp:1080)
==13954==    by 0x82C69E5: CheckStackAndEnterMethodJIT(JSContext*, js::StackFrame*, void*, bool) (MethodJIT.cpp:1112)
==13954==    by 0x82C6AA6: js::mjit::JaegerShot(JSContext*, bool) (MethodJIT.cpp:1124)
==13954==    by 0x8139FCD: js::RunScript(JSContext*, JSScript*, js::StackFrame*) (jsinterp.cpp:451)
==13954==    by 0x813AAAF: js::ExecuteKernel(JSContext*, JSScript*, JSObject&, JS::Value const&, js::ExecuteType, js::StackFrame*, JS::Value*) (jsinterp.cpp:657)
==13954==    by 0x813ACDA: js::Execute(JSContext*, JSScript*, JSObject&, JS::Value*) (jsinterp.cpp:698)
==13954==    by 0x8084B1A: JS_ExecuteScript (jsapi.cpp:5293)
==13954==  Address 0xbbdbe1c is 60 bytes inside a block of size 72 free'd
==13954==    at 0x48D8C02: free (vg_replace_malloc.c:366)
==13954==    by 0x804A99E: js_free (Utility.h:152)
==13954==    by 0x8058DFD: js::Foreground::free_(void*) (Utility.h:566)
==13954==    by 0x8062850: JSRuntime::free_(void*) (jscntxt.h:640)
==13954==    by 0x80628BB: JSContext::free_(void*) (jscntxt.h:1126)
==13954==    by 0x82C7557: js::mjit::ReleaseScriptCode(JSContext*, JSScript*, bool) (MethodJIT.cpp:1453)
==13954==    by 0x80C39B6: js::mjit::ReleaseScriptCode(JSContext*, JSScript*) (MethodJIT.h:904)
==13954==    by 0x80C14D6: JSCompartment::discardJitCode(JSContext*) (jscompartment.cpp:476)
==13954==    by 0x80C1755: JSCompartment::sweep(JSContext*, bool) (jscompartment.cpp:520)
==13954==    by 0x80F79C5: SweepPhase(JSContext*, js::JSGCInvocationKind) (jsgc.cpp:3196)
==13954==    by 0x80F7FFD: MarkAndSweep(JSContext*, js::JSGCInvocationKind) (jsgc.cpp:3288)
==13954==    by 0x80F8DEF: GCCycle(JSContext*, JSCompartment*, long long, js::JSGCInvocationKind) (jsgc.cpp:3636)


This could be related to IncrementalGC, but not entirely sure.
worst-case guess that we might be reading and then compiling garbage, possibly attacker-supplied garbage.
Whiteboard: js-triage-needed → [sg:critical]js-triage-needed
(In reply to Christian Holler (:decoder) from comment #0)
> This could be related to IncrementalGC, but not entirely sure.

Does that mean it doesn't happen in older builds? Which ones have been tested?
(In reply to Daniel Veditz [:dveditz] from comment #2)
> Does that mean it doesn't happen in older builds? Which ones have been
> tested?

Tests like this are fragile in general and not portable across builds.
Whiteboard: [sg:critical]js-triage-needed → [sg:critical] js-triage-needed
Brian, can you take a look at this one? It looks like it's a use-after-free bug in chunked compilation. The error in the trace above refers to the outerChunk.begin access here:

    if (index == CrossScriptSSA::OUTER_FRAME) {
        nextOffset = outerChunk.begin;
        lastOffset = outerChunk.end;
    }
Attached patch patch (obsolete) — Splinter Review
Yeah, the compiler needs to make sure the script's JIT hasn't been freed from under it when accessing that JIT (it will end up aborting compilation later since rt->gcNumber has changed).
Assignee: general → bhackett1024
Attachment #602604 - Flags: review?(dvander)
Missed some uses of outerChunk in Compiler.h (see bug 732776).
Attachment #602604 - Attachment is obsolete: true
Attachment #602853 - Flags: review?(dvander)
Attachment #602604 - Flags: review?(dvander)
Blocks: 732776
No longer blocks: 732776
Blocks: 732791
Attachment #602853 - Flags: review?(dvander) → review+
Thanks for the fast fix!
Actually, pasted that here accidentally.  Keep confusing this with bug 732776 for some reason.

https://hg.mozilla.org/integration/mozilla-inbound/rev/186ef1a79560
Comment on attachment 602853 [details] [diff] [review]
more thorough fix

[Approval Request Comment]
User impact if declined: Potential use-after-free dependent on GC timing.
Risk to taking this patch (and alternatives if risky): Very low, should have no observable effect on behavior.
Attachment #602853 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/186ef1a79560
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 602853 [details] [diff] [review]
more thorough fix

[Triage Comment]
Low risk sg:crit fix. Approved for Aurora 12.
Attachment #602853 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Fixed in 12 and 13, assuming we need this change on the ESR branch as well.
This fix isn't necessary for ESR, it is a regression from bug 706914 and did not land until 12.
Duplicate of this bug: 732791
Status: RESOLVED → VERIFIED
Group: core-security
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.