752087 - Crash [@ js::GetObjectClass(JSObject const *)] | [@ nsXPConnect::GetNativeOfWrapper]

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: Debug Crash Report

http://us.blizzard.com/en-us/games/music/sc2-wings-of-liberty.html
http://www.ohmstudio.com/

###!!! ASSERTION: bad param: 'aJSObj', file /work/mozilla/builds/nightly/mozilla/js/xpconnect/src/nsXPConnect.cpp, line 1429
###!!! ASSERTION: bad param: 'obj', file /work/mozilla/builds/nightly/mozilla/js/xpconnect/src/XPCWrappedNative.cpp, line 1786

Nightly Debug Crash @ js::GetObjectClass(JSObject const *)
Nightly Opt Crash @ nsXPConnect::GetNativeOfWrapper

bp-412995f0-045d-485c-92aa-daf952120504
bp-c96d3eee-924d-41d4-8a20-7e6832120504

Marking s-s since bug 729812 is s-s, but this is a crash at null. This may be a dupe of that though.

Comment 1

[Christian Holler (:decoder)]

(In reply to Bob Clary [:bc:] from comment #0)

> Marking s-s since bug 729812 is s-s, but this is a crash at null. This may
> be a dupe of that though.

That other bug is on the IonMonkey branch, so it's unlikely that this is the same issue.

Comment 2

[Boris Zbarsky [:bzbarsky]]

This is actually a media bug.  nsHTMLMediaElement::SetSrc has:

482   if (JSVAL_IS_OBJECT(aParams)) {
483     nsCOMPtr<nsIDOMMediaStream> stream;
484     stream = do_QueryInterface(nsContentUtils::XPConnect()->
485         GetNativeOfWrapper(aCtx, JSVAL_TO_OBJECT(aParams)));

But JSVAL_IS_OBJECT is true for null (in which case JSVAL_TO_OBJECT will return a null JSObject*).  This should presumably be testing !JSVAL_IS_PRIMITIVE, or better yet aParams.isObject() (which excludes null).

I don't think this is exploitable; it's a guaranteed null-deref.  Probably completely unrelated to bug 729812.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Use correct method to check is a jsval is an object

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 621387 [details] [diff] [review]
Use correct method to check is a jsval is an object

Patch per bz's analysis (I verified the bug in GDB, and that the fix corrects it)

Full try build pushed (against inbound):
https://tbpl.mozilla.org/?tree=Try&rev=21b02e25bdb4

Methinks these functions/defines aren't quite properly named...

Before landing we should add a test as well.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 621387 [details] [diff] [review]
Use correct method to check is a jsval is an object

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

Thank you!

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Use correct method to check is a jsval is an object and tests

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 621535 [details] [diff] [review]
Use correct method to check is a jsval is an object and tests

Just need review on the test; no change to code.  Verified without the patch we crash, with the patch we work (loading the test directly from a file).

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=b7f96aebd7f8

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 621535 [details] [diff] [review]
Use correct method to check is a jsval is an object and tests

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

You need to hg add test_source_null.html

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Weird, I did add it.

It may have been confused by a qpop (to check failure before)/qpush combo; the push failed to apply the test file I added (generated a .rej file) because it existed (to test without the code patch).  But I hg qref'd after that, I figured that would resolve it.  Apparently it decided to resolve it by removing the file...  Sigh.   Should have looked more closely at the list-of-files qref dumps - it was late here.

Update in a sec.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Use correct method to check is a jsval is an object and tests

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 621565 [details] [diff] [review]
Use correct method to check is a jsval is an object and tests

Test is there now.

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=7f5a2c571cb5

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Use correct method to check is a jsval is an object and tests (minor update)

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Use correct method to check is a jsval is an object and tests (minor update)

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 621581 [details] [diff] [review]
Use correct method to check is a jsval is an object and tests (minor update)

Ok, you need ok() in the test infrastructure (which is why I run try...)  :-)

New try build at https://tbpl.mozilla.org/?tree=Try&rev=77874b64b6da

Note there's a bug causing a lot of random reds....  If we don't feel we get good try results, we may need to re-trigger.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Oh, and as this does appear to be a null-deref only, should we mark it as non-ss?

Comment 16

[Bob Clary [:bc] (inactive)]

Let's wait until you land this and I can check bug 752340 to see if it is fixed by this patch.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

The try build (while fighting an infrastructure Error 500 bug that's hitting everyone right now) got multiple successful runs of the new test on different platforms.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/c77b4c6085c1

Comment 20

[Ed Morley [:emorley]]

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

Comment 23

[Al Billings [:abillings - ex-MoCo]]

Marking as verified fixed based on Bob Clary's testing of post-patch build with same URLs.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

This doesn't affect ESR and isn't a known regression?