852315 - plugin crash running script during plugin destruction

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Timothy Nikkel (:tnikkel)]

I made a clean debug mac build of rev e23e43a2c14e

If I load
http://congressoamericano.blogspot.ca/
wait for the music to start, then click on the stop button I get a crash. The stack goes from plugin destruction into running a script which plays with things and that causes the crash.

This page hasn't had a problem for me before, so I suspect something changed, either the website or something on mozilla-central.

This doesn't seem to crash the latest nightly for whatever reason.

Comment 1

[Steven Michaud [:smichaud] (Retired)]

> https://hg.mozilla.org/mozilla-central/rev/e23e43a2c14e

This is still the tip.  So whatever change triggered these crashes will presumably appear in tomorrow's mozilla-central nightly.

Please wait til it appears and test with it.

Comment 2

[Steven Michaud [:smichaud] (Retired)]

Tinderbox opt build corresponding to rev e23e43a2c14e:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64/1363637255/firefox-22.0a1.en-US.mac.dmg

Also try testing with this.

So will I.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: stack

Comment 4

[Steven Michaud [:smichaud] (Retired)]

I can't repro with the build I posted in comment #2.

I tested on OS X 10.8.2.

Comment 5

[Steven Michaud [:smichaud] (Retired)]

But I do see crashes testing with the tinderbox debug build corresponding with rev e23e43a2c14e:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-macosx64-debug/1363637255/firefox-22.0a1.en-US.mac64.dmg

bp-4c5b9c1d-53ba-4271-ba1f-afa362130318
bp-23d13f61-fd74-415c-b2d6-be0be2130318

They usually happen when I click on the Play button.

Comment 6

[Steven Michaud [:smichaud] (Retired)]

But the crashes also happen with today's mozilla-central-debug nightly:

bp-63ae03b0-a2fc-40cc-b273-1ac392130318

So someone will need to establish the regression range using mozilla-central-debug nightlies.

Comment 7

[John Schoenick [:johns]]

nsDocument calls EnumerateFreezables in order to send notifications to all relevant elements that the document has gone inactive, but nsObjectLoadingContent spins the plugin code in that notification, which re-enters, which runs arbitrary script, which appends a freezable element to the document, which modifies the hash table we're enumerating.

It might be possible to achieve arbitrary memory corruption with this :-/

Comment 8

[Andrew McCreight [:mccr8]]

Sounds like possible sec-critical.

Comment 9

[John Schoenick [:johns]]

Attachment: Use CheckPluginStopEvent for plugins when documents go inactive

Comment 10

[John Schoenick [:johns]]

Attachment: Add scary warning comment

Comment 11

[John Schoenick [:johns]]

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

Just use CheckPluginStopEvent when documents go inactive as well. We already use this for UnbindFromTree, and since nsDocument is non-reentrant at this point this seems like the sane solution if no tests explode.

Comment 12

[John Schoenick [:johns]]

Comment on attachment 726434 [details] [diff] [review]
Add scary warning comment

Add a warning comment, but split out since we wont want to land it until the fix is shipped

Comment 13

[John Schoenick [:johns]]

