Closed
Bug 1156861
Opened 9 years ago
Closed 9 years ago
crash in mozilla::plugins::PluginProcessParent::RunLaunchCompleteTask()
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(firefox38 wontfix, firefox38.0.5 wontfix, firefox39+ fixed, firefox40+ fixed, firefox41+ fixed, firefox-esr31 unaffected, firefox-esr3839+ fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master unaffected)
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox38 | --- | wontfix |
firefox38.0.5 | --- | wontfix |
firefox39 | + | fixed |
firefox40 | + | fixed |
firefox41 | + | fixed |
firefox-esr31 | --- | unaffected |
firefox-esr38 | 39+ | fixed |
b2g-v2.0 | --- | unaffected |
b2g-v2.0M | --- | unaffected |
b2g-v2.1 | --- | unaffected |
b2g-v2.1S | --- | unaffected |
b2g-v2.2 | --- | unaffected |
b2g-master | --- | unaffected |
People
(Reporter: MatsPalmgren_bugz, Assigned: bugzilla)
References
Details
(4 keywords, Whiteboard: [adv-main39+][adv-esr38.1+])
Crash Data
Attachments
(1 file)
3.04 KB,
patch
|
jimm
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr38+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
[Tracking Requested - why for this release]: topcrash This bug was filed from the Socorro interface and is report bp-3c210213-ad9d-4412-b227-c98062150412. ============================================================= Looks like a recent regression - first reported crash is in build 2015041100. Currently #40 topcrash in 39.0a2. Most crashes have: EXCEPTION_ACCESS_VIOLATION_READ 0x5a5a5a5a so filing it security-sensitive just in case.
Comment 1•9 years ago
|
||
Looks like the signature is 39 specific (although 40 may just be too low volume). Tracking 39 in case this turns into a sec issue.
Comment 2•9 years ago
|
||
Aaron - Flagging you because this is plug-in related. Ideas?
Flags: needinfo?(aklotz)
Assignee | ||
Comment 3•9 years ago
|
||
I think what we're seeing here is a runnable that is executing after its class has been destroyed.
Blocks: asyncplugininit
Flags: needinfo?(aklotz)
Comment 4•9 years ago
|
||
This is a regression but not a high volume crash (40 crashes reported total for the last 7 days). The signature isn't showing up at all on Firefox 40.
Comment 5•9 years ago
|
||
Untracking for 39 since it's not a topcrash (roughly defined as being in the top 10 for a channel at some point) and this doesn't have a sec rating. Please do renominate this if it becomes a bigger issue or is sec-high or sec-critical.
Reporter | ||
Comment 6•9 years ago
|
||
[Tracking Requested - why for this release]: 0x5a5a5a5a is a clear indication of use-after-free, so I think sec-high is a reasonable rating until we know more. Also, I disagree with "top 10 for a channel" as the definition of topcrash. I would say all 50 of the signatures on the "Top Crashers" list for each channel at crash-stats.com should be considered. There are always a handful of signatures that we never track, like OOM|small, EnterBaseline and such, and there are usually also plenty of dupes that are the same underlying bug like igd10umd32.dll@ etc. Usually there are also a few signatures in binary extensions / plugins that can be ignored. That leaves about ~30 that I would say should count as topcrash. (a few are usually on multiple channels, so it's less than 3*30 total). We don't need to track all of those of course. But signatures that are relatively new, like this bug, should *definitely* be tracked. Engineers that introduced this signature and/or changed related code around that date should be asked to investigate. We really need to address crash regressions like this promptly, and try hard to fix them before they reach the release channel.
tracking-firefox39:
--- → ?
Keywords: sec-high
Comment 7•9 years ago
|
||
Mats, I would love to be able to spend more time helping out with crash bugs. Here I was trying to put a priority on higher volume crashes. I agree with you that we would benefit from more resources to help fix recent regressions no matter what the crash volume is. I'll track this again for now and see if I can help out. Thanks for re-nomming and explaining your take on it. Since this signature first appeared on build 2015041100. April 11th was a Saturday, maybe looking at the changeset from the day before would help. http://hg.mozilla.org/releases/mozilla-aurora/pushloghtml?startdate=34+days+ago&enddate=31days+ago So from that, bug 1152395 looks like the most likely culprit. Aaron, maybe you can take this bug when you get back next week, or maybe someone else will work on it in the meantime.
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(aklotz)
Assignee | ||
Comment 8•9 years ago
|
||
This patch makes it possible to revoke the task in question so that it can't run after the PluginProcessParent object is deleted. [Security approval request comment] How easily could an exploit be constructed based on the patch? Not easily. It is heavily sensitive to timing where a plugin instantiation and teardown happen in quick succession. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No, it's very vague. It's not even obvious by looking at the code. Which older supported branches are affected by this flaw? 39+ If not all supported branches, which bug introduced the flaw? bug 998863 Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Should be a trivial merge. How likely is this patch to cause regressions; how much testing does it need? No regressions possible.
Assignee: nobody → aklotz
Status: NEW → ASSIGNED
Attachment #8610188 -
Flags: sec-approval?
Attachment #8610188 -
Flags: review?(jmathies)
Comment 9•9 years ago
|
||
Based on the regressing bug then Fx37 and 38 should be affected, too; I also don't see what would have fixed this in 40. Maybe the crashes are more or less likely based on timing affected by other code changes.
Blocks: 998863
status-firefox41:
--- → affected
status-firefox-esr31:
--- → unaffected
status-firefox-esr38:
--- → affected
tracking-firefox40:
--- → +
tracking-firefox41:
--- → +
tracking-firefox-esr38:
--- → 39+
Comment 10•9 years ago
|
||
Comment on attachment 8610188 [details] [diff] [review] Add TaskFactory to PluginProcessParent Normally please wait for a reviewed patch before requesting sec-approval, but in this case it looks straightforward enough. sec-approval=dveditz
Attachment #8610188 -
Flags: sec-approval? → sec-approval+
Updated•9 years ago
|
Attachment #8610188 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 11•9 years ago
|
||
(In reply to Daniel Veditz [:dveditz] from comment #9) > Based on the regressing bug then Fx37 and 38 should be affected, too; I also > don't see what would have fixed this in 40. Maybe the crashes are more or > less likely based on timing affected by other code changes. Good point. Usually for async plugin init the affected code is disabled in 38 and above, but not in this case.
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8610188 [details] [diff] [review] Add TaskFactory to PluginProcessParent [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is sec-high User impact if declined: Possible crashes and UAF under specific timing conditions. Fix Landed on Version: 41, uplift pending for 39 and 40 Risk to taking this patch (and alternatives if risky): None. Simple patch adding revocable tasks which is well tested code. String or UUID changes made by this patch: None See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info. Approval Request Comment [Feature/regressing bug #]: bug 998863 [User impact if declined]: Possible crashes and UAF under specific timing conditions. [Describe test coverage new/current, TreeHerder]: Plugin mochitests [Risks and why]: None. Simple patch adding revocable tasks which is well tested code. [String/UUID change made/needed]: None
Attachment #8610188 -
Flags: approval-mozilla-esr38?
Attachment #8610188 -
Flags: approval-mozilla-beta?
Attachment #8610188 -
Flags: approval-mozilla-aurora?
Comment 13•9 years ago
|
||
aaron, you mention this landed in 41. Was that in a different bug? I don't see it landing here. Happy to uplift the fix though.
Flags: needinfo?(aklotz)
Attachment #8610188 -
Flags: approval-mozilla-esr38?
Attachment #8610188 -
Flags: approval-mozilla-esr38+
Attachment #8610188 -
Flags: approval-mozilla-beta?
Attachment #8610188 -
Flags: approval-mozilla-beta+
Attachment #8610188 -
Flags: approval-mozilla-aurora?
Attachment #8610188 -
Flags: approval-mozilla-aurora+
Approving for uplift to esr38, Beta and Aurora.
Assignee | ||
Comment 15•9 years ago
|
||
Just wanted to land them all at once, is all.
Flags: needinfo?(aklotz)
Assignee | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/29a9acde728e https://hg.mozilla.org/releases/mozilla-aurora/rev/5faf6e698493 https://hg.mozilla.org/releases/mozilla-beta/rev/f3259a2d48bc https://hg.mozilla.org/releases/mozilla-esr38/rev/29cec4e0a255
Updated•9 years ago
|
status-b2g-v2.0:
--- → unaffected
status-b2g-v2.0M:
--- → unaffected
status-b2g-v2.1:
--- → unaffected
status-b2g-v2.1S:
--- → unaffected
status-b2g-v2.2:
--- → unaffected
status-b2g-master:
--- → unaffected
status-firefox38.0.5:
--- → wontfix
Assignee | ||
Comment 17•9 years ago
|
||
Fix for esr38 bustage https://hg.mozilla.org/releases/mozilla-esr38/rev/655459a91a88
Assignee | ||
Comment 18•9 years ago
|
||
and another https://hg.mozilla.org/releases/mozilla-esr38/rev/c9b1eb9d6235
Comment 19•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/29a9acde728e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•9 years ago
|
Whiteboard: [adv-main39+][adv-esr38.1+]
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•8 years ago
|
Group: core-security-release
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•