757199 - Assertion failure: [barrier verifier] Unmarked edge: shape, at jsgc.cpp:4443

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

Attachment: Test case for shell (run with -n -m -a)

The attached test asserts on mozilla-central revision d55df2c9c037 (options -m -n -a). S-s due to GC-related assert with possible security impact.

Comment 1

[Christian Holler (:decoder)]

Still reproduces on rev 73783bf75c4c, with the new verifier assertion format now.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fix

This is a pretty fundamental problem, although unlikely to hurt us in practice. The barrier code for JSOP_SETPROP assumes that only the slot being updated needs a write barrier. However, when we add new properties, we're also changing the object's shape. So that also needs a barrier.

In theory we could call out to a little stub routine that marks the shape. However, I don't think that's worth it given how close IonMonkey is. This patch just takes the slow path if we need to add a property and if incremental GC is running.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 632453 [details] [diff] [review]
fix

This should be done by disabling the cache if doing an addprop with barriers on, rather than generating stubs which are guaranteed to fail.  Otherwise we can keep hitting the same ADDPROP case in the slow path, and chain together a bunch of auto-failing stubs.  Look for the 'generateStub(initialShape, shape, true)' call for places where the cache can be disabled in the ADDPROP case.

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v2

Is this what you mean?

Comment 5

[Brian Hackett [Laid off!]]

Almost, the cache is being disabled but that should happen before we start trying to generate a stub for it.  If you put this test in the update() method, near the test below:

            if (obj->numDynamicSlots() != slots)
                return disable("insufficient slot capacity");

That should work fine.

Comment 6

[David Mandelin [:dmandelin]]

Can you guys put a security rating on this?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch v3

I'm 99% certain that this is harmless. The danger would be if the old shape never got marked. I just don't see how that could happen, though, since the new shape's parent pointer should always point to the old one.

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Helped land this since this was bothering the fuzzers:

http://hg.mozilla.org/integration/mozilla-inbound/rev/431fa10c63a6

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Backed out for orange:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=431fa10c63a6

https://hg.mozilla.org/integration/mozilla-inbound/rev/77e2d6789e02

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Orange seems to related to verifybarriers not being defined?

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d150934320be

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/bug757199.js.