Closed Bug 752087 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla15
Tracking Status
firefox15 --- fixed
firefox-esr10 - ---

People

(Reporter: bc, Assigned: jesup)

References

()

Details

(Keywords: crash, Whiteboard: [inbound][advisory-tracking+])

Crash Data

Attachments

(2 files, 4 obsolete files)

Attached file 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.
(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.
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.
Assignee: general → nobody
Blocks: 664918
Component: JavaScript Engine → Video/Audio
QA Contact: general → video.audio
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.
Attachment #621387 - Flags: review?(roc)
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!
Attachment #621387 - Flags: review?(roc) → review+
Attachment #621387 - Attachment is obsolete: true
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
Attachment #621535 - Flags: review?(roc)
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
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.
Attachment #621535 - Attachment is obsolete: true
Attachment #621535 - Flags: review?(roc)
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
Attachment #621565 - Flags: review?(roc)
Attachment #621565 - Attachment is obsolete: true
Attachment #621565 - Flags: review?(roc)
Attachment #621580 - Attachment is obsolete: true
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.
Attachment #621581 - Flags: review?(roc)
Oh, and as this does appear to be a null-deref only, should we mark it as non-ss?
Let's wait until you land this and I can check bug 752340 to see if it is fixed by this patch.
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.
Assignee: nobody → rjesup
Whiteboard: [inbound]
Duplicate of this bug: 752826
https://hg.mozilla.org/mozilla-central/rev/c77b4c6085c1
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Marking as verified fixed based on Bob Clary's testing of post-patch build with same URLs.
Status: RESOLVED → VERIFIED
This doesn't affect ESR and isn't a known regression?
Whiteboard: [inbound] → [inbound][advisory-tracking+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.