Closed
Bug 1427126
Opened 6 years ago
Closed 6 years ago
Assertion failure: !elements[i].isGCThing(), at js/src/gc/Marking.cpp:1669 with async
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla59
People
(Reporter: decoder, Assigned: jandem)
Details
(4 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main58+][post-critsmash-triage])
Attachments
(1 file)
693 bytes,
patch
|
bhackett1024
:
review+
gchang
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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) { yield; }(); asyncIter.next(); for (var i = 0; i < 20000; res++) { asyncIter.throw(); } Backtrace: 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.
Comment 1•6 years ago
|
||
(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.
Assignee | ||
Comment 2•6 years ago
|
||
I can take a look, considering this is probably TI related.
Flags: needinfo?(jdemooij)
Comment 3•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #2) Cheers Jan!
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jdemooij)
Assignee | ||
Comment 4•6 years ago
|
||
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
Status: NEW → ASSIGNED
Attachment #8939531 -
Flags: review?(bhackett1024)
Updated•6 years ago
|
Attachment #8939531 -
Flags: review?(bhackett1024) → review+
Assignee | ||
Updated•6 years ago
|
status-firefox57:
--- → affected
status-firefox58:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox58:
--- → ?
tracking-firefox59:
--- → ?
Keywords: sec-high
Assignee | ||
Comment 5•6 years ago
|
||
Comment on attachment 8939531 [details] [diff] [review] Patch [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? No. > Which older supported branches are affected by this flaw? 56+. > 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? Unlikely.
Attachment #8939531 -
Flags: sec-approval?
Comment 6•6 years ago
|
||
Comment on attachment 8939531 [details] [diff] [review] Patch 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+
Updated•6 years ago
|
Comment 7•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6be5a45a855e2af656477c53ee1d6f9a9f518212 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)
Comment 8•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6be5a45a855e (will be in the nightly which is currently being built)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Assignee | ||
Comment 9•6 years ago
|
||
Comment on attachment 8939531 [details] [diff] [review] Patch 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 10•6 years ago
|
||
Comment on attachment 8939531 [details] [diff] [review] Patch Fix a sec-high. Beta58+.
Attachment #8939531 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 11•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/3e7bec52b143
Updated•6 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main58+]
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [jsbugmon:update,bisect][adv-main58+] → [jsbugmon:update,bisect][adv-main58+][post-critsmash-triage]
Updated•6 years ago
|
Group: javascript-core-security → core-security-release
Updated•6 years ago
|
Comment 12•6 years ago
|
||
JSBugMon: This bug has been automatically verified fixed. JSBugMon: This bug has been automatically verified fixed on Fx58
Updated•6 years ago
|
Comment 13•6 years ago
|
||
JSBugMon: This bug has been automatically verified fixed on Fx59
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•