1462939 - Crash [@ MOZ_CrashPrintf] or Crash [@ js::ObjectGroup::sweep] or Assertion failure: MapTypeToTraceKind<typename mozilla::RemovePointer<T>::Type>::kind == thing->getTraceKind(), at gc/Marking.cpp:233

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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

var o = {
    p: 1,
    r: 3,
    s: 4,
};
for (var i in o) {
    gc();
    delete o.s;
}


Backtrace:

received signal SIGSEGV, Segmentation fault.
MOZ_CrashPrintf (aLine=aLine@entry=2613, aFormat=aFormat@entry=0xf404d0 "Got 0x%lx expected magic word 0x%lx flags 0x%lx objectSet 0x%lx") at mfbt/Assertions.cpp:63
#0  MOZ_CrashPrintf (aLine=aLine@entry=2613, aFormat=aFormat@entry=0xf404d0 "Got 0x%lx expected magic word 0x%lx flags 0x%lx objectSet 0x%lx") at mfbt/Assertions.cpp:63
#1  0x0000000000a68de3 in js::ReportMagicWordFailure (actual=<optimized out>, expected=expected@entry=11647069175327152090, flags=<optimized out>, objectSet=<optimized out>) at js/src/vm/TypeInference.cpp:2611
#2  0x0000000000c37a77 in js::ConstraintTypeSet::checkMagic (this=0x1ea1c88 <js::PropertyIteratorObject::class_+8>) at js/src/vm/TypeInference.h:739
#3  js::ObjectGroup::getProperty (sweep=..., i=<optimized out>, this=0x7ffff4ca2128) at js/src/vm/TypeInference-inl.h:1211
#4  js::GCMarker::lazilyMarkChildren (this=this@entry=0x7ffff5f1a098, group=0x7ffff4ca2128) at js/src/gc/Marking.cpp:1522
#5  0x0000000000c4c7ad in js::GCMarker::processMarkStackTop (this=this@entry=0x7ffff5f1a098, budget=...) at js/src/gc/Marking.cpp:1752
#6  0x0000000000c3cf37 in js::GCMarker::drainMarkStack (this=this@entry=0x7ffff5f1a098, budget=...) at js/src/gc/Marking.cpp:1651
#7  0x0000000000bf05d0 in js::gc::GCRuntime::drainMarkStack (this=0x7ffff5f194b0, sliceBudget=..., phase=<optimized out>) at js/src/gc/GC.cpp:5903
#8  0x0000000000c0e45a in js::gc::GCRuntime::incrementalCollectSlice (this=this@entry=0x7ffff5f194b0, budget=..., reason=reason@entry=JS::gcreason::API, session=...) at js/src/gc/GC.cpp:7116
#9  0x0000000000c0f427 in js::gc::GCRuntime::gcCycle (this=this@entry=0x7ffff5f194b0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:7478
#10 0x0000000000c0f9ad in js::gc::GCRuntime::collect (this=this@entry=0x7ffff5f194b0, nonincrementalByAPI=nonincrementalByAPI@entry=true, budget=..., reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:7621
#11 0x0000000000c0fc78 in js::gc::GCRuntime::gc (this=0x7ffff5f194b0, gckind=<optimized out>, reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:7691
#12 0x0000000000c0fcb0 in JS::GCForReason (cx=cx@entry=0x7ffff5f14000, gckind=<optimized out>, reason=reason@entry=JS::gcreason::API) at js/src/gc/GC.cpp:8531
#13 0x000000000079338a in GC (cx=0x7ffff5f14000, argc=<optimized out>, vp=0x7ffff49eb0a0) at js/src/builtin/TestingFunctions.cpp:351
#14 0x00000000005657b1 in js::CallJSNative (args=..., native=0x7932a0 <GC(JSContext*, unsigned int, JS::Value*)>, cx=0x7ffff5f14000) at js/src/vm/JSContext-inl.h:280
[...]
#27 main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at js/src/shell/js.cpp:9330
rax	0x1ec8660	32278112
rbx	0x7ffff5f1a098	140737319641240
rcx	0x7fffffad	2147483565
rdx	0x1ec8712	32278290
rsi	0x0	0
rdi	0x1ec86c0	32278208
rbp	0x1ea1c80 <js::PropertyIteratorObject::class_>
rsp	0x7fffffffc800	140737488340992
r8	0x0	0
r9	0x52	82
r10	0x0	0
r11	0xfffffffffffffffa	-6
r12	0x0	0
r13	0x7ffff4ca2128	140737300275496
r14	0x7ff8	32760
r15	0xa1a2b3b4c5c6d7da	-6799674898382399526
rip	0x430bcb <MOZ_CrashPrintf(int, char const*, ...)+243>
=> 0x430bcb <MOZ_CrashPrintf(int, char const*, ...)+243>:	movl   $0x0,0x0
   0x430bd6 <MOZ_CrashPrintf(int, char const*, ...)+254>:	ud2


This crashes in various ways depending on 32 or 64-bit build, sometimes asserts with magic word failure, in debug builds shows a type mismatch GC assert. Marking s-s and sec-high.

Comment 1

[Christian Holler (:decoder)]

This can also show up as:

Assertion failure: [barrier verifier] Unmarked edge: Object 0xf59610e0 'receiver_guard_shape' edge to Shape 0xf597a7f0, at gc/Verifier.cpp:384

Comment 2

[Fuzzing Team]

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/7d83c7de9404
user:        Jeff Walden
date:        Wed May 16 23:24:28 2018 -0700
summary:     Bug 1462540 - Remove NativeIterator::guard_array: its numeric value is identical to NativeIterator::props_end.  r=jandem

