Last Comment Bug 730806 - Crash [@ js::mjit::Compiler::scanInlineCalls]
: Crash [@ js::mjit::Compiler::scanInlineCalls]
[sg:critical] js-triage-needed
: crash, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
-- critical (vote)
: ---
Assigned To: Brian Hackett (:bhackett)
: Jason Orendorff [:jorendorff]
: 732791 (view as bug list)
Depends on:
Blocks: langfuzz 706914 732791
  Show dependency treegraph
Reported: 2012-02-27 07:18 PST by Christian Holler (:decoder)
Modified: 2013-01-19 13:57 PST (History)
7 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Test case for shell (run with -n -m -a) (1.24 KB, application/javascript)
2012-02-27 07:18 PST, Christian Holler (:decoder)
no flags Details
patch (882 bytes, patch)
2012-03-03 05:53 PST, Brian Hackett (:bhackett)
no flags Details | Diff | Splinter Review
more thorough fix (3.59 KB, patch)
2012-03-05 05:14 PST, Brian Hackett (:bhackett)
dvander: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2012-02-27 07:18:36 PST
Created attachment 600901 [details]
Test case for shell (run with -n -m -a)

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.
Comment 1 User image Daniel Veditz [:dveditz] 2012-02-29 16:16:19 PST
worst-case guess that we might be reading and then compiling garbage, possibly attacker-supplied garbage.
Comment 2 User image Daniel Veditz [:dveditz] 2012-02-29 16:17:12 PST
(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?
Comment 3 User image Christian Holler (:decoder) 2012-02-29 17:03:12 PST
(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.
Comment 4 User image David Mandelin [:dmandelin] 2012-03-02 11:41:12 PST
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;
Comment 5 User image Brian Hackett (:bhackett) 2012-03-03 05:53:05 PST
Created attachment 602604 [details] [diff] [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).
Comment 6 User image Brian Hackett (:bhackett) 2012-03-05 05:14:08 PST
Created attachment 602853 [details] [diff] [review]
more thorough fix

Missed some uses of outerChunk in Compiler.h (see bug 732776).
Comment 7 User image Brian Hackett (:bhackett) 2012-03-05 12:46:04 PST
Comment 8 User image David Mandelin [:dmandelin] 2012-03-05 12:56:23 PST
Thanks for the fast fix!
Comment 9 User image Brian Hackett (:bhackett) 2012-03-05 13:26:16 PST
Actually, pasted that here accidentally.  Keep confusing this with bug 732776 for some reason.
Comment 10 User image Brian Hackett (:bhackett) 2012-03-05 13:40:45 PST
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.
Comment 11 User image Brian Hackett (:bhackett) 2012-03-06 11:10:46 PST
Comment 12 User image Alex Keybl [:akeybl] 2012-03-09 12:00:31 PST
Comment on attachment 602853 [details] [diff] [review]
more thorough fix

[Triage Comment]
Low risk sg:crit fix. Approved for Aurora 12.
Comment 13 User image Brian Hackett (:bhackett) 2012-03-10 06:33:35 PST
Comment 14 User image Daniel Veditz [:dveditz] 2012-03-15 13:44:22 PDT
Fixed in 12 and 13, assuming we need this change on the ESR branch as well.
Comment 15 User image Brian Hackett (:bhackett) 2012-03-15 13:52:39 PDT
This fix isn't necessary for ESR, it is a regression from bug 706914 and did not land until 12.
Comment 16 User image Daniel Veditz [:dveditz] 2012-03-15 16:36:23 PDT
Thanks, marking older releases unaffected.
Comment 17 User image Christian Holler (:decoder) 2012-03-23 14:54:20 PDT
*** Bug 732791 has been marked as a duplicate of this bug. ***
Comment 18 User image Christian Holler (:decoder) 2013-01-19 13:57:50 PST
Automatically extracted testcase for this bug was committed:

Note You need to log in before you can comment on or make changes to this bug.