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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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.

Comment 1

[Fuzzing Team]

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.

Comment 2

[Christian Holler (:decoder)]

Needinfo from Eric based on comment 1.

Comment 3

[Andrew McCreight [:mccr8]]

Is this disabled outside of Nightly? It is a regression from what looks like a trunky-only feature, but maybe this affects older branches?

Comment 4

[Andrew McCreight [:mccr8]]

Jan, could you look into this or find somebody to look into it? It is almost a month old. Thanks.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

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 <[e-mail]>
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?

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

See previous comment.

Comment 7

[Al Billings [:abillings - ex-MoCo]]

Does this still reproduce, Christian?

Comment 8

[Fuzzing Team]

JSBugMon: This bug has been automatically confirmed to be still valid (reproduced on revision 3cc3b1968524).

Comment 9

[Christian Holler (:decoder)]

Needinfo from bbouvier based on comment 8 :)

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

It's crashing in the GC, needinfoing jonco.

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

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) { }
}

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43 since this is sec-critical. We could still take a fix on beta.

Comment 13

[Jon Coppeard (:jonco) (PTO until 24th June)]

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

Comment 14

[Jon Coppeard (:jonco) (PTO until 24th June)]

Looks like we're not marking new.target in native exit frames.

Comment 15

[Jon Coppeard (:jonco) (PTO until 24th June)]

Attachment: bug1207571-new-target

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.

Comment 16

[Jon Coppeard (:jonco) (PTO until 24th June)]

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 17

[Jan de Mooij [:jandem]]

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?

Comment 18

[Jon Coppeard (:jonco) (PTO until 24th June)]

Attachment: bug1207571-new-target v2

Ah, we can add a new exit frame token to tell us whether new.target is present!  That seems like the best approach.

Comment 19

[Jan de Mooij [:jandem]]

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.

Comment 20

[Jon Coppeard (:jonco) (PTO until 24th June)]

Attachment: bug1207571-new-target v3

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.

Comment 21

[Jon Coppeard (:jonco) (PTO until 24th June)]

Attachment: bug1207571-testcode

Comment 22

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk. We'll want Aurora and Beta patches nominated as well.

Comment 23

[Jon Coppeard (:jonco) (PTO until 24th June)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31dc40f11fbd

Comment 24

[Jon Coppeard (:jonco) (PTO until 24th June)]

I verified that the same patch applies cleanly to aurora and beta and the tests pass.

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/31dc40f11fbd

Comment 26

[Jon Coppeard (:jonco) (PTO until 24th June)]

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.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8690116 [details] [diff] [review]
bug1207571-new-target v3

Crash fix, recent regression, please uplift to aurora and beta.

Comment 28

[Jon Coppeard (:jonco) (PTO until 24th June)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/61733221899d
https://hg.mozilla.org/releases/mozilla-beta/rev/ffcd93a3cbba

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/61733221899d

Comment 30

[Fuzzing Team]

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