Closed Bug 1233925 Opened 8 years ago Closed 8 years ago

Crash [@ JSObject::allocKindForTenure] with rest parameter

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla46
Tracking Status
firefox42 --- wontfix
firefox43 --- wontfix
firefox44 + fixed
firefox45 + verified
firefox46 + verified
firefox-esr38 44+ fixed
firefox-esr45 - fixed
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: jandem)

Details

(4 keywords, Whiteboard: [jsbugmon:update,testComment=11,origRev=7c83da46ea74,ignore][fuzzblocker][adv-main44+][adv-esr38.6+])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 66fb852962c0 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions main.js):

See attachment.


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
JSObject::allocKindForTenure (this=0x7ffff54fff90, nursery=...) at ../../dist/include/js/Proxy.h:434
#0  JSObject::allocKindForTenure (this=0x7ffff54fff90, nursery=...) at ../../dist/include/js/Proxy.h:434
#1  0x0000000000bdccc4 in js::TenuringTracer::moveToTenured (this=0x7fffffec7b50, src=0x7ffff54fff90) at js/src/gc/Marking.cpp:2070
#2  0x0000000000bdd0ab in js::TenuringTracer::traverse<JSObject> (this=<optimized out>, objp=0x7fffffec78d0) at js/src/gc/Marking.cpp:1940
#3  0x0000000000bf15fd in operator()<JSObject> (this=<synthetic pointer>, trc=<optimized out>, t=0x7ffff54fff90) at js/src/gc/Marking.cpp:1946
#4  js::DispatchTyped<js::TenuringTraversalFunctor<JS::Value>, js::TenuringTracer* const>(js::TenuringTraversalFunctor<JS::Value>, JS::Value const&, (decltype ({parm#1}((JSObject*)((decltype(nullptr))0), (Forward<js::TenuringTracer* const>)({parm#3})))&&)...) (f=f@entry=..., val=...) at ../../dist/include/js/Value.h:1885
#5  0x0000000000bdf777 in traverse<JS::Value> (thingp=0x7ffff506e130, this=0x7fffffec7b50) at js/src/gc/Marking.cpp:1955
#6  traceSlots (end=0x7ffff506e138, vp=0x7ffff506e130, this=0x7fffffec7b50) at js/src/gc/Marking.cpp:2163
#7  js::TenuringTracer::traceObject (this=this@entry=0x7fffffec7b50, obj=obj@entry=0x7ffff506e100) at js/src/gc/Marking.cpp:2139
#8  0x0000000000bdfa28 in js::Nursery::collectToFixedPoint (this=this@entry=0x7ffff695d468, mover=..., tenureCounts=...) at js/src/gc/Marking.cpp:2103
#9  0x0000000000be0b14 in js::Nursery::collect (this=this@entry=0x7ffff695d468, rt=0x7ffff695d000, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY, pretenureGroups=pretenureGroups@entry=0x7fffffec7ea0) at js/src/gc/Nursery.cpp:484
#10 0x00000000008e8c65 in js::gc::GCRuntime::minorGCImpl (this=this@entry=0x7ffff695d410, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY, pretenureGroups=pretenureGroups@entry=0x7fffffec7ea0) at js/src/jsgc.cpp:6619
#11 0x00000000008f2709 in js::gc::GCRuntime::minorGC (this=this@entry=0x7ffff695d410, cx=cx@entry=0x7ffff6907400, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY) at js/src/jsgc.cpp:6631
#12 0x0000000000bcc27a in tryNewNurseryObject<(js::AllowGC)1> (clasp=0x1b67900 <js::ArrayObject::class_>, nDynamicSlots=0, thingSize=64, cx=0x7ffff6907400, this=0x7ffff695d410) at js/src/gc/Allocator.cpp:159
#13 js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff6907400, kind=kind@entry=js::gc::OBJECT4_BACKGROUND, nDynamicSlots=0, heap=heap@entry=js::gc::DefaultHeap, clasp=0x1b67900 <js::ArrayObject::class_>) at js/src/gc/Allocator.cpp:125
#14 0x000000000052fc0d in js::ArrayObject::createArrayInternal (cx=0x7ffff6907400, kind=js::gc::OBJECT4_BACKGROUND, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/vm/ArrayObject-inl.h:54
#15 0x000000000052fe10 in js::ArrayObject::createArray (cx=cx@entry=0x7ffff6907400, kind=kind@entry=js::gc::OBJECT4_BACKGROUND, heap=<optimized out>, shape=..., shape@entry=..., group=..., group@entry=..., length=length@entry=1, metadata=...) at js/src/vm/ArrayObject-inl.h:82
#16 0x0000000000519727 in NewArray<4294967295u> (cxArg=0x7ffff6907400, length=1, protoArg=..., newKind=js::GenericObject) at js/src/jsarray.cpp:3374
#17 0x000000000051c19e in js::NewDenseCopiedArray (cx=<optimized out>, length=length@entry=1, values=values@entry=0x7fffffec82f8, proto=..., proto@entry=..., newKind=<optimized out>) at js/src/jsarray.cpp:3439
#18 0x00000000007fdff5 in js::jit::InitRestParameter (cx=<optimized out>, length=1, rest=0x7fffffec82f8, templateObj=..., objRes=...) at js/src/jit/VMFunctions.cpp:931
#19 0x00007ffff7fed8dd in ?? ()
[...]
#29 0x0000000000000000 in ?? ()
rax	0x7ffff4f74af0	140737303235312
rbx	0x7ffff54fff90	140737309048720
rcx	0x1ba9c00	29006848
rdx	0xfffc7ffff4f724c0	-985162603617088
rsi	0x7ffff695d468	140737330402408
rdi	0x7ffff54fff90	140737309048720
rbp	0x7fffffec7820	140737487075360
rsp	0x7fffffec7800	140737487075328
r8	0x0	0
r9	0x7ffff5107150	140737304883536
r10	0x7ffff5107000	140737304883200
r11	0x1f	31
r12	0x1bb4800	29050880
r13	0x1bb5268	29053544
r14	0x7fffffec7900	140737487075584
r15	0x7ffff506e100	140737304256768
rip	0x92b8a4 <JSObject::allocKindForTenure(js::Nursery const&) const+68>
=> 0x92b8a4 <JSObject::allocKindForTenure(js::Nursery const&) const+68>:	mov    0x8(%rdx),%ecx
   0x92b8a7 <JSObject::allocKindForTenure(js::Nursery const&) const+71>:	test   $0x100000,%ecx


