Closed Bug 1207571 Opened 4 years ago Closed 4 years ago

Crash [@ js::DispatchValueTyped] or Crash [@ js::gc::TenuredCell::zone] with asm.js

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla45
Tracking Status
firefox41 --- wontfix
firefox42 --- wontfix
firefox43 + verified
firefox44 + verified
firefox45 + verified
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update,reconfirm,ignore][adv-main43+])

Crash Data

Attachments

(2 files, 2 obsolete files)

The following testcase crashes on mozilla-central revision 2235e56c94cf (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2):

var lfcode = new Array();
lfcode.push = loadFile;
lfcode.push('');
lfcode.push('');
lfcode.push('');
lfcode.push('');
lfcode.push('0');
lfcode.push(`
function Function(stdlib)
{
  "use asm";
  var abs = stdlib.Math.abs;
  function f(p)
  {
    p = p|0;
  }
  return f;
}
`);
lfcode.push('');
lfcode.push('2');
lfcode.push('');
lfcode.push('');
lfcode.push('');
lfcode.push('4');
lfcode.push("gczeal(14, 1);");
lfcode.push('2');
lfcode.push('');
function loadFile(lfVarx) {
    try {
        if (lfVarx.substr(-3) != ".js" && lfVarx.length != 1) {
            switch (lfRunTypeId) {
                case 0: evaluate(lfVarx); break;
                case 2: new Function(lfVarx)(); break;
                case 4: eval("(function() { " + lfVarx + " })();"); break;
            }
        } else if (!isNaN(lfVarx)) {
            lfRunTypeId = parseInt(lfVarx);
        }
    } catch (lfVare) {
    }
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
js::DispatchValueTyped<DoMarkingFunctor<JS::Value>, js::GCMarker*&>(DoMarkingFunctor<JS::Value>, JS::Value const&, (decltype ({parm#1}((JSObject*)((decltype(nullptr))0), (Forward<js::GCMarker*&>)({parm#3})))&&)...) (f=..., val=...) at ../../dist/include/js/Value.h:1841
#0  js::DispatchValueTyped<DoMarkingFunctor<JS::Value>, js::GCMarker*&>(DoMarkingFunctor<JS::Value>, JS::Value const&, (decltype ({parm#1}((JSObject*)((decltype(nullptr))0), (Forward<js::GCMarker*&>)({parm#3})))&&)...) (f=..., val=...) at ../../dist/include/js/Value.h:1841
#1  0x00000000006e78f0 in DoMarking<JS::Value> (val=..., gcmarker=0x7ffff693e040) at js/src/gc/Marking.cpp:759
#2  DispatchToTracer<JS::Value> (trc=<optimized out>, thingp=0x7fffffffc890, name=<optimized out>) at js/src/gc/Marking.cpp:610
#3  0x00000000006e7b65 in js::TraceRootRange<JS::Value> (trc=trc@entry=0x7ffff693e040, len=4, vec=<optimized out>, name=0xba15cc "fp argv") at js/src/gc/Marking.cpp:497
#4  0x00000000006126ad in js::InterpreterFrame::markValues (this=this@entry=0x7ffff47ea0c8, trc=trc@entry=0x7ffff693e040, sp=<optimized out>, pc=<optimized out>) at js/src/vm/Stack.cpp:391
#5  0x000000000061622c in MarkInterpreterActivation (act=0x7fffffffc2d0, trc=0x7ffff693e040) at js/src/vm/Stack.cpp:400
#6  js::MarkInterpreterActivations (rt=<optimized out>, trc=trc@entry=0x7ffff693e040) at js/src/vm/Stack.cpp:411
#7  0x00000000006d18b3 in js::gc::GCRuntime::markRuntime (this=this@entry=0x7ffff693c3f0, trc=trc@entry=0x7ffff693e040, traceOrMark=traceOrMark@entry=js::gc::GCRuntime::MarkRuntime) at js/src/gc/RootMarking.cpp:314
#8  0x000000000090ee7f in js::gc::GCRuntime::beginMarkPhase (this=this@entry=0x7ffff693c3f0, reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:3951
#9  0x00000000009345f1 in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff693c3f0, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:5892
#10 0x000000000093529d in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff693c3f0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6166
#11 0x000000000093561c in js::gc::GCRuntime::collect (this=this@entry=0x7ffff693c3f0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6282
#12 0x0000000000935991 in js::gc::GCRuntime::gc (this=0x7ffff693c3f0, gckind=<optimized out>, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6346
#13 0x000000000093708d in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff693c3f0) at js/src/jsgc.cpp:6785
#14 0x00000000005378ef in js::gc::GCRuntime::gcIfNeededPerAllocation (this=this@entry=0x7ffff693c3f0, cx=cx@entry=0x7ffff6907000) at js/src/gc/Allocator.cpp:28
#15 0x0000000000565470 in checkAllocatorState<(js::AllowGC)1> (kind=js::gc::FIRST, cx=0x7ffff6907000, this=0x7ffff693c3f0) at js/src/gc/Allocator.cpp:55
#16 js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff6907000, kind=kind@entry=js::gc::FIRST, nDynamicSlots=0, heap=heap@entry=js::gc::DefaultHeap, clasp=clasp@entry=0x17fc0e0 <JSFunction::class_>) at js/src/gc/Allocator.cpp:121
#17 0x00000000005a5ae7 in JSObject::create (cx=0x7ffff6907000, kind=js::gc::FIRST, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/jsobjinlines.h:329
#18 0x00000000009057c6 in NewObject (cx=cx@entry=0x7ffff6907000, group=group@entry=..., kind=kind@entry=js::gc::FIRST, newKind=newKind@entry=js::GenericObject, initialShapeFlags=initialShapeFlags@entry=0) at js/src/jsobj.cpp:684
#19 0x0000000000906b85 in js::NewObjectWithClassProtoCommon (cxArg=cxArg@entry=0x7ffff6907000, clasp=clasp@entry=0x17fc0e0 <JSFunction::class_>, protoArg=protoArg@entry=..., allocKind=allocKind@entry=js::gc::FIRST, newKind=js::GenericObject) at js/src/jsobj.cpp:812
#20 0x000000000093107d in NewObjectWithClassProto (newKind=<optimized out>, allocKind=js::gc::FIRST, proto=..., clasp=0x17fc0e0 <JSFunction::class_>, cx=0x7ffff6907000) at js/src/jsobjinlines.h:721
#21 NewFunctionClone (proto=..., allocKind=js::gc::FIRST, newKind=(unknown: 4294949072), fun=..., cx=0x7ffff6907000) at js/src/jsfun.cpp:2080
#22 js::CloneFunctionReuseScript (cx=cx@entry=0x7ffff6907000, fun=..., fun@entry=..., parent=parent@entry=..., allocKind=allocKind@entry=js::gc::FIRST, newKind=newKind@entry=js::GenericObject, proto=..., proto@entry=...) at js/src/jsfun.cpp:2115
#23 0x00000000005fe857 in js::CloneFunctionObjectIfNotSingleton (cx=0x7ffff6907000, fun=..., parent=..., proto=proto@entry=..., newKind=newKind@entry=js::GenericObject) at js/src/jsfuninlines.h:90
#24 0x00000000005d73b3 in js::Lambda (cx=<optimized out>, fun=..., parent=...) at js/src/vm/Interpreter.cpp:4278
#25 0x00000000005db9b9 in Interpret (cx=0x7ffff6907000, state=...) at js/src/vm/Interpreter.cpp:3524
#26 0x00000000005e64bd in js::RunScript (cx=cx@entry=0x7ffff6907000, state=...) at js/src/vm/Interpreter.cpp:706
#27 0x00000000005e6d91 in js::Invoke (cx=cx@entry=0x7ffff6907000, args=..., construct=<optimized out>) at js/src/vm/Interpreter.cpp:783
#28 0x00000000005081ea in HandleDynamicLinkFailure (name=..., module=..., args=..., cx=0x7ffff6907000) at js/src/asmjs/AsmJSLink.cpp:886
#29 LinkAsmJS (cx=0x7ffff6907000, argc=<optimized out>, vp=<optimized out>) at js/src/asmjs/AsmJSLink.cpp:1053
#30 0x00007ffff7ff3c9c in ?? ()
#31 0x00007fffffffc918 in ?? ()
#32 0x00007fffffffc850 in ?? ()
#33 0x0000000000000000 in ?? ()
rax	0x4949494949494949	5280832617179597129
rbx	0x7ffff7e7d380	140737352553344
rcx	0xfa70	64112
rdx	0x7ffff6974730	140737330497328
rsi	0x1000000000000	281474976710656
rdi	0x0	0
rbp	0x7ffff693e040	140737330274368
rsp	0x7fffffffb060	140737488334944
r8	0x1	1
r9	0x0	0
r10	0x56029e99	1443012249
r11	0x7	7
r12	0x7ffff693e040	140737330274368
r13	0xfffb800000000000	-1266637395197952
r14	0x4	4
r15	0xfffa7fffffffffff	-1548112371908609
rip	0x6ddb10 <js::DispatchValueTyped<DoMarkingFunctor<JS::Value>, js::GCMarker*&>(DoMarkingFunctor<JS::Value>, JS::Value const&, (decltype ({parm#1}((JSObject*)((decltype(nullptr))0), (Forward<js::GCMarker*&>)({parm#3})))&&)...)+176>
=> 0x6ddb10 <js::DispatchValueTyped<DoMarkingFunctor<JS::Value>, js::GCMarker*&>(DoMarkingFunctor<JS::Value>, JS::Value const&, (decltype ({parm#1}((JSObject*)((decltype(nullptr))0), (Forward<js::GCMarker*&>)({parm#3})))&&)...)+176>:	mov    0x10(%rax),%rax
   0x6ddb14 <js::DispatchValueTyped<DoMarkingFunctor<JS::Value>, js::GCMarker*&>(DoMarkingFunctor<JS::Value>, JS::Value const&, (decltype ({parm#1}((JSObject*)((decltype(nullptr))0), (Forward<js::GCMarker*&>)({parm#3})))&&)...)+180>:	movb   $0x1,0x291(%rax)


Assuming sec-critical because of the crash pattern in register rax and GC being involved. Likely use-after-free.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/02a02fcab9eb
user:        Eric Faust
date:        Wed Jun 03 02:01:14 2015 -0700
summary:     Bug 1141865 - Part 2: Plumb new.target on the stack and make it accessible to JSNatives. (r=jorendorff, r=jandem, r=shu)

This iteration took 159.109 seconds to run.
Needinfo from Eric based on comment 1.
Flags: needinfo?(efaustbmo)
Is this disabled outside of Nightly? It is a regression from what looks like a trunky-only feature, but maybe this affects older branches?
Crash Signature: [@ js::DispatchValueTyped][@ js::gc::TenuredCell::zone] → [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone]
Jan, could you look into this or find somebody to look into it? It is almost a month old. Thanks.
Flags: needinfo?(jdemooij)
I can reproduce on the mentioned changeset in comment 0, not on trunk.

Bisection lead to:

The first good revision is:
changeset:   266953:cd25dbf77e57
user:        Eric Faust <efaustbmo@mozilla.com>
date:        Thu Oct 08 17:01:49 2015 -0700
summary:     Bug 1141863 - Part 1: Make |this| object creation account for new.target. (r=jandem, r=jorendorff)

This patch added a few Rooted, so as it seems related to GC, it could explain how it fixed this crash. Also, it could be provoked by anything, as it could just be about GC timing, etc.
Eric, can you tell whether the issue has been fixed or if it's just been hidden?
Flags: needinfo?(jdemooij)
Flags: needinfo?(efaustbmo)
See previous comment.
Flags: needinfo?(efaustbmo)
Does this still reproduce, Christian?
Flags: needinfo?(choller)
Flags: needinfo?(choller)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,reconfirm]
Crash Signature: [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone] → [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone]
Whiteboard: [jsbugmon:update,reconfirm] → [jsbugmon:update,reconfirm,ignore]
JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision 3cc3b1968524).
Needinfo from bbouvier based on comment 8 :)
Crash Signature: [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone] → [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone]
Flags: needinfo?(benj)
It's crashing in the GC, needinfoing jonco.
Flags: needinfo?(jcoppeard)
Flags: needinfo?(efaustbmo)
Flags: needinfo?(benj)
Couldn't reduce really better:

loadFile('');
loadFile('');
loadFile('');
loadFile('');
loadFile('0');
loadFile(`
function Function(stdlib)
{
  "use asm";
  var abs = stdlib.Math.abs;
  function f()
  { }
  return f;
}
`);
loadFile('');
loadFile('2');
loadFile('');
loadFile('');
loadFile('');
loadFile('4');
loadFile("gczeal(14, 1);");
loadFile('2');
loadFile('');
function loadFile(lfVarx) {
    try {
        if (lfVarx.length == 1) {
            lfRunTypeId = parseInt(lfVarx);
            return;
        }

        switch (lfRunTypeId) {
            case 0: evaluate(lfVarx); break;
            case 2: new Function(lfVarx)(); break;
            case 4: eval("(function() { " + lfVarx + " })();"); break;
        }
    } catch (lfVare) { }
}
Tracking for 43 since this is sec-critical. We could still take a fix on beta.
Reproduced.  I reduced the testcase to the following, run with --baseline-eager:

function Function(stdlib)
{
  "use asm";
  var abs = stdlib.Math.abs;
  function f() { }
  return f;
}

function f() {
    try { 
        new Function('')();
    } catch (e) {
    }
}

f();
gczeal(14, 1);
f();
Looks like we're not marking new.target in native exit frames.
Blocks: new.target
Flags: needinfo?(jcoppeard)
Attached patch bug1207571-new-target (obsolete) — Splinter Review
Patch to mark new.target in native exit frames.

This checks whether thisv is the magic constructing value to tell whether there's a new target argument present.  Jan is this the right way to do this?  I couldn't see how to tell otherwise without adding some more information to the stack frame.
Assignee: nobody → jcoppeard
Attachment #8690034 - Flags: review?(jdemooij)
Also worth noting I couldn't get this to reproduce in any kind of debug build.  I don't know why this should be so.
Comment on attachment 8690034 [details] [diff] [review]
bug1207571-new-target

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

Thanks for fixing this!

I think this should work, but it assumes natives don't touch their thisv slot, which is pretty subtle. I think we should either:

(1) Assert in CallJSNative, after the call, that the thisv slot wasn't modified if we're constructing, with a comment explaining why.

(2) Split NativeExitFrameLayout into 2 different ones for call and construct, with different tokens.

What do you think?
Attachment #8690034 - Flags: review?(jdemooij)
Attached patch bug1207571-new-target v2 (obsolete) — Splinter Review
Ah, we can add a new exit frame token to tell us whether new.target is present!  That seems like the best approach.
Attachment #8690034 - Attachment is obsolete: true
Attachment #8690080 - Flags: review?(jdemooij)
Comment on attachment 8690080 [details] [diff] [review]
bug1207571-new-target v2

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

Beautiful.

::: js/src/jit-test/tests/gc/bug-1207571.js
@@ +1,1 @@
> +function Function(stdlib)

We should probably land the test later if it's s-s.

::: js/src/jit/JitFrames.h
@@ +600,5 @@
> +};
> +
> +template<>
> +inline bool
> +ExitFrameLayout::is<NativeExitFrameLayout>() {

Nit: { on own line.
Attachment #8690080 - Flags: review?(jdemooij) → review+
Patch with testcase removed.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Possible but difficult and requires knowledge of the workings of the JITs.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

Everything back to FF41.

If not all supported branches, which bug introduced the flaw?

Bug 1141865.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

This patch should apply to all branches as is or with trivial changes.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely but could do with a few days to bake on central.
Attachment #8690080 - Attachment is obsolete: true
Attachment #8690116 - Flags: sec-approval?
Attachment #8690116 - Flags: review+
Attachment #8690117 - Flags: review+
sec-approval+ for trunk. We'll want Aurora and Beta patches nominated as well.
Attachment #8690116 - Flags: sec-approval? → sec-approval+
I verified that the same patch applies cleanly to aurora and beta and the tests pass.
https://hg.mozilla.org/mozilla-central/rev/31dc40f11fbd
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8690116 [details] [diff] [review]
bug1207571-new-target v3

Approval Request Comment
[Feature/regressing bug #]: Bug 1141865.
[User impact if declined]: Possible security vulnerability.
[Describe test coverage new/current, TreeHerder]: On central since 24/11/15.
[Risks and why]: Low.
[String/UUID change made/needed]: None.
Attachment #8690116 - Flags: approval-mozilla-beta?
Attachment #8690116 - Flags: approval-mozilla-aurora?
Comment on attachment 8690116 [details] [diff] [review]
bug1207571-new-target v3

Crash fix, recent regression, please uplift to aurora and beta.
Attachment #8690116 - Flags: approval-mozilla-beta?
Attachment #8690116 - Flags: approval-mozilla-beta+
Attachment #8690116 - Flags: approval-mozilla-aurora?
Attachment #8690116 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:update,reconfirm,ignore] → [jsbugmon:update,reconfirm,ignore][adv-main42+]
Status: RESOLVED → VERIFIED
Crash Signature: [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone] → [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone]
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx43
JSBugMon: This bug has been automatically verified fixed on Fx44
Group: javascript-core-security → core-security-release
Crash Signature: [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone] → [@ js::DispatchValueTyped] [@ js::gc::TenuredCell::zone]
Whiteboard: [jsbugmon:update,reconfirm,ignore][adv-main42+] → [jsbugmon:update,reconfirm,ignore][adv-main43+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.