565604 - "Assertion failure: getClass()->flags & JSCLASS_HAS_PRIVATE" after setting __proto__ of a WebGLIntArray

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

I initially hit this in the browser, but these testcases work in xpcshell.

Testcase 1:
o = {}; o.__proto__ = WebGLIntArray(1); o.length

Testcase 2:
o = Components.interfaces.nsIAtom; o.__proto__ = WebGLIntArray(1); o.length

Testcase 1 reliably produces
Assertion failure: getClass()->flags & JSCLASS_HAS_PRIVATE, at js/src/jsobj.h:391

Testcase 2 gives strange numbers (indicating a type safety violation) or
Assertion failure: INT_FITS_IN_JSVAL(i), at js/src/jsapi.h:252

Comment 1

[Damon Sicore (:damons)]

Waldo, any luck here?

Comment 2

[Jeff Walden [:Waldo]]

Just a simple failure to class-check inside a property getter, due to the usual problem of a class-requiring getter being invoked on a different object through the wonders of the prototype chain.  Easy enough to fix, should have a patch tomorrow.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch and test

The first instance is with a class without a private, and getPrivate() asserts privacy.  The second instance is with a class *with* a private, and we cast it to something bogus and get something bogus back.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 446311 [details] [diff] [review]
Patch and test

Looks fine to me, though I wrote the bug in the first place so maybe i'm not the best judge :-)  Do any changes need to happen on the tracing side?

Also you'll have to do a bit of merging in the CustomQS_WebGL side of things -- two of those functions (BufferData and TexImage2D) can actually accept a null data argument; I just landed a patch that enables/fixes that.

Comment 5

[Jeff Walden [:Waldo]]

I don't believe any tracing changes are necessary.  This is basically applying the treatment in array_length_getter to typed arrays, and I'm aware of no issues with that.

The good side of this bug, so far as it exists, is that none of the affected properties do anything but read memory.  Thus the worst that can happen is a crash or a read from an arbitrarily choosable memory location (arbitrary because there's no private slot in objects without privates, as Shaver so astutely reminds me, so the "private slot" value is user-controllable) -- information and memory disclosure, but no code execution or anything worse.

I made the necessary tweaks but ran out of time to push today, will do tomorrow.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

note that we never shipped this code anywhere except maybe a trunk alpha.

Comment 7

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/mozilla-central/rev/f9eb2652b13d

Actually, I was wrong about this only being sg:moderate -- I forgot that one of the accessors is .buffer which returns an object whose contents are then arbitrarily choosable.  I discovered this when I added in tests for the other properties (since .length goes only through the very specialized JSOP_LENGTH path) and discovered a truly egregious typo in the implementation of .byteOffset which pointed out the object-valued property -- fixed in the landing.

I tend to hold trunk-only bugs closed for a little while after fix (to get most nightly users upgraded) plus the next alpha and a week-ish (to catch beta-channeling users).  Past that it's of course better to open it up.  This has been in since the original typed-array landing, so it's certainly in at least one or two alphas and thus needs some delay for straggling beta users to upgrade.