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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: 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

[Daniel Veditz [:dveditz]]

worst-case guess that we might be reading and then compiling garbage, possibly attacker-supplied garbage.

Comment 2

[Daniel Veditz [:dveditz]]

(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

[Christian Holler (:decoder)]

(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

[David Mandelin [:dmandelin]]

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

[Brian Hackett [Laid off!]]

Attachment: patch

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

[Brian Hackett [Laid off!]]

Attachment: more thorough fix

Missed some uses of outerChunk in Compiler.h (see bug 732776).

Comment 7

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54226ef0199e

Comment 8

[David Mandelin [:dmandelin]]

Thanks for the fast fix!

Comment 9

[Brian Hackett [Laid off!]]

Actually, pasted that here accidentally.  Keep confusing this with bug 732776 for some reason.

https://hg.mozilla.org/integration/mozilla-inbound/rev/186ef1a79560

Comment 10

[Brian Hackett [Laid off!]]

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

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/mozilla-central/rev/186ef1a79560

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 602853 [details] [diff] [review]
more thorough fix

[Triage Comment]
Low risk sg:crit fix. Approved for Aurora 12.

Comment 13

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1fd7cde39558

Comment 14

[Daniel Veditz [:dveditz]]

Fixed in 12 and 13, assuming we need this change on the ESR branch as well.

Comment 15

[Brian Hackett [Laid off!]]

This fix isn't necessary for ESR, it is a regression from bug 706914 and did not land until 12.

Comment 16

[Daniel Veditz [:dveditz]]

Thanks, marking older releases unaffected.

Comment 18

[Christian Holler (:decoder)]

Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929