This iteration took 244.659 seconds to run.

Comment 3

[Christian Holler (:decoder)]

Also seeing:

Assertion failure: !overlay->isForwarded(), at gc/Marking-inl.h:41


Needinfo from Jeff based on comment 2. Also marking fuzzblocker due to the high amount of different signatures caused by this.

Comment 4

[Alex Gaynor [:Alex_Gaynor]]

FYI, OSS-Fuzz has also discovered this, which attaches a 90 day disclosure deadline to it.

Comment 5

[Jeff Walden [:Waldo]]

Good to know.  This should be fixable far before then.

Also: hrm.  This was very carefully written to avoid such issues, so it will be interesting to see what's up here.

Comment 6

[Jeff Walden [:Waldo]]

Bah, I am a dumb.

Property deletion causes NativeIterator::props_end to be decremented, which means guardArray() starts pointing into zeroed-out GCPtrFlatStrings and mis-offsetting which causes (in this case, with a property deletions) an ObjectGroup* to be interpreted as a Shape* and so that gets you the trace-kind mismatch from the assertion.

Readding the field is the stupidest fix, but obviously it's better to not have the extra field.  What if we preserved the lost-field by flipping the trailer fields after the NativeIterator?  That is, instead of having


  [NativeIterator fields][prop0][prop1]...[propM-1][guard0][guard1]...[guardN-1]

we had

  [NativeIterator fields][guard0][guard1]...[guardM-1][prop0][prop1]...[propN-1]

?  NativeIterator::guard_length is only mutated during NativeIterator creation, now, so we could basically just swap props/guard in all the fields and functions and the current broken design would be fixed.  This would slow down begin() because it'd have to do a pointer-load in |this->guards_end|, but it's only called in somewhat rare-ish places -- and I can't find any in JITted code, where |this->props_cursor| is already a necessary pointer-load -- so marginally slower perf seems unimportant.  And if we changed how props are accessed to have a index/length to access them, deleting a property would just be decrementing the length, not doing |this->props_end - 1|, so it seems like a perf wash.

Is there anything in this that seems un-sensible?  I don't see clear, new complexity to making the change, but I'd rather get a seems-right before tackling.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Patch that reorders guards to precede property strings and has pointer fields for guards-end, property-cursor, properties-end

I went ahead and did comment 6.  The renaming is a combination of style cleanup and forcing function to be sure I touched everything that could be affected by this.  (I could style-fix the rest of this struct in a followup patch if desired, but I wanted to not grow this too much.)

As far as size-consumption goes, we change from

  ...
  JSString* props_cursor;
  JSString* props_end;
  uint32_t guard_length, guard_key, flags;
  NativeIterator* next_;
  ...

which including next_ (because it forces extra alignment on 64-bit) is 6 uint32_t on 32-bit, 10 uint32_t on 64-bit, to

  HeapReceiverGuard* guardsEnd_;
  JSString* propertyCursor_;
  JSString* propertiesEnd_;
  uint32_t guard_key, flags;
  NativeIterator* next_;
  ...

which including next_ is 6 uint32_t on 32-bit, 10 uint32_t on 64-bit.  So, no size-regression in this patch.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Test

The 1-deletion variant is the only one that crashes.  1 gets you the offset that mis-interprets a Shape* as ObjectGroup*.  But if you have 2 or 3 or presumably more, you're going to just hit a mismatch and nothing will happen.  Or so it appears at casual glance -- the extra tests are because why not, not because anything breaks if you run them.

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8979773 [details] [diff] [review]
Patch that reorders guards to precede property strings and has pointer fields for guards-end, property-cursor, properties-end

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

Gah, I wish I had thought of the property suppression code messing with the properties list.

::: js/src/vm/Iteration.cpp
@@ +63,5 @@
>      // GC removes any elements from the list, it won't remove this one.
>      if (iterObj_)
>          TraceManuallyBarrieredEdge(trc, &iterObj_, "iterObj");
> +
> +    std::for_each(guardsBegin(), guardsEnd(),

Fancy

::: js/src/vm/Iteration.h
@@ +50,5 @@
> +    // The next property, pointing into an array of strings directly after any
> +    // HeapReceiverGuards that appear directly after |*this|, as part of an
> +    // overall allocation that stores |*this|, receiver guards, and iterated
> +    // strings.
> +    GCPtrFlatString* propertyCursor_; // initialized by constructor

Nit: for these public fields I'd prefer either making them private and adding accessors, or dropping the trailing _. That would also be consistent with {obj, guard_key, flags}.

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jeff Walden [:Waldo] from comment #7)
> (I could style-fix the rest of this struct in a followup
> patch if desired, but I wanted to not grow this too much.)

I missed this part. That's fine, also considering [fuzzblocker] status.

Comment 11

[Jeff Walden [:Waldo]]

Kicked off a try-run, will land once that looks good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1295b639a833b12b2084a3d3cc318261da77ccf

In light of comment 10, I didn't bother changing anything style-wise for comment 9's nit.  Will get to after this has landed in a separate bug.

Also per my usual practice, I'll hold off landing the test for a week or so as nicety to nightly users, even if technically I could just land it with the patch because nightly-only.

Comment 12

[Jeff Walden [:Waldo]]

Landed the patch:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7c84e815c0c2eef81ac9cf8f5fa94b5a416029

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6f7c84e815c0

Comment 14

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4299313fa51
Add a test.  r=jandem

Comment 16

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/b4299313fa51