Assertion failure: *thingp, at gc/Marking.cpp:166

RESOLVED FIXED in Firefox 35

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: decoder, Assigned: Waldo)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla36
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [jsbugmon:update,bisect])

Attachments

(2 attachments)

The following testcase asserts on mozilla-central revision 54217864bae9 (run with --no-threads --fuzzing-safe):


function test() {
Object.defineProperty(Object.prototype, "__proto__", { set: function() {}});
Object.defineProperty(Object.prototype, "__proto__", { set: undefined });
} test();
Marked s-s because it's GC-related.
Whiteboard: [jsbugmon:update,bisect]
Better testcase:

  Object.defineProperty(Object.prototype, "x",
                        { get() { return 2; },
                          set(v) { },
                          configurable: true,
                          enumerable: false });
  function test()
  {
    Object.defineProperty(Object.prototype, "x", { set: function() {}});
    Object.defineProperty(Object.prototype, "x", { set: undefined });
  }
  test();

__proto__ has no involvement in this issue at all, so best not to mention it.

On the other hand, Object.prototype has *some* involvement.  Switching to a new object and defining on that doesn't have issues.  Using a new object, that's made into a delegate (by creating another object using it as [[Prototype]], also doesn't have issues.  I imagine whatever it is that makes Object.prototype special will show up in the course of debugging.

I'm quite curious why this issue was never caught by the exhaustive Object.defineProperty tests we have, that everyone loves to hate.  :-)  Perhaps because three steps of definition are required, and those tests only (as I recall) go to two?
This looks, by my hazy understanding of the crash spot, like a barrier issue.  I don't know enough about GC and barriers.  Time to rectify.  Taking.
Assignee: nobody → jwalden+bmo
This bug was, I think, introduced between attachment 8500531 [details] [diff] [review] and attachment 8501596 [details] [diff] [review].  In the initial state, GetterSetterWriteBarrierPost used putRelocatableCell.  The corresponding buffer entry will return if the edge contains null (CellPtrEdge::mark).  The post-state uses putGeneric with ShapeGetterSetterRef, but ShapeGetterSetterRef::mark has no such early return if the edge contains null.  Add it here to fix things.

I...think this is how barriers are supposed to be used, but honestly I've never written a barrier patch before or really taken time to grok barriers, so this could be a wrong tack, or not entirely the right one.  :-)  I could also imagine something like if the new setter/getter is null, removing the store buffer entry entirely.  But then you can potentially have thrashing if someone mutates back and forth between undefined and callable for [[Get]] or [[Set]], so might as well not bother.  Or so I am hazily claiming based on my weak barrier-fu.

This being introduced in bug 1073700, it'll require uplift.  If I continue reading through MarkInternal it looks like we'll just null-deref at a fixed offset when computing |thing->zone()|, so I think this is not a security issue.
Attachment #8505034 - Flags: review?(terrence)
Blocks: 1073700
My bad, thanks for fixing this Waldo!
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> I'm quite curious why this issue was never caught by the exhaustive
> Object.defineProperty tests we have, that everyone loves to hate.  :-) 

(I also used to hate those tests because they used to (?) timeout and are not easy to debug when they fail, but when I worked on bug 1073700 they found a few issues that no other test hit, so I appreciate them now ;)
Comment on attachment 8505034 [details] [diff] [review]
Don't crash trying to mark a dictionary accessor shape whose getter/setter field has previously been mutated from a callable to |undefined|.  NOT REVIEWED YET

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

Yes, this is the right solution.
Attachment #8505034 - Flags: review?(terrence) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbde56396273

Unhiding as just a null-deref as far as I can tell...flagging for aurora approval momentarily.
Group: core-security
Comment on attachment 8505034 [details] [diff] [review]
Don't crash trying to mark a dictionary accessor shape whose getter/setter field has previously been mutated from a callable to |undefined|.  NOT REVIEWED YET

Approval Request Comment
[Feature/regressing bug #]: bug 1073700
[User impact if declined]: crashes with certain manipulations of properties backed by getter/setter functions; semi-unlikely to be too common in the real world where getters/setters aren't changed much after creation, but someone probably does it, and it's early in cycle and a simple, clear fix
[Describe test coverage new/current, TBPL]: landed with test, narrowed down to the exact failing scenario pretty clearly
[Risks and why]: a null-check with early return is about as little risk as you can get, plus it's early
[String/UUID change made/needed]: N/A
Attachment #8505034 - Flags: approval-mozilla-aurora?
(In reply to Jan de Mooij [:jandem] from comment #7)
> (I also used to hate those tests because they used to (?) timeout and are
> not easy to debug when they fail, but when I worked on bug 1073700 they
> found a few issues that no other test hit, so I appreciate them now ;)

I am going to frame this and place it on a wall.
https://hg.mozilla.org/mozilla-central/rev/cbde56396273
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8505034 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.