1696886 - Assertion failure: [barrier verifier] Unmarked edge: JS Shape 0x173ac18b8a80 'setter' edge to JS Object 0x173ac18b7900, at gc/Verifier.cpp:387

Status: VERIFIED FIXED

(Core :: JavaScript: GC, defect, P1)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 20210307-74e3d611be8f (debug build, run with --fuzzing-safe --ion-offthread-compile=off):

try { evaluate(`
  for (let v3 = 0; v3 < 256; v3++)
    x1 = this.__defineSetter__("x", function(z69) { })
  const v9 = {};
  const v10 = v9[gczeal(4)];
`); } catch(exc) {}
for (let v3 = 0; v3 < 256; v3++)
  x1 = this.__defineSetter__("x", function(z69) {})

Backtrace:

received signal SIGSEGV, Segmentation fault.
0x00005555574b0f54 in js::gc::GCRuntime::endVerifyPreBarriers() ()
#0  0x00005555574b0f54 in js::gc::GCRuntime::endVerifyPreBarriers() ()
#1  0x000055555740ce94 in js::gc::GCRuntime::collect(bool, js::SliceBudget const&, mozilla::Maybe<JSGCInvocationKind> const&, JS::GCReason) ()
#2  0x00005555573c52d3 in js::gc::GCRuntime::gc(JSGCInvocationKind, JS::GCReason) ()
#3  0x0000555556f16ecb in JSRuntime::destroyRuntime() ()
#4  0x0000555556e51a18 in js::DestroyContext(JSContext*) ()
#5  0x00005555569f3f54 in main ()
rax	0x555555718574	93824994084212
rbx	0x555555821e65	93824995171941
rcx	0x555557fdb3f8	93825036825592
rdx	0x0	0
rsi	0x7ffff7105770	140737338431344
rdi	0x7ffff7104540	140737338426688
rbp	0x7fffffffdbd0	140737488346064
rsp	0x7fffffffd700	140737488344832
r8	0x7ffff7105770	140737338431344
r9	0x7ffff7f99840	140737353717824
r10	0x0	0
r11	0x0	0
r12	0x55555586c5c9	93824995476937
r13	0x7ffff0937b70	140737229585264
r14	0x7fffffffd780	140737488344960
r15	0x5555557fb5b8	93824995014072
rip	0x5555574b0f54 <js::gc::GCRuntime::endVerifyPreBarriers()+1892>
=> 0x5555574b0f54 <_ZN2js2gc9GCRuntime20endVerifyPreBarriersEv+1892>:	movl   $0x184,0x0
   0x5555574b0f5f <_ZN2js2gc9GCRuntime20endVerifyPreBarriersEv+1903>:	callq  0x555556a7d8ac <abort>

GC issue, so marking s-s as a start. Might be related to verifier only, in which case we should be fine but I prefer to wait until this is triaged by JS devs.

Comment 1

[Christian Holler (:decoder)]

Attachment: Testcase

Comment 2

[Jan de Mooij [:jandem]]

NI myself in case this is from the ObjectGroup removal patches.

Comment 3

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #2)

NI myself in case this is from the ObjectGroup removal patches.

I can reproduce this on the revision before that landed...

Comment 4

[Jon Coppeard (:jonco)]

Bisecting points at bug 1694209.

Comment 5

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as reproducible on mozilla-central 20210308094833-31551f880fc3.
The bug appears to have been introduced in the following build range:

Start: 8994342c7c86cda3b90a5c66aa5968fe9de32bf6 (20210301115638)
End: 678d1355789d1891e6a3a8530a0ce8240613683a (20210301140308)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8994342c7c86cda3b90a5c66aa5968fe9de32bf6&tochange=678d1355789d1891e6a3a8530a0ce8240613683a

Comment 6

[Jon Coppeard (:jonco)]

Attachment: Bug 1696886 - Add a write barrier when updating accessor shapes' getters and setters r?jandem

I'm not sure why this wasn't found before, but it seems there's no pre-write barrier when changing the getter/setter objects. It's debateable whether this is required because the shape is newly created here and the getters/setters haven't had a chance to eacape by this point, but it seems safest just to add the barrier.

After adding the barrier it bacame aparrent we call the Shape consturctor for accessor shapes allocated by NativeObject::replaceWithNewEquivalentShape. I think this is UB. We at least need to initialize getters/setters for the barrier to work.

Comment 7

[Steve Fink [:sfink] [:s:]]

This sounds familiar. I have a dim memory of looking at that code and deciding that it was an initialization and so did not need the barrier. But I may be thinking of something else, because at the very least the current code does not make it obvious that this would be an initialization.

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1694209

Comment 9

[Andrew McCreight [:mccr8]]

Does "I'm not sure why this wasn't found before" mean that this needs to be backported to an earlier branch?

Comment 10

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
I'm not sure why our fuzzing didn't pick this up sooner. Certainly it's not a new issue, however...

I don't think this is actually a problem, because of the way marking currently works. What happens in this test is:

1. Root existing shape with attached setter object
2. Remove shape from GC graph (OK, with barrier)
3. The root marking phase of an incremental GC runs
4. Replace setter object on shape (bug, no barrier!)

The verifier is complaining because we don't mark the previous setter object when it's removed in stage 4. But because we eagerly mark the children of shapes we have actually already marked this during root marking in stage 3. This is an implementation detail and we shouldn't be depending on it, but it means that this is not actually a problem right now.

We will add the barrier in case this changes, but this doesn't need backporting.

Comment 11

[Jon Coppeard (:jonco)]

Downgrading to sec-low as I don't think this is exploitable.

Comment 12

[Andrew McCreight [:mccr8]]

Thanks for the explanation. I'll mark older branches as "wontfix" then.

Comment 13

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

Add a write barrier when updating accessor shapes' getters and setters r=jandem
https://hg.mozilla.org/integration/autoland/rev/5d238495d2351b9db9cbbadc284ff28040008bd2
https://hg.mozilla.org/mozilla-central/rev/5d238495d235

Comment 14

[Bugmon [:jkratzer for issues]]

Bugmon Analysis:
Verified bug as fixed on rev mozilla-central 20210311220018-fe11dc32ac20.
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.