Marking as s-s because this is a fragile GC-related crash with weird memory address. I was not able to merge the files into a single-file testcase. Bonus points if you can show me how to do that (in particular, replacing load with evaluate totally did not work in this testcase, not even with all the options I tried for evaluate).
Attached file Testcase
This is an automated crash issue comment:

Summary: Crash [@ JSObject::allocKindForTenure]
Build version: mozilla-central revision 388bdc46ba51
Build flags: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug
Runtime options: --fuzzing-safe --thread-count=2 --ion-eager --ion-extra-checks --ion-offthread-compile=off

Testcase:

function runTestCase() testcase();
setJitCompilerOption("ion.warmup.trigger", 40);
gczeal(2);
function testcase(... of)  {
	function foo() testcase(foo);
	this;
	foo();
}
runTestCase();


Backtrace:

Program received signal SIGSEGV, Segmentation fault.
JSObject::allocKindForTenure (this=0x7ffff4803ae0, nursery=...) at js/src/jsobj.h:122
#0  JSObject::allocKindForTenure (this=0x7ffff4803ae0, nursery=...) at js/src/jsobj.h:122
#1  0x0000000000bdcb64 in js::TenuringTracer::moveToTenured (this=0x7ffffffee2d0, src=0x7ffff4803ae0) at js/src/gc/Marking.cpp:2070
#2  0x0000000000bdcf4b in js::TenuringTracer::traverse<JSObject> (this=<optimized out>, objp=0x7ffffffee050) at js/src/gc/Marking.cpp:1940
#3  0x0000000000bf149d in operator()<JSObject> (this=<synthetic pointer>, trc=<optimized out>, t=0x7ffff4803ae0) at js/src/gc/Marking.cpp:1946
#4  js::DispatchTyped<js::TenuringTraversalFunctor<JS::Value>, js::TenuringTracer* const>(js::TenuringTraversalFunctor<JS::Value>, JS::Value const&, (decltype ({parm#1}((JSObject*)((decltype(nullptr))0), (Forward<js::TenuringTracer* const>)({parm#3})))&&)...) (f=f@entry=..., val=...) at ../../dist/include/js/Value.h:1885
#5  0x0000000000bdf617 in traverse<JS::Value> (thingp=0x7ffff7e88770, this=0x7ffffffee2d0) at js/src/gc/Marking.cpp:1955
#6  traceSlots (end=0x7ffff7e88778, vp=0x7ffff7e88770, this=0x7ffffffee2d0) at js/src/gc/Marking.cpp:2163
#7  js::TenuringTracer::traceObject (this=this@entry=0x7ffffffee2d0, obj=obj@entry=0x7ffff7e88740) at js/src/gc/Marking.cpp:2139
#8  0x0000000000bdf8c8 in js::Nursery::collectToFixedPoint (this=this@entry=0x7ffff695d468, mover=..., tenureCounts=...) at js/src/gc/Marking.cpp:2103
#9  0x0000000000be09b4 in js::Nursery::collect (this=this@entry=0x7ffff695d468, rt=0x7ffff695d000, reason=reason@entry=JS::gcreason::DEBUG_GC, pretenureGroups=pretenureGroups@entry=0x0) at js/src/gc/Nursery.cpp:484
#10 0x00000000008e8b85 in js::gc::GCRuntime::minorGCImpl (this=this@entry=0x7ffff695d410, reason=reason@entry=JS::gcreason::DEBUG_GC, pretenureGroups=pretenureGroups@entry=0x0) at js/src/jsgc.cpp:6619
#11 0x0000000000915c13 in js::gc::GCRuntime::evictNursery (this=0x7ffff695d410, reason=JS::gcreason::DEBUG_GC) at js/src/gc/GCRuntime.h:611
#12 0x0000000000909e95 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff695d410, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6227
#13 0x000000000090a541 in js::gc::GCRuntime::collect (this=this@entry=0x7ffff695d410, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6388
#14 0x000000000090a773 in js::gc::GCRuntime::gc (this=0x7ffff695d410, gckind=<optimized out>, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:6446
#15 0x000000000090bcc5 in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff695d410) at js/src/jsgc.cpp:6931
#16 0x0000000000bbd987 in js::gc::GCRuntime::gcIfNeededPerAllocation (this=this@entry=0x7ffff695d410, cx=cx@entry=0x7ffff6907400) at js/src/gc/Allocator.cpp:28
#17 0x0000000000bc6b6f in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=0x7ffff695d410, cx=0x7ffff6907400, kind=js::gc::OBJECT4_BACKGROUND) at js/src/gc/Allocator.cpp:55
#18 0x0000000000bcc057 in js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff6907400, kind=kind@entry=js::gc::OBJECT4_BACKGROUND, nDynamicSlots=0, heap=heap@entry=js::gc::DefaultHeap, clasp=0x1b67920 <js::ArrayObject::class_>) at js/src/gc/Allocator.cpp:121
#19 0x000000000052fbcd in js::ArrayObject::createArrayInternal (cx=0x7ffff6907400, kind=js::gc::OBJECT4_BACKGROUND, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/vm/ArrayObject-inl.h:54
#20 0x000000000052fdd0 in js::ArrayObject::createArray (cx=cx@entry=0x7ffff6907400, kind=kind@entry=js::gc::OBJECT4_BACKGROUND, heap=<optimized out>, shape=..., shape@entry=..., group=..., group@entry=..., length=length@entry=1, metadata=...) at js/src/vm/ArrayObject-inl.h:82
#21 0x0000000000519717 in NewArray<4294967295u> (cxArg=0x7ffff6907400, length=1, protoArg=..., newKind=js::GenericObject) at js/src/jsarray.cpp:3374
#22 0x000000000051c18e in js::NewDenseCopiedArray (cx=<optimized out>, length=length@entry=1, values=values@entry=0x7ffffffeed78, proto=..., proto@entry=..., newKind=<optimized out>) at js/src/jsarray.cpp:3439
#23 0x00000000007fdf95 in js::jit::InitRestParameter (cx=<optimized out>, length=1, rest=0x7ffffffeed78, templateObj=..., objRes=...) at js/src/jit/VMFunctions.cpp:925
#24 0x00007ffff7fed8dd in ?? ()
#25 0x00007ffff694e800 in ?? ()
#26 0x00007ffffffeecd0 in ?? ()
#27 0x0000000001bc43c0 in js::jit::GetPropertyInfo ()
#28 0x00007ffff7e5a580 in ?? ()
#29 0x00007ffff7fd4225 in ?? ()
#30 0x0000000000000c00 in ?? ()
#31 0x0000000000000001 in ?? ()
#32 0x00007ffffffeed78 in ?? ()
#33 0x00007ffff7e84040 in ?? ()
#34 0x0000000000000000 in ?? ()
rax	0x130000	1245184
rbx	0x7ffff4803ae0	140737295432416
rcx	0x1b67920	28735776
rdx	0x7ffff695d468	140737330402408
rsi	0x7ffff695d468	140737330402408
rdi	0x7ffff4803ae0	140737295432416
rbp	0x7ffffffedfa0	140737488281504
rsp	0x7ffffffedf80	140737488281472
r8	0x0	0
r9	0x0	0
r10	0x7ffff69629f0	140737330424304
r11	0x7ffff6962908	140737330424072
r12	0x7ffffffee2d0	140737488282320
r13	0x7ffff7e88778	140737352599416
r14	0x7ffffffee080	140737488281728
r15	0x7ffff7e88740	140737352599360
rip	0x92b798 <JSObject::allocKindForTenure(js::Nursery const&) const+24>
=> 0x92b798 <JSObject::allocKindForTenure(js::Nursery const&) const+24>:	mov    (%rax),%rdx
   0x92b79b <JSObject::allocKindForTenure(js::Nursery const&) const+27>:	cmp    %rcx,%rdx


