Closed Bug 1464472 Opened 7 years ago Closed 6 years ago

Crash [@ ShouldMark<JSString*>] or Assertion failure: addr % CellAlignBytes == 0, at gc/Cell.h:242

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- verified

People

(Reporter: decoder, Assigned: Waldo)

References

Details

(6 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision bf4762f10b8d (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe): gczeal(17, 1); for (var i = 0; i < 100; ++i) { Object.prototype[1012] = "value"; imports = {} for (dmod in imports) {} } Backtrace: received signal SIGSEGV, Segmentation fault. ShouldMark<JSString*> (str=0xe5e5e5e5e5e5e5e5, gcmarker=0x7ffff5f1a098) at js/src/gc/Marking.cpp:808 #0 ShouldMark<JSString*> (str=0xe5e5e5e5e5e5e5e5, gcmarker=0x7ffff5f1a098) at js/src/gc/Marking.cpp:808 #1 DoMarking<JSString> (gcmarker=0x7ffff5f1a098, thing=0xe5e5e5e5e5e5e5e5) at js/src/gc/Marking.cpp:820 #2 0x0000000000974a12 in js::NativeIterator::<lambda(js::GCPtrFlatString&)>::operator() (prop=..., __closure=<synthetic pointer>) at js/src/vm/Iteration.cpp:74 #3 std::for_each<js::GCPtr<JSFlatString*>*, js::NativeIterator::trace(JSTracer*)::<lambda(js::GCPtrFlatString&)> > (__f=..., __last=<optimized out>, __first=0x7ffff49e8c50) at /usr/include/c++/6/bits/stl_algo.h:3769 #4 js::NativeIterator::trace (this=<optimized out>, trc=0x7ffff5f1a098) at js/src/vm/Iteration.cpp:75 #5 0x0000000000c4cf67 in js::Class::doTrace (this=<optimized out>, obj=0x7ffff4c7f160, trc=0x7ffff5f1a098) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/instrumentation/none/type/opt/dist/include/js/Class.h:869 #6 CallTraceHook<TraverseObjectFunctor, js::GCMarker*, JSObject*&> (check=CheckGeneration::DoChecks, obj=0x7ffff4c7f160, trc=0x7ffff5f1a098, f=...) at js/src/gc/Marking.cpp:1619 #7 js::GCMarker::processMarkStackTop (this=this@entry=0x7ffff5f1a098, budget=...) at js/src/gc/Marking.cpp:1855 #8 0x0000000000c3d827 in js::GCMarker::drainMarkStack (this=this@entry=0x7ffff5f1a098, budget=...) at js/src/gc/Marking.cpp:1665 #9 0x0000000000bf0750 in js::gc::GCRuntime::drainMarkStack (this=0x7ffff5f194b0, sliceBudget=..., phase=<optimized out>) at js/src/gc/GC.cpp:5908 #10 0x0000000000c0eb0a in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff5f194b0, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC, session=...) at js/src/gc/GC.cpp:7121 #11 0x0000000000c0fad7 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f194b0, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/gc/GC.cpp:7483 #12 0x0000000000c1005d in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f194b0, nonincrementalByAPI=nonincrementalByAPI@entry=false, budget=..., reason=reason@entry=JS::gcreason::DEBUG_GC) at js/src/gc/GC.cpp:7626 #13 0x0000000000c110d8 in js::gc::GCRuntime::runDebugGC (this=this@entry=0x7ffff5f194b0) at js/src/gc/GC.cpp:8188 #14 0x0000000000c11298 in js::gc::GCRuntime::gcIfNeededAtAllocation (this=0x7ffff5f194b0, cx=0x7ffff5f14000) at js/src/gc/Allocator.cpp:312 #15 0x0000000000c2c079 in js::gc::GCRuntime::checkAllocatorState<(js::AllowGC)1> (this=<optimized out>, cx=0x7ffff5f14000, kind=<optimized out>) at js/src/gc/Allocator.cpp:273 #16 0x0000000000c2c6bd in js::AllocateString<JSString, (js::AllowGC)1> (cx=cx@entry=0x7ffff5f14000, heap=heap@entry=js::gc::DefaultHeap) at js/src/gc/Allocator.cpp:179 #17 0x00000000008b7ce2 in js::Allocate<JSThinInlineString, (js::AllowGC)1> (heap=js::gc::DefaultHeap, cx=0x7ffff5f14000) at js/src/gc/Allocator.h:47 #18 JSThinInlineString::new_<(js::AllowGC)1> (cx=0x7ffff5f14000) at js/src/vm/StringType-inl.h:279 #19 js::AllocateInlineString<(js::AllowGC)1, unsigned char> (chars=<synthetic pointer>, len=<optimized out>, cx=0x7ffff5f14000) at js/src/vm/StringType-inl.h:34 #20 js::NewInlineString<(js::AllowGC)1, unsigned char> (chars=..., cx=0x7ffff5f14000) at js/src/vm/StringType-inl.h:60 #21 js::Int32ToString<(js::AllowGC)1> (cx=cx@entry=0x7ffff5f14000, si=1012) at js/src/jsnum.cpp:627 #22 0x0000000000982762 in js::IdToString (id=..., cx=0x7ffff5f14000) at js/src/vm/JSAtom-inl.h:154 #23 js::NativeIterator::NativeIterator (this=0x7ffff49e8c10, cx=0x7ffff5f14000, propIter=..., objBeingIterated=..., props=..., numGuards=2, guardKey=0, hadError=0x7fffffffcd28) at js/src/vm/Iteration.cpp:679 #24 0x0000000000982fa8 in CreatePropertyIterator (guardKey=0, numGuards=2, props=..., objBeingIterated=..., cx=0x7ffff5f14000) at js/src/vm/Iteration.cpp:610 #25 VectorToKeyIterator (numGuards=2, props=..., obj=..., cx=0x7ffff5f14000) at js/src/vm/Iteration.cpp:747 #26 js::GetIterator (cx=cx@entry=0x7ffff5f14000, obj=obj@entry=...) at js/src/vm/Iteration.cpp:910 #27 0x00000000009847ee in js::ValueToIterator (cx=0x7ffff5f14000, vp=...) at js/src/vm/Iteration.cpp:1150 [...] #38 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9339 rax 0xe5e5e5e5e5e00000 -1880844493790380032 rbx 0x7ffff49e8c58 140737297419352 rcx 0xfe2c 65068 rdx 0xf23e05 15875589 rsi 0xe5e5e5e5e5e5e5e5 -1880844493789993499 rdi 0x7ffff5f1a098 140737319641240 rbp 0x7ffff49e8c70 140737297419376 rsp 0x7fffffffc7b8 140737488340920 r8 0x1 1 r9 0x100000000000 17592186044416 r10 0x104104104100 17871427092736 r11 0x0 0 r12 0x7ffff5f1a098 140737319641240 r13 0xf23e05 15875589 r14 0x7ffff4c7f160 140737300132192 r15 0x7fffffffcaf0 140737488341744 rip 0xc41909 <DoMarking<JSString>(js::GCMarker*, JSString*)+9> => 0xc41909 <DoMarking<JSString>(js::GCMarker*, JSString*)+9>: mov 0xffff8(%rax),%rdx 0xc41910 <DoMarking<JSString>(js::GCMarker*, JSString*)+16>: cmp %rdx,(%rdi) Marking s-s and sec-high due to GC crash/assert and dangerous 0xe5e5 pattern in crash.
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/6f7c84e815c0 user: Jeff Walden date: Wed May 23 10:14:05 2018 -0700 summary: Bug 1462939. r=jandem This iteration took 246.504 seconds to run.
Keywords: sec-high
Simple bug when you look at it. Tricky thing is figuring out a fix. Should have one shortly nonetheless. <3 fuzzers, and also I am a dumb still.
Assignee: nobody → jwalden+bmo
Status: NEW → ASSIGNED
Attached patch Patch (deleted) — Splinter Review
Sigh.
Attachment #8980904 - Flags: review?(jdemooij)
Attached patch Add some comments and the test (deleted) — Splinter Review
Attachment #8980905 - Flags: review?(jdemooij)
Comment on attachment 8980904 [details] [diff] [review] Patch Review of attachment 8980904 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, good find. Properties must be initialized before guards because of the guardKey thing? It's pretty weird but pre-existing. ::: js/src/jit/BaselineCacheIRCompiler.cpp @@ +1979,5 @@ > masm.loadPtr(iterAddr, output); > masm.loadObjPrivate(output, JSObject::ITER_CLASS_NFIXED_SLOTS, niScratch); > > + // Ensure the iterator is reusable: see NativeIterator::isReusable. > + masm.branchTestNativeIteratorNotReusable(niScratch, failure->label()); Nit: maybe s/branchTest/branchIf/ ?
Attachment #8980904 - Flags: review?(jdemooij) → review+
Comment on attachment 8980905 [details] [diff] [review] Add some comments and the test Review of attachment 8980905 [details] [diff] [review]: ----------------------------------------------------------------- Nice comments. ::: js/src/vm/Iteration.h @@ +67,5 @@ > + // of being constructed. At such time |guardsEnd_| accounts only for > + // guards that have been initialized -- potentially none of them. > + // Instead, |propertyCursor_| is initialized to the ultimate/actual > + // start of properties and must be used instead of |propertiesBegin()|, > + // which asserts that thsi flag is present to guard against misuse. s/thsi/this/
Attachment #8980905 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6e1c06f4d76667e00ae0b520eff0225292396a Holding off on test/comments for a week or so to give nightly users time to update, as usual.
Whoops, typo in the other patches landed then, nothing wrong with this patch. Relanded: https://hg.mozilla.org/integration/mozilla-inbound/rev/1268d562bda0
Flags: needinfo?(jwalden+bmo)
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: