Closed
Bug 872774
Opened 12 years ago
Closed 12 years ago
Crash with expando on SVGRect
Categories
(Core :: SVG, defect)
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
Details
(5 keywords, Whiteboard: [adv-main23-])
Attachments
(3 files)
(deleted),
image/svg+xml
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
lsblakk
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•12 years ago
|
||
Assignee | ||
Comment 2•12 years ago
|
||
Do we have cycle collection for SVGIRect?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 3•12 years ago
|
||
Assignee: nobody → dzbarsky
Status: NEW → ASSIGNED
Attachment #750172 -
Flags: review?(bzbarsky)
Flags: needinfo?(amarchesini)
Comment 4•12 years ago
|
||
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+
Comment 5•12 years ago
|
||
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
Updated•12 years ago
|
status-b2g18:
--- → unaffected
status-firefox21:
--- → unaffected
status-firefox22:
--- → unaffected
status-firefox23:
--- → affected
status-firefox24:
--- → affected
status-firefox-esr17:
--- → unaffected
tracking-firefox23:
--- → ?
tracking-firefox24:
--- → ?
Updated•12 years ago
|
Keywords: sec-critical
Comment 6•12 years ago
|
||
I'll do an audit of the CC goop for bug 866796 tomorrow.
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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?
Comment 9•12 years ago
|
||
Please wait for sec-approval+ before landing on inbound.
Reporter | ||
Comment 10•12 years ago
|
||
Does this patch also fix bug 872790 and/or bug 872812?
Comment 11•12 years ago
|
||
Comment on attachment 750172 [details] [diff] [review]
Patch
sec-approval+ for m-c. Please nominate for Aurora.
Attachment #750172 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Assignee | ||
Comment 13•12 years ago
|
||
Updated•12 years ago
|
Comment 14•12 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•11 years ago
|
Attachment #750172 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 15•11 years ago
|
||
Updated•11 years ago
|
Whiteboard: [adv-main23-]
Updated•11 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•