Managed to find a single-file testcase for this now :)
Assuming sec-high due to the crash address being read here.
Keywords: sec-high
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
Whiteboard: [jsbugmon:bisect] → [jsbugmon:update,bisect,testComment=2,origRev=388bdc46ba51]
Whiteboard: [jsbugmon:update,bisect,testComment=2,origRev=388bdc46ba51] → [jsbugmon:update,testComment=2,origRev=388bdc46ba51]
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/52d7c9292ecf
user:        Jan de Mooij
date:        Sat Nov 21 14:33:13 2015 +0100
summary:     Bug 1132183 - Make |this| a real binding, remove lazy this computation. r=efaust,shu

This iteration took 7.407 seconds to run.
Jan, is bug 1132183 a likely regressor?
Blocks: 1132183
Flags: needinfo?(jdemooij)
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #6)
> Jan, is bug 1132183 a likely regressor?

No. The test uses 'this' and that bug changed how 'this' is implemented, but it should be possible to write a test that fails at an earlier revision.

The problem here is MRest codegen reading the frame's formals directly but MarkThisAndArguments not always marking the formals. We do mark the formals explicitly if the function uses 'arguments' and we just need to do something similar for 'this'.

Will post a patch later.
No longer blocks: 1132183
(In reply to Jan de Mooij [:jandem] from comment #7)
> We do mark the formals
> explicitly if the function uses 'arguments' and we just need to do something
> similar for 'this'.

Er, s/'this'/rest/
Whiteboard: [jsbugmon:update,testComment=2,origRev=388bdc46ba51] → [jsbugmon:update,testComment=2,origRev=388bdc46ba51,ignore]
Whiteboard: [jsbugmon:update,testComment=2,origRev=388bdc46ba51,ignore] → [jsbugmon:testComment=2,origRev=388bdc46ba51,bisectfix]
Whiteboard: [jsbugmon:testComment=2,origRev=388bdc46ba51,bisectfix] → [jsbugmon:testComment=2,origRev=388bdc46ba51]
I can still reproduce using the testcase in comment 2:

Summary: Crash [@ JSObject::allocKindForTenure]
Build version: mozilla-central revision 7c83da46ea74
Build flags: --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug
Runtime options: --fuzzing-safe --no-threads --ion-eager

Testcase:

function runTestCase() testcase();
setJitCompilerOption("ion.warmup.trigger", 40);
gczeal(2);
function testcase(... of)  {
	function foo() testcase(foo);
	this;
	foo();
}
runTestCase();
Whiteboard: [jsbugmon:testComment=2,origRev=388bdc46ba51] → [jsbugmon:testComment=11,origRev=7c83da46ea74]
Whiteboard: [jsbugmon:testComment=11,origRev=7c83da46ea74] → [jsbugmon:update,testComment=11,origRev=7c83da46ea74]
Whiteboard: [jsbugmon:update,testComment=11,origRev=7c83da46ea74] → [jsbugmon:update,testComment=11,origRev=7c83da46ea74,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 7c83da46ea74).
(In reply to Fuzzing Team from comment #12)
> JSBugMon: The testcase found in this bug no longer reproduces (tried
> revision 7c83da46ea74).

Sigh. I think this is an intermittent bug, though fairly reliably reproducible on some machines.
Attached patch PatchSplinter Review
Treat functions with rest parameters like functions with an arguments object: don't spill to their formal argument slots and always mark the formals. To avoid duplicating this logic in multiple places, I added a helper method, script->mayReadFrameArgsDirectly().
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8703121 - Flags: review?(bhackett1024)
This causes all sorts of other crashes that don't have any REST-associated function in the stack anywhere. Please land asap, thanks!
Whiteboard: [jsbugmon:update,testComment=11,origRev=7c83da46ea74,ignore] → [jsbugmon:update,testComment=11,origRev=7c83da46ea74,ignore][fuzzblocker]
Comment on attachment 8703121 [details] [diff] [review]
Patch

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

This sounds good to me.
Attachment #8703121 - Flags: review?(bhackett1024) → review+
Comment on attachment 8703121 [details] [diff] [review]
Patch

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
It's not trivial to trigger a GC right there, but with some effort it might be possible for someone to figure out the problem and find a reliable way to do that.

* 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?
38+

* If not all supported branches, which bug introduced the flaw?
Bug 934502.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply or be easy to backport.

* How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8703121 - Flags: sec-approval?
sec-approval+ for trunk.

Please prepare and nominate branch patches (including ESR38) as well after checkin.
Attachment #8703121 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/dc55c41b6331
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Comment on attachment 8703121 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: Bug 934502.
[User impact if declined]: Security bugs, crashes.
[Describe test coverage new/current, TreeHerder]: Fixes the crash.
[Risks and why]: Low risk. Furthermore, rest parameters are relatively new so they are not used much on the web.
[String/UUID change made/needed]: None.
Attachment #8703121 - Flags: approval-mozilla-esr38?
Attachment #8703121 - Flags: approval-mozilla-beta?
Attachment #8703121 - Flags: approval-mozilla-aurora?
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 8703121 [details] [diff] [review]
Patch

Sec-fix that was verified, taking it to beta, aurora, esr38.
Attachment #8703121 - Flags: approval-mozilla-esr38?
Attachment #8703121 - Flags: approval-mozilla-esr38+
Attachment #8703121 - Flags: approval-mozilla-beta?
Attachment #8703121 - Flags: approval-mozilla-beta+
Attachment #8703121 - Flags: approval-mozilla-aurora?
Attachment #8703121 - Flags: approval-mozilla-aurora+
This doesn't apply cleanly to aurora or beta (so probably definitely doesn't apply cleanly to esr38). Can we get rebased patches?
Flags: needinfo?(jdemooij)
JSBugMon: This bug has been automatically verified fixed on Fx45
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update,testComment=11,origRev=7c83da46ea74,ignore][fuzzblocker] → [jsbugmon:update,testComment=11,origRev=7c83da46ea74,ignore][fuzzblocker][adv-main44+][adv-esr38.6+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.