Closed
Bug 895222
Opened 11 years ago
Closed 11 years ago
Crash with expandos, HTMLPropertiesCollection, CC
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla25
Tracking | Status | |
---|---|---|
firefox22 | --- | unaffected |
firefox23 | + | fixed |
firefox24 | + | fixed |
firefox25 | + | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: jruderman, Assigned: smaug)
References
Details
(Keywords: crash, sec-high, testcase, Whiteboard: [adv-main23-])
Crash Data
Attachments
(3 files)
333 bytes,
text/html
|
Details | |
12.29 KB,
text/plain
|
Details | |
1.88 KB,
patch
|
mccr8
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
1. Install https://www.squarefree.com/extensions/domFuzzLite3.xpi
2. Load the testcase
Usually crashes [@ mozilla::dom::HTMLPropertiesCollection::EnsureFresh]. The crash is more reliable in debug builds.
bp-4962a59a-6f1e-4700-9fd7-41e382130718
Reporter | ||
Comment 1•11 years ago
|
||
Comment 2•11 years ago
|
||
Um...
So the crash location is dereferencing mNames in EnsureFresh.
mNames is set in the HTMLPropertiesCollection constructor like so:
mNames = new PropertyStringList(this);
The only place it's touched after this is in HTMLPropertiesCollection unlink. And indeed, in the debugger I see us unlinking this particular HTMLPropertiesCollection object... and then doing a call into it from JS!
I added breakpoints to AddRef/Release on HTMLPropertiesCollection, and I see several calls to AddRef: one from the PropertyStringList ctor, one from nsGenericHTMLElement::Properties, and one from wrapping into the DOM binding. Then I'm seeing the object get CCed. Something is seriously broken here. I wonder whether this is a snowhite regression...
status-firefox25:
--- → ?
Assignee | ||
Comment 3•11 years ago
|
||
(I'd guess it is the proxy handling which causes problems.)
Assignee | ||
Comment 4•11 years ago
|
||
Oh, PropertyStringList has two refcnts. That is bad. Super error prone.
Updated•11 years ago
|
Assignee: nobody → continuation
Updated•11 years ago
|
Flags: needinfo?(peterv)
Comment 6•11 years ago
|
||
As bz expected, the CC is identifying this object as garbage.
Here's the object, with its refcount of 3:
0x129362e00 [rc=3] HTMLPropertiesCollection
> 0x129362ce0 mRoot
> 0x1293425c0 mNames
> 0x1184cb300 Preserved wrapper
Here are the three references:
0x129362ce0 [rc=1] FragmentOrElement (xhtml) div file:///Users/amccreight/mz/tests/895222.html
> 0x129362e00
0x1293425c0 [rc=1] PropertyStringList
> 0x129362e00 mCollection
0x1184cb300 [gc] JS Object (Proxy)
> 0x1184353e0 extra0
> 0x129362e00 UnwrapDOMObject(obj)
I'm not sure which of these is unexpected. I'm guessing the Proxy?
It is rather peculiar that the div element's reference doesn't have a name associated with it, but that could just be a superficial deficiency.
Comment 7•11 years ago
|
||
I'm marking this sec-high because when combined with another bug that leaves an unlinked object with a dangling pointer, this could be turned into a UAF. sec-moderate might also be okay, depending on how confident we are that doesn't happen. ;)
Keywords: sec-high
Comment 8•11 years ago
|
||
I don't have much time for this for the next day or two, it sounds like, and Olli said he'd take a look. Thanks!
Assignee: continuation → bugs
Comment 9•11 years ago
|
||
All three references from comment 6 are expected to be references to the object... The proxy's reference, however, should keep it alive, since the proxy is alive.
Comment 10•11 years ago
|
||
The only reference to it in the CC graph is the HTMLPropertiesCollection.
In the GC graph, its references are thus:
via DOM expando object :
0x1184cb300 [Proxy <no private>]
via mExpandoAndGeneration.expando :
0x118435400 [Object <no private>]
--[y]-> 0x1184cb300 [Proxy <no private>]
So I guess we're not tracing through the mExpandoAndGeneration.expando in the CC graph for some reason? 0x118435400 doesn't show up in the CC graph at all.
Assignee | ||
Comment 11•11 years ago
|
||
This should be pretty clear.
We added something new to trace, but didn't change NeedsScriptTraverse which is used in CanSkipInCC (I know, NeedsScriptTraverse was error prone).
This should work for now. If we start to add more complex tracing to various nodes, we
may need to re-think this some more.
Change to nsChildContentList is needed because NeedsScriptTraverse starts to take nsINode as param.
Attachment #778750 -
Flags: review?(continuation)
Assignee | ||
Updated•11 years ago
|
status-firefox23:
--- → affected
status-firefox24:
--- → affected
tracking-firefox23:
--- → ?
tracking-firefox24:
--- → ?
tracking-firefox25:
--- → ?
Comment 12•11 years ago
|
||
Comment on attachment 778750 [details] [diff] [review]
patch
Review of attachment 778750 [details] [diff] [review]:
-----------------------------------------------------------------
Yikes!
Attachment #778750 -
Flags: review?(continuation) → review+
Assignee | ||
Comment 13•11 years ago
|
||
Comment on attachment 778750 [details] [diff] [review]
patch
[Security approval request comment]
How easily could an exploit be constructed based on the patch? The crash in this bug is a null-pointer crash.
It is possible, although I think unlikely, that some other type of crash can be found.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment will be about improving skippability optimization
Which older supported branches are affected by this flaw?
beta (ff23), aurora (ff24), nightly (ff25)
If not all supported branches, which bug introduced the flaw?
bug 820846, kind of.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch applies cleanly to beta/aurora/nightly
How likely is this patch to cause regressions; how much testing does it need?
The patch may cause tiny bit higher cycle collection times. Telemetry would tell the regression quite fast, but
unfortunately telemetry is all broken atm. Hopefully it will be fixed in the next few days.
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 820846
User impact if declined: Random crashes because expando properties on document and form element aren't effectively
strong pointers
Testing completed (on m-c, etc.): tryserver looks ok.
Risk to taking this patch (and alternatives if risky): Low-risky. We're just more careful when to _not_ optimize something
out from cycle collection graph
String or IDL/UUID changes made by this patch: NA
Attachment #778750 -
Flags: sec-approval?
Attachment #778750 -
Flags: approval-mozilla-beta?
Attachment #778750 -
Flags: approval-mozilla-aurora?
Comment 14•11 years ago
|
||
I need release management comments about whether we can take this on beta at this point before sec-approval. It is only a four line patch though!
Updated•11 years ago
|
Flags: needinfo?(release-mgmt)
Comment 15•11 years ago
|
||
(In reply to Al Billings [:abillings] from comment #14)
> I need release management comments about whether we can take this on beta at
> this point before sec-approval. It is only a four line patch though!
Since telemetry would be our indicator of regression but telemetry is broken atm, I'm not comfortable taking this to Beta. Given it's a higher likelihood of crashing in debug builds as well as comment 7 suggesting this is closer to moderate in risk (though labeled 'high') I think we should hold off for now. If telemetry is repaired before this Thursday's beta I would reconsider but that's pushing it. Might be better just to wait for FF24.
Flags: needinfo?(release-mgmt)
Updated•11 years ago
|
Assignee | ||
Comment 16•11 years ago
|
||
telemetry is not broken anymore.
Assignee | ||
Comment 17•11 years ago
|
||
And note, I don't expect there to be any significant regression.
(effectively one virtual call in case there is expando properties on a DOM Node)
Updated•11 years ago
|
Comment 18•11 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #16)
> telemetry is not broken anymore.
OK then :)
Abillings - if you're comfortable with sec-approval+ here, we can take this to branches then.
Flags: needinfo?(abillings)
Comment 19•11 years ago
|
||
Comment on attachment 778750 [details] [diff] [review]
patch
Sounds good. Let's do it.
Attachment #778750 -
Flags: sec-approval?
Attachment #778750 -
Flags: sec-approval+
Attachment #778750 -
Flags: approval-mozilla-beta?
Attachment #778750 -
Flags: approval-mozilla-beta+
Attachment #778750 -
Flags: approval-mozilla-aurora?
Attachment #778750 -
Flags: approval-mozilla-aurora+
Updated•11 years ago
|
Flags: needinfo?(abillings)
Assignee | ||
Comment 20•11 years ago
|
||
Comment 21•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Comment 22•11 years ago
|
||
Firefox 22 is unaffected by this issue?
Assignee | ||
Comment 23•11 years ago
|
||
yes. bug 820846 landed to FF23
Updated•11 years ago
|
status-b2g18:
--- → unaffected
status-firefox22:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Updated•11 years ago
|
Updated•11 years ago
|
status-b2g18:
--- → unaffected
status-firefox-esr17:
--- → unaffected
Updated•11 years ago
|
Group: core-security
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•