Closed Bug 872774 Opened 6 years ago Closed 6 years ago

Crash with expando on SVGRect

Categories

(Core :: SVG, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 --- unaffected
firefox23 + fixed
firefox24 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: dzbarsky)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [adv-main23-])

Attachments

(3 files)

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
Attached file assertion stack (gdb)
Do we have cycle collection for SVGIRect?
Flags: needinfo?(amarchesini)
Attached patch PatchSplinter Review
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #750172 - Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
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...
Attachment #750172 - Flags: review?(bzbarsky) → review+
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.
Blocks: 866796
I'll do an audit of the CC goop for bug 866796 tomorrow.
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
Attachment #750172 - Flags: approval-mozilla-aurora?
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.
Attachment #750172 - Flags: sec-approval?
Please wait for sec-approval+ before landing on inbound.
Does this patch also fix bug 872790 and/or bug 872812?
Comment on attachment 750172 [details] [diff] [review]
Patch

sec-approval+ for m-c. Please nominate for Aurora.
Attachment #750172 - Flags: sec-approval? → sec-approval+
(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.
https://hg.mozilla.org/mozilla-central/rev/c5a16f9876e5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Attachment #750172 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 872790
Whiteboard: [adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.