872774 - Crash with expando on SVGRect

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Debug:    Assertion failure: ccISupports, at nsContentUtils.h:1288
 
Nightly:  Crash [@ mozilla::dom::SVGRectBinding::_addProperty]
          bp-a9ffdf10-268f-47d7-b677-f6fa82130515

Likely a regression from something in:
http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=08be63954b6b
Perhaps bug 853386 or bug 866796

Comment 1

[Jesse Ruderman]

Attachment: assertion stack (gdb)

Comment 2

[David Zbarsky (:dzbarsky)]

Do we have cycle collection for SVGIRect?

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: Patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 750172 [details] [diff] [review]
Patch

r=me, and please nominate for branches as needed.

That said, why is SVGIRect itself not cced?  Seems to me like that would be preferable, since it's the thing that inherits from nsWrapperCache...

Comment 5

[Andrew McCreight [:mccr8]]

Thanks for the quick fix!

> That said, why is SVGIRect itself not cced?  Seems to me like that would be preferable, since it's the thing that inherits from nsWrapperCache...

The general approach that is taken is that "abstract" classes like nsIFoo never have CC in them, even if they have fields that need to be CCed.  Instead, any "concrete" subclasses nsFoo implement CC.  Probably not a great approach for a class like this that has multiple inheritors.

Anyways, we're never tracing the wrapper cache for these classes, so we can end up touching dead memory, so I'm marking this sec-crit.

This is on Aurora and Nightly, so fill out the sec-approval form (under patch details) before landing on inbound, please.

Comment 6

[Andrew McCreight [:mccr8]]

I'll do an audit of the CC goop for bug 866796 tomorrow.

Comment 7

[David Zbarsky (:dzbarsky)]

Comment on attachment 750172 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 866796
User impact if declined: crashes, possibly exploitable
Testing completed (on m-c, etc.): fixes crash
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 750172 [details] [diff] [review]
Patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch? probably not too difficult to somebody who understand it

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? no

Which older supported branches are affected by this flaw? Aurora

If not all supported branches, which bug introduced the flaw? I marked the dependency

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? should be the same

How likely is this patch to cause regressions; how much testing does it need?  It looks fairly safe to me.

Comment 9

[Andrew McCreight [:mccr8]]

Please wait for sec-approval+ before landing on inbound.

Comment 10

[Jesse Ruderman]

Does this patch also fix bug 872790 and/or bug 872812?

Comment 11

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

Comment on attachment 750172 [details] [diff] [review]
Patch

sec-approval+ for m-c. Please nominate for Aurora.

Comment 12

[David Zbarsky (:dzbarsky)]

(In reply to Jesse Ruderman from comment #10)
> Does this patch also fix bug 872790 and/or bug 872812?

I can't reproduce bug 872790 with this patch.  bug 872812 still exists.

Comment 13

[David Zbarsky (:dzbarsky)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e250ccb7a9b