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)
Tracking
()
VERIFIED
FIXED
mozilla64
People
(Reporter: philipp, Assigned: handyman)
References
Details
(4 keywords, Whiteboard: [post-critsmash-triage][adv-main63+][adv-esr60.3+])
Crash Data
Attachments
(2 files)
46 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
4.95 KB,
patch
|
RyanVM
:
approval-mozilla-esr60+
|
Details | Diff | Splinter Review |
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.
Comment 1•6 years ago
|
||
David, can you please take a look?
Group: core-security → dom-core-security
Flags: needinfo?(davidp99)
Assignee | ||
Comment 2•6 years ago
|
||
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
Assignee | ||
Comment 3•6 years ago
|
||
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.
Assignee | ||
Comment 4•6 years ago
|
||
FYI, there is really no way to reproduce the crash, which is timing related. The bug very rarely happens at shutdown.
Comment 5•6 years ago
|
||
AFAICT, ESR60 should also be affected by this issue. Can you suggest a severity rating, David? https://wiki.mozilla.org/Security_Severity_Ratings
Assignee | ||
Comment 6•6 years ago
|
||
Sure -- timing and plugin factors suggest sec-moderate
Keywords: sec-moderate
Assignee | ||
Updated•6 years ago
|
Keywords: checkin-needed
Comment 7•6 years ago
|
||
https://hg.mozilla.org/integration/autoland/rev/87fec627956b
Keywords: checkin-needed
Assignee | ||
Comment 8•6 years ago
|
||
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 9•6 years ago
|
||
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?
Assignee | ||
Comment 10•6 years ago
|
||
Thanks. Somehow, I managed to completely avoid learning that.
Comment 11•6 years ago
|
||
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
Assignee | ||
Comment 12•6 years ago
|
||
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 13•6 years ago
|
||
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+
Comment 14•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/2704bbd93c5655757d331b7463bcbf49ab497956
Updated•6 years ago
|
Updated•6 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Comment 15•6 years ago
|
||
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)
Assignee | ||
Comment 16•6 years ago
|
||
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)
Assignee | ||
Comment 17•6 years ago
|
||
[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 18•6 years ago
|
||
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+
Comment 19•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-esr60/rev/4a99c842c818
Updated•6 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63+][adv-esr60.3+]
Updated•4 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•