Pushed to try folded with other misc plugin patches (it's a pretty non-obvious issue):

https://tbpl.mozilla.org/?tree=Try&rev=b8b8c22ad7ae

Comment 14

[John Schoenick [:johns]]

Marking b2g unaffected as this requires a spawned plugin

Comment 15

[John Schoenick [:johns]]

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

This looks good on try -- the bc failure was from one of the other patches in that run.

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Unknown, has existed for a long while

User impact if declined:
sec bug - fairly easy to trigger memory corruption

Testing completed (on m-c, etc.):
None, will land on m-c prior to branches

Risk to taking this patch (and alternatives if risky):
Low risk

String or UUID changes made by this patch:
None

Comment 16

[John Schoenick [:johns]]

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
If the issue is noticed, the exploit is fairly trivial, but it is a somewhat obscure re-entrance issue.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The warning comment about re-entry was split into another patch that can land later.

Which older supported branches are affected by this flaw?
All current branches -- but the issue requires a running plugin, so not relevant to b2g.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Rebasing on branches is trivial

How likely is this patch to cause regressions; how much testing does it need?
Ordering of plugin events and teardown has always been finicky, but this changes "document has gone inactive" behavior to be identical to "removed from document" behavior for plugins, and should be fairly low risk.

Comment 17

[John Schoenick [:johns]]

Comment on attachment 726434 [details] [diff] [review]
Add scary warning comment

[Security approval request comment]
This just adds a comment to warn about the non-obvious re-entrance risk, and should land once the fix is shipped.

Comment 18

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

We're too close to release to take this now, AFAIK. I want to get release management commentary but I expect that we won't approve this for a couple of weeks until after Firefox 20 ships.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

It's definitely too late for FF20, marking tracking accordingly.  This is internally reported and if Al isn't concerned about it sitting until a couple weeks into FF21 let's do that and buy some proper bake time.

Comment 20

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

For these same reasons, I'd prefer to wait until after we ship 20. I'd say that any time from 4/16 (end of second week of the cycle) on would be good since, if noticed, an exploit wouldn't be hard per comment 16.

Comment 21

[John Schoenick [:johns]]

This is flagged to be checked in this week but still lacks branch approvals -- are we waiting on an m-c landing first, or do we want this to land everywhere?

Comment 22

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

Please land it on m-c first and mark the patch (or patches) for branch as necessary after. It doesn't need to land everywhere at once.

Comment 23

[bhavana bajaj [:bajaj]]

All the branch patches will be approved, once this has landed on m-c .

Comment 24

[John Schoenick [:johns]]

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

https://hg.mozilla.org/integration/mozilla-inbound/rev/021bca10985b

Comment 25

[John Schoenick [:johns]]

argh, bugzilla

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/021bca10985b

Comment 27

[bhavana bajaj [:bajaj]]

Comment on attachment 726433 [details] [diff] [review]
Use CheckPluginStopEvent for plugins when documents go inactive

Adding Matt as QA contact to help verify this on trunk & branches once this lands.

Comment 28

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b8aeaed19ddd

Required some tweaks to rebase on beta/esr17, so waiting on this build to finish before pushing there...

Comment 29

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5ec90ea60b60

Comment 30

[John Schoenick [:johns]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/214646821a0d

Comment 31

[Matt Wobensmith [:mwobensmith][:matt:]]

I'm not able to repro on any debug builds on or around reported date. Also, try builds don't appear to be available that far back either.

So I guess I'd like to ask Timothy/Steven to verify that an affected config no longer crashes, if that's possible.

Comment 32

[Steven Michaud [:smichaud] (Retired)]

I can no longer reproduce this bug's crashes using the build I tested with in comment #6.  I assume the website's changed.

Comment 33

[Timothy Nikkel (:tnikkel)]

Yeah, I couldn't reproduce a crash either. I think the site must have changed as well, because I had been testing with this site for a while before the first crash happen, so it seems a site changed is what made this show up in the first place, it makes sense that a site change with make it go away too.

Comment 34

[Timothy Nikkel (:tnikkel)]

Or a similar comment saying the same but with correct grammar. :)

Comment 35

[u279076]

Given we've lost our testcase (due to a website change) I don't think we'll be able to truly verify this is fixed. For this reason I am tagging this bug qa-. Please re-add the verifyme keyword if circumstances change which allow us to test the fix.

Comment 36

[John Schoenick [:johns]]

Attachment: Testcase

Not as useful as a live site using flash, but this test case reliably causes the crash using the test plugin, which should at least be helpful in ensuring its properly fixed on branches.

Comment 37

[u279076]

Thanks John.

Matt can you please verify this is fixed using John's testcase?

Comment 38

[John Schoenick [:johns]]

Attachment: Mochitest

For landing along with scary warning comment once this is fixed everywhere

Comment 39

[Matt Wobensmith [:mwobensmith][:matt:]]

John, thanks for going the extra mile and making the test case!

Only issue is that I can't get it to crash for me. I'm using debug builds, from 3/18-4/18, all affected branches - I just get an iframe and the missing plugin UI. Nothing crashes.

Any ideas? Thanks.

Comment 40

[John Schoenick [:johns]]

(In reply to Matt Wobensmith from comment #39)
> John, thanks for going the extra mile and making the test case!
> 
> Only issue is that I can't get it to crash for me. I'm using debug builds,
> from 3/18-4/18, all affected branches - I just get an iframe and the missing
> plugin UI. Nothing crashes.
> 
> Any ideas? Thanks.

The test needs the nptest plugin to be a co-conspirator in behaving badly, which I think was changed to not be loaded by default in debug builds. You can fix this by copying/symlinking dist/plugins to dist/bin/browser/plugins. Make sure about:plugins shows a plugin claiming "application/x-test"

You can also load nptest into release builds by grabbing a version of it from a similarly-versioned debug build (it never changes much) and putting it into some_test_profile/plugins/ or (I think?) install_dir/plugins/, though the latter may have been disabled.

Comment 41

[Matt Wobensmith [:mwobensmith][:matt:]]

Got it. I see a crash now, but it affects the fixed builds too. This is Mac.

Needs more investigation. John mentioned to me that he will take another look at it shortly.

Comment 42

[John Schoenick [:johns]]

(In reply to Matt Wobensmith from comment #41)
> Needs more investigation. John mentioned to me that he will take another
> look at it shortly.

I borrowed someone's OS X build and gave this a shot, but wasn't able to reproduce the crash. I don't have a proper OS X environment setup at the moment -- before I try to dig into that, do you think you can get a backtrace w/symbols of the crash you're seeing?

Comment 43

[Matt Wobensmith [:mwobensmith][:matt:]]

OK, working with John, I see that I have an unrelated crash due to a faulty plugin or issue therein. However, this bug is fixed.

Verified crash in m-c, 2013-03-19
Verified fix in m-c 23.0a1, 2013-04-25
Verified fix in Aurora, 22.0a2 2013-04-25
Verified fix in 21.0, 2013-04-24
Verified fix in 17.0.5esrpre, 2013-04-22

Comment 44

[u279076]

Thanks Matt!

Comment 45

[John Schoenick [:johns]]

Attachment: Test. r=bsmedberg

Un-bitrotted

Comment 46

[John Schoenick [:johns]]

Comment on attachment 726434 [details] [diff] [review]
Add scary warning comment

https://hg.mozilla.org/integration/mozilla-inbound/rev/23dfc36f99d4

Comment 47

[John Schoenick [:johns]]

Comment on attachment 8379260 [details] [diff] [review]
Test. r=bsmedberg

Pushed test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13cc8a7ee925

Try:
https://tbpl.mozilla.org/?tree=Try&rev=fd55a944e195

Comment 48

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I backed this (and everything else from that push to inbound) out in http://hg.mozilla.org/integration/mozilla-inbound/rev/128cbf1edc40 due to various Java/plugin related failures they caused:
Crashtest failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35003251&tree=Mozilla-Inbound
XPCShell failure: https://tbpl.mozilla.org/php/getParsedLog.php?id=35003355&tree=Mozilla-Inbound

Comment 49

[John Schoenick [:johns]]

Attempt 2:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9e44139023
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d186c979373

Extensive try run since this push burned the tree before:
https://tbpl.mozilla.org/?tree=Try&rev=b656bdd77ce1

Comment 50

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5c9e44139023
https://hg.mozilla.org/mozilla-central/rev/0d186c979373