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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: bhackett)

Tracking

(Blocks: 1 bug, {crash, regression, testcase})

Trunk
x86
Linux
crash, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox11 unaffected, firefox12+ fixed, firefox13+ fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [sg:critical] js-triage-needed, crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
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?
Keywords: regressionwindow-wanted
(Reporter)

Comment 3

5 years ago
(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.

Updated

5 years ago
status-firefox10: --- → wontfix
status-firefox11: --- → wontfix
status-firefox12: --- → affected
status-firefox13: --- → affected
tracking-firefox10: --- → -
tracking-firefox11: --- → -
tracking-firefox12: --- → +
tracking-firefox13: --- → +
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;
    }
(Assignee)

Comment 5

5 years ago
Created attachment 602604 [details] [diff] [review]
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).
Assignee: general → bhackett1024
Attachment #602604 - Flags: review?(dvander)
(Assignee)

Comment 6

5 years ago
Created attachment 602853 [details] [diff] [review]
more thorough fix

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)
(Assignee)

Updated

5 years ago
Blocks: 732776
(Assignee)

Updated

5 years ago
No longer blocks: 732776
(Assignee)

Updated

5 years ago
Blocks: 732791
Attachment #602853 - Flags: review?(dvander) → review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/54226ef0199e
Thanks for the fast fix!
(Assignee)

Comment 9

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

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

Comment 10

5 years ago
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?
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/mozilla-central/rev/186ef1a79560
Status: NEW → RESOLVED
Last Resolved: 5 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+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/1fd7cde39558
Fixed in 12 and 13, assuming we need this change on the ESR branch as well.
status-firefox-esr10: --- → ?
status-firefox12: affected → fixed
status-firefox13: affected → fixed
tracking-firefox-esr10: --- → ?
(Assignee)

Comment 15

5 years ago
This fix isn't necessary for ESR, it is a regression from bug 706914 and did not land until 12.
Thanks, marking older releases unaffected.
Blocks: 706914
status-firefox-esr10: ? → unaffected
status-firefox10: wontfix → ---
status-firefox11: wontfix → unaffected
tracking-firefox-esr10: ? → ---
tracking-firefox10: - → ---
tracking-firefox11: - → ---
Keywords: regressionwindow-wanted → regression
(Reporter)

Updated

5 years ago
Duplicate of this bug: 732791
(Reporter)

Updated

5 years ago
Status: RESOLVED → VERIFIED
Group: core-security
(Reporter)

Comment 18

4 years ago
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.