Closed Bug 1495245 Opened 6 years ago Closed 6 years ago

Crash in PLDHashTable::Iterator::Iterator | mozilla::plugins::PluginUtilsWin::AudioDeviceMessageRunnable::Run

Categories

(Core :: Security: Process Sandboxing, defect, P1)

62 Branch
Unspecified
Windows
defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ fixed
firefox62 --- wontfix
firefox63 + verified
firefox64 + verified

People

(Reporter: philipp, Assigned: handyman)

References

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main63+][adv-esr60.3+])

Crash Data

Attachments

(2 files)

This bug was filed from the Socorro interface and is
report bp-0418108d-c89d-4720-9a35-6d4cb0180925.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll PLDHashTable::Iterator::Iterator xpcom/ds/PLDHashTable.cpp:733
1 xul.dll mozilla::plugins::PluginUtilsWin::AudioDeviceMessageRunnable::Run dom/plugins/ipc/PluginUtilsWin.cpp:45
2 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1051
3 xul.dll nsThread::Shutdown xpcom/threads/nsThread.cpp:799
4 xul.dll mozilla::detail::RunnableMethodImpl<RefPtr<nsUrlClassifierDBServiceWorker>, void  xpcom/threads/nsThreadUtils.h:1216
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1051
6 xul.dll nsThread::Shutdown xpcom/threads/nsThread.cpp:799
7 xul.dll mozilla::detail::RunnableMethodImpl<RefPtr<nsUrlClassifierDBServiceWorker>, void  xpcom/threads/nsThreadUtils.h:1216
8 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1051
9 xul.dll XPTC__InvokebyIndex xpcom/reflect/xptcall/md/win32/xptcinvoke_asm_x86_64.asm:97

=============================================================

this crash signature is newly showing up in firefox 62 - perhaps related to the changes in bug 1449388. in the absolute majority of cases it's a win64 build that's crashing with accessibility enabled.
David, can you please take a look?
Group: core-security → dom-core-security
Flags: needinfo?(davidp99)
It looks like I got sloppy around monitoring the IMMNotificationClients lifetime wrt the asynchronous commands and the other bug was hiding the failure.  This shouldn't be a hard one to fix.
Assignee: nobody → davidp99
Flags: needinfo?(davidp99)
Priority: -- → P1
Changes AudioDeviceMessageRunnable to hold the AudioNotification instead of its PluginModuleSet to simplify object lifetime issues.  AudioNotification is ref-counted so we can increment its ref-count during the asynchronous Runnable.
FYI, there is really no way to reproduce the crash, which is timing related.  The bug very rarely happens at shutdown.
AFAICT, ESR60 should also be affected by this issue. Can you suggest a severity rating, David?
https://wiki.mozilla.org/Security_Severity_Ratings
Sure -- timing and plugin factors suggest sec-moderate
Keywords: sec-moderate
Keywords: checkin-needed
Comment on attachment 9013863 [details]
Bug 1495245: Refactor AudioDeviceMessageRunnable's use of the PluginModuleSet (r?jmathies)

I just realized I was too caught up in fighting Lando to remember to get sec-approval.

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: Not very.  The crash issue is thread-timing related and happens at shutdown.  And in the parent process.  The patch does not mention there is currently a problem.

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

Which older supported branches are affected by this flaw?: likely all of them

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

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: Not at all hard but it is possible that changes since esr60 might require manual effort.

How likely is this patch to cause regressions; how much testing does it need?: There is asynchronous code so it isn't completely straightforward but it's still unlikely to cause new problems.  The only substantive change is to properly maintain a refcount.
Attachment #9013863 - Flags: sec-approval?
Comment on attachment 9013863 [details]
Bug 1495245: Refactor AudioDeviceMessageRunnable's use of the PluginModuleSet (r?jmathies)

It's rated sec-moderate, so there's no need for sec-approval. Feel free to do the Beta & ESR60 approval requests, though :)
Attachment #9013863 - Flags: sec-approval?
Thanks.  Somehow, I managed to completely avoid learning that.
https://hg.mozilla.org/integration/autoland/rev/87fec627956b5fa5a56138ae8f4bb9e7c0e9d943
https://hg.mozilla.org/mozilla-central/rev/87fec627956b
Group: dom-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Comment on attachment 9013863 [details]
Bug 1495245: Refactor AudioDeviceMessageRunnable's use of the PluginModuleSet (r?jmathies)

Just checked -- this patch should apply cleanly to beta.  ESR60 will be another story.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1251202

User impact if declined: Crashes, apparently on main process shutdown

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Medium

Why is the change risky/not risky? (and alternatives if risky): There is asynchronous code so it isn't completely straightforward but it's still unlikely to cause new problems.  The only substantive change is to properly maintain a refcount that should only matter in rare occasions on shutdown.

String changes made/needed: None
Attachment #9013863 - Flags: approval-mozilla-beta?
Comment on attachment 9013863 [details]
Bug 1495245: Refactor AudioDeviceMessageRunnable's use of the PluginModuleSet (r?jmathies)

Approved for 63.0b14.
Attachment #9013863 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
No crashes on Nightly or Beta since this landed :D. David, given that bug 1449388 was marked as disabled for ESR60, is it safe to say that the same applies here?
Status: RESOLVED → VERIFIED
Flags: needinfo?(davidp99)
The crash in bug 1449388 was due to Flash code and was fixed by Adobe in Flash 31.  Our patch in that bug created an API in Firefox that Adobe used in their fix.  The crash here was (assuming we've really squashed it) due to old Firefox code in bug 1251202.
In comment 2 I said that the work in bug 1449388 might have blocked this crash but now I think thats highly unlikely.  This bug (again, if we really caught it), is highly timing related (and at shutdown) so the best I can say is, before, we must have been getting lucky.
Flags: needinfo?(davidp99)
[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is a crash on shutdown with an easy fix based on proper ref-count maintenance.

User impact if declined: Occasional thread-timing-based crashes on shutdown when the user has been using Flash on Windows.

Fix Landed on Version: 64, beta uplift to 63

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): I might have said medium before this sat in nightly but any dumb mistakes should have shown up by now and the patch is pretty basic.

String or UUID changes made by this patch: none
Attachment #9017693 - Flags: approval-mozilla-esr60?
Comment on attachment 9017693 [details] [diff] [review]
Refactor AudioDeviceMessageRunnable's use of the PluginModuleSet -- esr60 version

Fix is looking good on Nightly/Beta, approved for 60.3esr also.
Attachment #9017693 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+][adv-esr60.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: