Closed Bug 1427126 Opened 3 years ago Closed 3 years ago

Assertion failure: !elements[i].isGCThing(), at js/src/gc/Marking.cpp:1669 with async


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox-esr52 --- unaffected
firefox57 --- wontfix
firefox58 + verified
firefox59 + verified
firefox60 --- verified


(Reporter: decoder, Assigned: jandem)


(5 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main58+][post-critsmash-triage])


(1 file)

The following testcase crashes on mozilla-central revision 01cbfc6c625f (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off):

class testWithExtends extends null {
    get 1() {}
s = "a%2b%20def%00A0";
res = decodeURI(s);
var asyncIter = async function*(...each) {
for (var i = 0; i < 20000; res++) {


received signal SIGSEGV, Segmentation fault.
0x0000000000e724f3 in ObjectDenseElementsMayBeMarkable (nobj=0x7ffff448e2a0) at js/src/gc/Marking.cpp:1669
#0  0x0000000000e724f3 in ObjectDenseElementsMayBeMarkable (nobj=0x7ffff448e2a0) at js/src/gc/Marking.cpp:1669
#1  0x0000000000e7ddf0 in js::TenuringTracer::traceObject (this=this@entry=0x7fffffffbde0, obj=obj@entry=0x7ffff448e2a0) at js/src/gc/Marking.cpp:2889
#2  0x0000000000e7e211 in js::Nursery::collectToFixedPoint (this=this@entry=0x7ffff5f1cd88, mover=..., tenureCounts=...) at js/src/gc/Marking.cpp:3146
#3  0x0000000000e7e6b1 in js::Nursery::doCollection (this=this@entry=0x7ffff5f1cd88, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY, tenureCounts=...) at js/src/gc/Nursery.cpp:827
#4  0x0000000000e7ed9d in js::Nursery::collect (this=0x7ffff5f1cd88, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY) at js/src/gc/Nursery.cpp:677
#5  0x00000000009f6e2a in js::gc::GCRuntime::minorGC (this=0x7ffff5f1a740, reason=reason@entry=JS::gcreason::OUT_OF_NURSERY, phase=phase@entry=js::gcstats::PhaseKind::MINOR_GC) at js/src/jsgc.cpp:7705
#6  0x0000000000d94efa in js::gc::GCRuntime::tryNewNurseryObject<(js::AllowGC)1> (this=this@entry=0x7ffff5f1a740, cx=cx@entry=0x7ffff5f16000, thingSize=thingSize@entry=96, nDynamicSlots=nDynamicSlots@entry=0, clasp=0x1fc9320 <PromiseDebugInfo::class_>) at js/src/gc/Allocator.cpp:92
#7  0x0000000000da134c in js::Allocate<JSObject, (js::AllowGC)1> (cx=cx@entry=0x7ffff5f16000, kind=kind@entry=js::gc::AllocKind::OBJECT8_BACKGROUND, nDynamicSlots=0, heap=heap@entry=js::gc::DefaultHeap, clasp=clasp@entry=0x1fc9320 <PromiseDebugInfo::class_>) at js/src/gc/Allocator.cpp:56
#8  0x0000000000a2df2a in js::NativeObject::create (cx=0x7ffff5f16000, kind=<optimized out>, heap=js::gc::DefaultHeap, shape=..., group=...) at js/src/vm/NativeObject-inl.h:540
#9  0x0000000000a5d194 in NewObject (cx=0x7ffff5f16000, group=..., kind=<optimized out>, newKind=js::GenericObject, initialShapeFlags=<optimized out>) at js/src/jsobj.cpp:732
#10 0x0000000000a5da96 in js::NewObjectWithClassProtoCommon (cx=cx@entry=0x7ffff5f16000, clasp=clasp@entry=0x1fc9320 <PromiseDebugInfo::class_>, protoArg=..., protoArg@entry=..., allocKind=js::gc::AllocKind::OBJECT8_BACKGROUND, newKind=newKind@entry=js::GenericObject) at js/src/jsobj.cpp:853
#11 0x000000000062ba7e in js::NewObjectWithClassProto (newKind=js::GenericObject, allocKind=<optimized out>, proto=..., clasp=0x1fc9320 <PromiseDebugInfo::class_>, cx=0x7ffff5f16000) at js/src/jsobjinlines.h:689
#12 js::NewObjectWithClassProto (newKind=js::GenericObject, proto=..., clasp=0x1fc9320 <PromiseDebugInfo::class_>, cx=0x7ffff5f16000) at js/src/jsobjinlines.h:697
#13 js::NewObjectWithClassProto<PromiseDebugInfo> (newKind=js::GenericObject, proto=..., cx=0x7ffff5f16000) at js/src/jsobjinlines.h:705
#14 PromiseDebugInfo::create (cx=cx@entry=0x7ffff5f16000, promise=..., promise@entry=...) at js/src/builtin/Promise.cpp:186
#15 0x000000000061d097 in CreatePromiseObjectInternal (cx=cx@entry=0x7ffff5f16000, proto=..., proto@entry=..., protoIsWrapped=protoIsWrapped@entry=false, informDebugger=informDebugger@entry=true) at js/src/builtin/Promise.cpp:1504
#16 0x000000000061dff4 in CreatePromiseObjectWithoutResolutionFunctions (cx=cx@entry=0x7ffff5f16000) at js/src/builtin/Promise.cpp:856
#17 0x00000000006213ba in js::AsyncGeneratorEnqueue (cx=cx@entry=0x7ffff5f16000, asyncGenVal=..., completionKind=completionKind@entry=js::CompletionKind::Throw, completionValue=..., result=...) at js/src/builtin/Promise.cpp:2976
#18 0x0000000000b2423c in AsyncGeneratorThrow (cx=0x7ffff5f16000, argc=<optimized out>, vp=<optimized out>) at js/src/vm/AsyncIteration.cpp:258
#19 0x00002eb9e98bec81 in ?? ()
#20 0x00002eb9e98befca in ?? ()
#21 0x00007fffffffc640 in ?? ()
#22 0x0000000000000000 in ?? ()
rax	0x0	0
rbx	0x7ffff448e2a0	140737291805344
rcx	0x7ffff6c282ad	140737333330605
rdx	0x0	0
rsi	0x7ffff6ef7770	140737336276848
rdi	0x7ffff6ef6540	140737336272192
rbp	0x7fffffffbca0	140737488338080
rsp	0x7fffffffbc80	140737488338048
r8	0x7ffff6ef7770	140737336276848
r9	0x7ffff7fe4780	140737354024832
r10	0x58	88
r11	0x7ffff6b9e7a0	140737332766624
r12	0x0	0
r13	0x7ffff4488100	140737291780352
r14	0x2	2
r15	0xbad0bad1	3134241489
rip	0xe724f3 <ObjectDenseElementsMayBeMarkable(js::NativeObject*)+259>
=> 0xe724f3 <ObjectDenseElementsMayBeMarkable(js::NativeObject*)+259>:	movl   $0x0,0x0
   0xe724fe <ObjectDenseElementsMayBeMarkable(js::NativeObject*)+270>:	ud2

Marking s-s because this looks like a GC hazard.
(In reply to Christian Holler (:decoder) from comment #0)
The assertion tells us that we're tracing an array containing a GC pointer but whose type information tells us should not contain any.

Either the checks in ObjectDenseElementsMayBeMarkable is wrong or the type information for this array is wrong.
I can take a look, considering this is probably TI related.
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #2)
Cheers Jan!
Flags: needinfo?(jdemooij)
Attached patch PatchSplinter Review
We need to use setElementWithType instead of setElement in AppendToList.

Even though these objects are not exposed to JS, it affects ObjectDenseElementsMayBeMarkable...

I filed bug 1427727 to make it easier to catch these bugs (and harder to introduce them).
Assignee: nobody → jdemooij
Attachment #8939531 - Flags: review?(bhackett1024)
Attachment #8939531 - Flags: review?(bhackett1024) → review+
Comment on attachment 8939531 [details] [diff] [review]

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Not super easily but it's possible.

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

> Which older supported branches are affected by this flaw?

> If not all supported branches, which bug introduced the flaw?
Bug 1272697 added the code (for ReadableStream). Since bug 1390082 we use this code for async generators as well.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Just a one-liner - should apply or be easy to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Attachment #8939531 - Flags: sec-approval?
Comment on attachment 8939531 [details] [diff] [review]

sec-approval+ for trunk. Please nominate this patch for beta (I assume it applies cleanly since it is so tiny).
Attachment #8939531 - Flags: sec-approval? → sec-approval+

This also grafts cleanly to Beta, so please request approval on it ASAP so we can get it uplifted for this week's b16 build still.
Flags: needinfo?(jdemooij) (will be in the nightly which is currently being built)
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Comment on attachment 8939531 [details] [diff] [review]

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1272697 added the code (for ReadableStream). Since bug 1390082 we use this code for async generators as well.
[User impact if declined]: Crashes, security bugs.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: One-liner, safe change.
[String changes made/needed]: None.
Flags: needinfo?(jdemooij)
Attachment #8939531 - Flags: approval-mozilla-beta?
Comment on attachment 8939531 [details] [diff] [review]

Fix a sec-high. Beta58+.
Attachment #8939531 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main58+]
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect][adv-main58+] → [jsbugmon:update,bisect][adv-main58+][post-critsmash-triage]
Group: javascript-core-security → core-security-release
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx58
JSBugMon: This bug has been automatically verified fixed on Fx59
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.