Closed Bug 1464061 Opened 2 years ago Closed 2 years ago

Crash in static struct already_AddRefed<T> GetRootWidgetForPluginFrame

Categories

(Core :: Plug-ins, defect, critical)

Unspecified
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- fixed
firefox60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed

People

(Reporter: marcia, Assigned: m_kato)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is
report bp-a7cdd01e-a618-44b1-93d1-be9e30180520.
=============================================================

Seen while looking at nightly crash stats: https://bit.ly/2kni1c0. Crashes appear to have started using Build ID 20180518222751. 18 crashes/12 installs, although it appears that some of the crashes are from the same user.

Bug 1456294 landed in that timeframe. ni on :m_kato

Top 10 frames of crashing thread:

0 xul.dll static struct already_AddRefed<nsIWidget> GetRootWidgetForPluginFrame dom/plugins/base/nsPluginInstanceOwner.cpp:632
1 xul.dll nsPluginInstanceOwner::EnableIME dom/plugins/base/nsPluginInstanceOwner.cpp:907
2 xul.dll mozilla::plugins::PluginInstanceParent::RecvEnableIME dom/plugins/ipc/PluginInstanceParent.cpp:2306
3 xul.dll mozilla::plugins::PPluginInstanceParent::OnMessageReceived ipc/ipdl/PPluginInstanceParent.cpp:1624
4 xul.dll mozilla::plugins::PPluginModuleParent::OnMessageReceived ipc/ipdl/PPluginModuleParent.cpp:795
5 xul.dll mozilla::ipc::MessageChannel::DispatchAsyncMessage ipc/glue/MessageChannel.cpp:2136
6 xul.dll mozilla::ipc::MessageChannel::DispatchMessageW ipc/glue/MessageChannel.cpp:2066
7 xul.dll mozilla::ipc::MessageChannel::Call ipc/glue/MessageChannel.cpp:1688
8 xul.dll mozilla::plugins::PPluginInstanceParent::CallNPP_Destroy ipc/ipdl/PPluginInstanceParent.cpp:966
9 xul.dll mozilla::plugins::PluginInstanceParent::Destroy dom/plugins/ipc/PluginInstanceParent.cpp:204

=============================================================
Flags: needinfo?(m_kato)
Group: core-security → dom-core-security
Assignee: nobody → m_kato
Flags: needinfo?(m_kato)
Attachment #8980526 - Attachment description: Don → Return error immedently when plugin frame is destroyed
Attachment #8980526 - Attachment is patch: true
Comment on attachment 8980526 [details] [diff] [review]
Return error immedently when plugin frame is destroyed

EnableIME needs whether plugin frame is already destroyed since this is async IPC.
Attachment #8980526 - Flags: review?(masayuki)
Comment on attachment 8980526 [details] [diff] [review]
Return error immedently when plugin frame is destroyed

Assuming that nsPluginInstanceOwner::Destroy() is called before this.
Attachment #8980526 - Flags: review?(masayuki) → review+
Blocks: 1456294
Keywords: regression
Comment on attachment 8980526 [details] [diff] [review]
Return error immedently when plugin frame is destroyed

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

No, this depedns on Flash's behavior.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes, I don't add any comments.

Which older supported branches are affected by this flaw?

62 only

If not all supported branches, which bug introduced the flaw?

62 only, so it is unnecessary for old branch

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

N/A

How likely is this patch to cause regressions; how much testing does it need?

No, this fix adds nullptr check that is crash condition.
Attachment #8980526 - Flags: sec-approval?
If this is nightly only you don't need sec-approval.
https://hg.mozilla.org/mozilla-central/rev/5be1985874a0
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Group: core-security-release
See Also: → 1502347
Crash Signature: [@ static struct already_AddRefed<T> GetRootWidgetForPluginFrame] → [@ static struct already_AddRefed<T> GetRootWidgetForPluginFrame] [@ GetRootWidgetForPluginFrame]
Comment on attachment 8980526 [details] [diff] [review]
Return error immedently when plugin frame is destroyed

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: I don't know how to reproduce it.  When using Flash plugin, plugin process may crash to use IME.

User impact if declined: When using Flash plugin, plugin process may crash to use IME.

Fix Landed on Version: 62

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Low.  When plugin frame is nothing, we don't handle IME message from OS/chrome process.  Plugin already disappears on this situation.

String or UUID changes made by this patch: No
Attachment #8980526 - Flags: approval-mozilla-esr60?
Duplicate of this bug: 1502347
Comment on attachment 8980526 [details] [diff] [review]
Return error immedently when plugin frame is destroyed

Simple fix for an ESR60 topcrash. Approved for 60.3.1esr (or 60.4.0esr, whatever release we end up shipping next).
Attachment #8980526 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
You need to log in before you can comment on or make changes to this bug.