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

VERIFIED FIXED in Firefox 62

Status

()

defect
--
critical
VERIFIED FIXED
a year ago
a day ago

People

(Reporter: decoder, Assigned: Waldo)

Tracking

(Blocks 1 bug, 6 keywords)

Trunk
mozilla62
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61 unaffected, firefox62 verified)

Details

(Whiteboard: [jsbugmon:update], crash signature)

Attachments

(2 attachments)

Reporter

Description

a year ago
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.

Updated

a year ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]

Comment 1

a year ago
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.
Assignee

Comment 2

a year ago
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
Assignee

Comment 3

a year ago
Posted patch PatchSplinter Review
Sigh.
Attachment #8980904 - Flags: review?(jdemooij)
Assignee

Comment 4

a year ago
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+
Assignee

Comment 7

a year ago
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.
Assignee

Comment 9

a year ago
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)
https://hg.mozilla.org/mozilla-central/rev/1268d562bda0
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

Updated

a year ago
Status: RESOLVED → VERIFIED

Comment 11

a year ago
JSBugMon: This bug has been automatically verified fixed.
Group: core-security-release

Comment 12

11 months ago
Pushed by jwalden@mit.edu:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8add80f35d02
Add some comments and a test.  r=jandem
You need to log in before you can comment on or make changes to this bug.