895222 - Crash with expandos, HTMLPropertiesCollection, CC

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jesse Ruderman]

Attachment: testcase (requires extension for CC)

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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

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...

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(I'd guess it is the proxy handling which causes problems.)

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Oh, PropertyStringList has two refcnts. That is bad. Super error prone.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Peter, do you have time to look into this?

Comment 6

[Andrew McCreight [:mccr8]]

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

[Andrew McCreight [:mccr8]]

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. ;)

Comment 8

[Andrew McCreight [:mccr8]]

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!

Comment 9

[Boris Zbarsky [:bzbarsky]]

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

[Andrew McCreight [:mccr8]]

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.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

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.

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 778750 [details] [diff] [review]
patch

Review of attachment 778750 [details] [diff] [review]:
-----------------------------------------------------------------

Yikes!

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 14

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

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!

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

telemetry is not broken anymore.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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)

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

(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.

Comment 19

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

Comment on attachment 778750 [details] [diff] [review]
patch

Sounds good. Let's do it.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ef1ee8a5061
https://hg.mozilla.org/releases/mozilla-aurora/rev/a0784e2d511d
https://hg.mozilla.org/releases/mozilla-beta/rev/476e5f4f5603

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7ef1ee8a5061

Comment 22

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

Firefox 22 is unaffected by this issue?

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

yes. bug 820846 landed to FF23