Closed
Bug 1154876
Opened 9 years ago
Closed 9 years ago
Flash calls PPluginModuleChild::SendSetCursor() off the main thread
Categories
(Core Graveyard :: Plug-ins, defect)
Core Graveyard
Plug-ins
Tracking
(firefox37 wontfix, firefox38 wontfix, firefox38.0.5 wontfix, firefox39+ verified, firefox40+ verified, firefox41 verified, firefox-esr3139+ verified, firefox-esr3839+ verified, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-master unaffected)
VERIFIED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox37 | --- | wontfix |
firefox38 | --- | wontfix |
firefox38.0.5 | --- | wontfix |
firefox39 | + | verified |
firefox40 | + | verified |
firefox41 | --- | verified |
firefox-esr31 | 39+ | verified |
firefox-esr38 | 39+ | verified |
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: mccr8, Assigned: smichaud)
References
Details
(Keywords: csectype-race, sec-high, Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+])
Crash Data
Attachments
(2 files)
440.17 KB,
application/zip
|
Details | |
3.19 KB,
patch
|
spohl
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr31+
ritu
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
I came across this signature on crash-stats: https://crash-stats.mozilla.com/report/index/67b8450e-48d8-4b67-9fcc-dabd02150414#allthreads It looks like the Flash player is trying to send a message, but it hits an IPC thread assertion that Bill added recently.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(aklotz)
Reporter | ||
Comment 1•9 years ago
|
||
Another similar signature, also inside PPluginModuleChild::SendSetCursor(): https://crash-stats.mozilla.com/report/index/c2c850e1-96e4-4b78-b02a-265ce2150414
Reporter | ||
Comment 2•9 years ago
|
||
Another (sorry for the bug spam): https://crash-stats.mozilla.com/report/index/c6bfe48f-67bd-4a9e-afd3-1c88b2150414
Summary: Flash tries to send a message off the main thread → Flash calls PPluginModuleChild::SendSetCursor() off the main thread
Comment 3•9 years ago
|
||
I'm not all that familiar with the OSX interpose stuff. Clearly sending the message off the main thread is bad, but it's not clear to me if the interpose stuff should be proxying that to the main thread, or what. I'd be curious what smichaud has to say.
Flags: needinfo?(aklotz) → needinfo?(smichaud)
Assignee | ||
Comment 4•9 years ago
|
||
> but it's not clear to me if the interpose stuff should be proxying that to the main thread
I don't know, either. We *could*, if it's really necessary. But the added complexity might cause trouble down the line. We could also drop any hooked/interposed call that comes in on anything but the main thread, without crashing -- which would be a lot better than our current behavior.
As far as I know, none of the interposed calls is supported on anything but the main thread. So it's technically a bug to call them (as Flash does with -[NSCursor set]) on secondary threads. So we'd be well within our rights to drop them. But I don't know how much Flash depends on these calls working from secondary threads, and whether or not this is an issue for other plugins.
One way to find out would be to start dropping them and see what happens :-)
Jeromie, what do you think?
Flags: needinfo?(smichaud) → needinfo?(jeclark)
Comment 5•9 years ago
|
||
We have an ongoing engineering engagement happening between the Mozilla and Adobe engineering teams. Working together to devise an optimal solution for this seems like an ideal agenda item for the next time we get together.
Flags: needinfo?(jeclark)
Comment 6•9 years ago
|
||
As an aside, if you need contact information for the people on your side directly involved in that effort, I'd be more than happy to put you in touch.
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to comment #5) Sounds good to me. Thanks! I really hope Adobe can adopt something like the DOM Cursors spec (https://wiki.mozilla.org/NPAPI:DOMCursors). It would work around this problem, and also allow us to stop proxying cursor management methods from a background process to the main process. It was suggested a long time ago (for example it's mentioned at bug 621117 comment #57). Apparently it's gone nowhere, though -- which I think is really too bad.
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to comment #6) I don't know who's involved in these discussions on the Mozilla side. In another context you mentioned Johnny Stenback's name (jst@mozilla.com) as one of the people who might be involved. If he is, I'm sure he'd appreciate the information. I'm not involved in these discussions ... at least not yet. I'm not particularly eager to do so. But I'm happy to give advice, if it's needed.
Comment 9•9 years ago
|
||
Yep, let's try and get this on the list for the next time Johnny's guys come out. It far more effective to just have engineers look at the code together and hash out the most optimal solution.
Assignee | ||
Comment 10•9 years ago
|
||
Aklotz: In light of the future discussion with Adobe, it'd be very good if you (or someone) could explain here exactly why we don't want plugin-related IPC calls (like the one involved here) happening off the main thread.
Flags: needinfo?(aklotz)
Reporter | ||
Comment 11•9 years ago
|
||
I think that applies to all IPC, not just plugin-related. Billm would probably be a better person to ask.
Flags: needinfo?(wmccloskey)
Comment 12•9 years ago
|
||
I looked into this particular instance of NSCursor usage, and it appears that we set the arrow cursor before we show the file browse dialog (which we display from a background thread) to fix an issue with the cursor becoming invisible when the dialog was shown. Our bug notes mention that this was an issue in our standalone player (not the plugin), so a potential fix could be simply disabling that set cursor call in the plugin. Would need QE to verify that removing the call doesn't introduce the disappearing cursor issue (As I said, our bug notes only mention our standalone player having the issue, but it could have been an issue in the plugin as well) Of course, we could pursue the DOM Cursors work in parallel, but just wanted to mention that there is a potentially quicker fix if this is a troublesome crash that needs to be addressed sooner.
Well, our IPC code isn't threadsafe and we aren't going to change that. I think the alternatives are pretty simple, as Steven laid out: send the message on the main thread or drop it. Sending on the main thread seems like it would be fine, but dropping the message would be cleaner.
Flags: needinfo?(wmccloskey)
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(aklotz)
Assignee | ||
Comment 14•9 years ago
|
||
(In reply to comment #12) > it appears that we set the arrow cursor before we show the file > browse dialog Could you post a Flash "movie" that demonstrates this behavior, along with instructions on how to use it? In other words, could you post an example of a Flash movie that, when you click on a button somewhere in it, it causes a filepicker to be displayed? I'd like to test with it.
Flags: needinfo?(mtalistu)
Comment 15•9 years ago
|
||
.zip file with simple .html/.swf test case and .fla project. Clicking "Browse..." button opens a FileBrowse dialog for local file upload, which triggers a call to two NSCursor methods from a background thread.
Flags: needinfo?(mtalistu)
Assignee | ||
Comment 16•9 years ago
|
||
Here's a possible fix for this bug. It's very simple -- it just drops all calls to the hooked/interposed methods that come in on secondary threads. Mtalistu, I tested this patch with your testcase, and didn't see any problems. So it's possible we can drop these calls without having any ill effect on Flash. I've started a set of tryserver builds: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b73a8618ec14 A test build should eventually be available. When it is, I'll post a link to it.
Assignee | ||
Comment 17•9 years ago
|
||
Here's a link to my test build: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-b73a8618ec14/try-macosx64/firefox-40.0a1.en-US.mac.dmg Please try it, mtalistu, and let us know your results.
Flags: needinfo?(mtalistu)
Comment 18•9 years ago
|
||
Thanks, I will ask QE to do some tests with this build. Will let you know the results.
Comment 19•9 years ago
|
||
For what it's worth: the imgur.com uploader triggered this and it seems to behave fine with the patch here.
Reporter | ||
Comment 20•9 years ago
|
||
This seems potentially bad, and is fairly easy to trigger, so I'm going to be conservative and mark it sec-high.
Keywords: csectype-race,
sec-high
Reporter | ||
Updated•9 years ago
|
Comment 21•9 years ago
|
||
Ok, so the simple test case I provided earlier was related to an issue in the Standalone player, but we identified another issue that was fixed by the calls to NSCursor API that is broken in the test build of Firefox. Here are the repro steps. The mouse is visible when you switch tabs in Release Firefox, but it stays hidden in the test build of Firefox. I had to click on another app, like a Finder window, to get the mouse to become visible again. Method: 1. Open a Firefox window with multiple tabs opened. 2. Go to http://cbtcafe.com/flash/mousecursor/mousecursor.html 3. Mouse over the flash movie(sample swf for 'Flash: Mouse.Hide meets With'), The mouse pointer should disappear. 4. Switch tabs using command-option-arrow 5. Move the mouse Result: The mouse cursor disappears. Have to move the mouse out of this tab or browser to get the cursor back Expected: mouse arrow cursor should be visible.
Flags: needinfo?(mtalistu)
Assignee | ||
Comment 22•9 years ago
|
||
(In reply to comment #21) I see this bug even without my patch (for example in the 2015-04-22 and 2015-04-17 mozilla-central nightlies). So I suspect it's unrelated. I'll look for a regression range. If/when I find one, I'll open a new bug, paste in your STR, and CC you on it.
Assignee | ||
Comment 23•9 years ago
|
||
Actually it's my patch for bug 1130435 that triggered the behavior you describe in comment #21 ... which means that it *is* related to this bug, indirectly. I'm very puzzled how that patch could have caused/triggered *any* bug. So I need to get to the bottom of this, over the next few days (and probably into next week). Once I do, I'll (probably) open another bug, which I may or may not mark security-related, depending on what I find.
Assignee | ||
Comment 24•9 years ago
|
||
(Following up comment #23) I was right the first time: The bug you (mtalistu) report in comment #21 is completely unrelated. It's still our bug, though -- which I think is caused by us not sending an NPCocoaEventFocusChanged event to Flash in step 4 when running in e10s mode. Notice that the bug doesn't happen in non-e10s mode. When the bug doesn't happen in step 4 (and Flash does receive an NPCocoaEventFocusChanged event), Flash calls +[NSCursor unhide] *on the main thread* (so my patch from comment #16 wouldn't drop the call). I'm working on a related bug at bug 1153145. I'll make sure that whatever fix I come up with there also fixes this bug. The true trigger for this bug (and probably also for bug 1153145) is bug 1092630. My patch for bug 1130435 isn't the trigger. Before it landed, *no* native cursor calls from Flash (or any plugin) worked in e10s mode -- including the call to +[NSCursor hide] that happens when (in step 3) you move the mouse over the newCursor block. So before that patch landed you'd see *both* the custom cursor and the default native cursor (the northwest-pointing arrow) just above it. And it didn't matter when the call to +[NSCursor unhide] didn't happen in step 4, and the custom cursor disappeared -- because the default native cursor was already visible.
Assignee | ||
Comment 25•9 years ago
|
||
So as best I can now tell, my patch from comment #21 will have no ill effects on Flash. Any objections to my seeking review for that patch and landing it on the trunk?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mtalistu)
Assignee | ||
Comment 27•9 years ago
|
||
We may have STR for this bug at bug 1159800.
Assignee | ||
Comment 28•9 years ago
|
||
I should have said we may have STR for this bug's crashes at bug 1159800.
Reporter | ||
Comment 29•9 years ago
|
||
Any chance we can move forward with this?
Assignee | ||
Comment 30•9 years ago
|
||
I'm ready to land my patch from comment #16 ... though it may have bitrotted.
Assignee | ||
Updated•9 years ago
|
Attachment #8593605 -
Flags: review?(spohl.mozilla.bugs)
Updated•9 years ago
|
Attachment #8593605 -
Flags: review?(spohl.mozilla.bugs) → review+
Assignee | ||
Comment 31•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads I will land this when mozilla-inbound reopens.
Reporter | ||
Comment 32•9 years ago
|
||
Thanks! It would also be good to get this at least on Aurora at some point, as the thread safety assertions have made it that far along.
Assignee | ||
Comment 33•9 years ago
|
||
> It would also be good to get this at least on Aurora at some point
I'll let the patch bake a while on trunk, then request permission to uplift.
Note that we could also request uplift to beta, since the problem also exists there.
Assignee | ||
Comment 34•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads Landed on mozilla-inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/3950e0f52532 It's frankly hard to land a patch like this without giving the game away. I tried to disguise it a bit by giving it an obscure commit message, instead of just a bug number (which would have drawn peoples' attention).
Updated•9 years ago
|
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment 36•9 years ago
|
||
This should have had sec-approval before landing. Please request it retroactively. Also, does this affect esr31? Please nominate this for Aurora/Beta/esr38 (and esr31 if applicable) ASAP.
Assignee: nobody → smichaud
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
status-firefox-esr31:
--- → ?
status-firefox-esr38:
--- → affected
tracking-firefox-esr31:
--- → ?
tracking-firefox-esr38:
--- → ?
Flags: needinfo?(smichaud)
Assignee | ||
Comment 37•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads Sorry for overlooking this :-( [Security approval request comment] How easily could an exploit be constructed based on the patch? Fairly easily, I think, but only from a malicious plugin (which could, of course, do all other kinds of nasty things). 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? This is really a Flash bug. So all branches are effected that support OOP plugins. If not all supported branches, which bug introduced the flaw? Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? The same patch should apply to all current branches -- the code they patch is quite old and hasn't changed much recently. How likely is this patch to cause regressions; how much testing does it need? It's very unlikely to cause regressions in Flash -- Adobe has already had a chance to test with a test build that contained the patch. But if other plugins (for example Silverlight) have the same bug, this patch may also cause (apparent) regressions in them.
Flags: needinfo?(smichaud)
Attachment #8593605 -
Flags: sec-approval?
Updated•9 years ago
|
Assignee | ||
Comment 38•9 years ago
|
||
> Also, does this affect esr31? Please nominate this for
> Aurora/Beta/esr38 (and esr31 if applicable) ASAP.
It effects all branches -- since it's really a Flash bug.
Since other plugins may also have the same bug, I'd like to let this
bake on the trunk for a while (say a week) before requesting uplift.
The most important other plugin is Silverlight. So I'll do some
simple tests with Silverlight to see if it also has this bug.
Comment 39•9 years ago
|
||
OK, I've verified locally that this rebases all the way to esr31 w/o issue. Will leave it to you and Al to determine proper bake time.
Assignee | ||
Comment 40•9 years ago
|
||
> How easily could an exploit be constructed based on the patch? > > Fairly easily, I think, but only from a malicious plugin (which > could, of course, do all other kinds of nasty things). Let me revise this. I think it'd be fairly easy to crash the main process. But I really don't know how easy it'd be do an exploit. Others who know more about this than I do may be able to comment.
Comment 41•9 years ago
|
||
I'll give sec-approval+ on this after the fact (thanks for filling out the form). I agree with letting it bake a week on trunk. We'll want this on Aurora, Beta, ESR38, and (hopefully) ESR31 after that.
tracking-firefox39:
--- → +
tracking-firefox40:
--- → +
Updated•9 years ago
|
Attachment #8593605 -
Flags: sec-approval? → sec-approval+
Assignee | ||
Comment 42•9 years ago
|
||
I tested briefly with both the Silverlight and Unity plugins, with a debug logging version of my patch (which logs to console and stdout whenever it drops a hooked event that comes in on a secondary thread), in both e10s and non-e10s mode. I didn't see any drops at all. For Silverlight I played around with http://demos.telerik.com/silverlight/. QA has a Netflix test account, to which I have access -- but it apparently no longer works, so I was unable to test with it. For Unity I played around with some of the demos at https://unity3d.com/showcase/live-demos.
Comment 43•9 years ago
|
||
Steven can you request uplift for this if you think it's ready to go? Thanks!
Flags: needinfo?(smichaud)
Assignee | ||
Comment 44•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Potential trouble from malicious plugins Fix Landed on Version: 41 Risk to taking this patch (and alternatives if risky): Low to moderate risk. It's impossible to know how many other plugins than Flash have this bug (the bug this patch works around) without actually getting this patch into a release. But the risk for the most important plugins (Flash, Silverlight, Unity) is low. 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 #]: Plugin bug [User impact if declined]: Potential trouble from malicious plugins [Describe test coverage new/current, TreeHerder]: baked for a week on m-c [Risks and why]: Low to moderate. See above. [String/UUID change made/needed]: none
Flags: needinfo?(smichaud)
Attachment #8593605 -
Flags: approval-mozilla-esr38?
Attachment #8593605 -
Flags: approval-mozilla-beta?
Attachment #8593605 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(mtalistu)
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads Approving to uplift to Aurora, Beta and ESR38+ as this is a sec-high bug and the fix has been in m-c for a week without showing any negative impact.
Attachment #8593605 -
Flags: approval-mozilla-esr38?
Attachment #8593605 -
Flags: approval-mozilla-esr38+
Attachment #8593605 -
Flags: approval-mozilla-beta?
Attachment #8593605 -
Flags: approval-mozilla-beta+
Attachment #8593605 -
Flags: approval-mozilla-aurora?
Attachment #8593605 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 46•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads Landed on mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/583c3552c9e4
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 47•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads Landed on mozilla-beta: https://hg.mozilla.org/releases/mozilla-beta/rev/c9cc5b1e1b0f
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 48•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads Landed on mozilla-esr38: https://hg.mozilla.org/releases/mozilla-esr38/rev/2fa7846306f2
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 49•9 years ago
|
||
> We'll want this on Aurora, Beta, ESR38, and (hopefully) ESR31 after that.
How about ESR31? Should I request uplift for that now, or should I wait?
Flags: needinfo?(abillings)
Assignee | ||
Comment 51•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads If this is not a sec:{high,crit} bug, please state case for ESR consideration: User impact if declined: Potential trouble from malicious plugins Fix Landed on Version: 41 Risk to taking this patch (and alternatives if risky): Low to moderate risk. It's impossible to know how many other plugins than Flash have this bug (the bug this patch works around) without actually getting this patch into a release. But the risk for the most important plugins (Flash, Silverlight, Unity) is low. String or UUID changes made by this patch: none See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8593605 -
Flags: approval-mozilla-esr31?
Updated•9 years ago
|
Flags: qe-verify+
Updated•9 years ago
|
Attachment #8593605 -
Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Assignee | ||
Comment 52•9 years ago
|
||
Comment on attachment 8593605 [details] [diff] [review] Possible fix -- drop calls from secondary threads Landed on mozilla-esr31: https://hg.mozilla.org/releases/mozilla-esr31/rev/772f5b18522c
Updated•9 years ago
|
Comment 53•9 years ago
|
||
I`m unable to reproduce the initial crash using old Nightly (2015-04-15) on Windows 7 64-bit, Windows XP 32-bit, Ubuntu 14.04 32-bit. Socorro shows many crashes with this signature (73 crashes in the last 7 days), most of them on Windows XP and latest builds: https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aipc%3A%3AMessageChannel%3A%3ACxxStackFrame%3A%3ACxxStackFrame%28mozilla%3A%3Aipc%3A%3AMessageChannel%26%2C+mozilla%3A%3Aipc%3A%3ADirection%2C+IPC%3A%3AMessage+const%2A%29#tab-sigsummary Any thoughts on this?
Flags: needinfo?(smichaud)
Assignee | ||
Comment 54•9 years ago
|
||
> Any thoughts on this?
Those crash stacks are completely unrelated to this bug. It's yet another case of the so-called "signature" being misleading.
Even if they *weren't* unrelated, though, they'd need to go into another bug: The specific problem that this bug reports (and which my patch fixes) is Mac-specific.
Flags: needinfo?(smichaud)
Comment 55•9 years ago
|
||
(In reply to Steven Michaud [:smichaud] from comment #54) > The specific problem that this bug reports (and which my patch fixes) > is Mac-specific. I retested using old Nightly (2015-04-15) on Mac OS X 10.9.5 and I was able to get a crash: https://crash-stats.mozilla.com/report/index/f1868dc0-6a52-4027-b8b1-ebd0d2150617 Verified that using the attached testcase and latest builds the crash did not occur: - Firefox 31.7 esr: Built from https://hg.mozilla.org/releases/mozilla-esr31/rev/4028bc28f4c6 20150615064244 - Firefox 38.0.1 esr: Built from https://hg.mozilla.org/releases/mozilla-esr38/rev/bbac190ad1e7 20150616140147 - latest Nightly 41.0a1 - latest Aurora 40.0a2 - Firefox 39 beta 6
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Assignee | ||
Comment 56•9 years ago
|
||
(Following up comment #24) Mtalistu, the bug you report in comment #21 is fixed by my patch for bug 1153145. That patch is now in current mozilla-central and mozilla-aurora (i.e. Developer Edition) nightlies, in case you care to test with them.
Updated•9 years ago
|
Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+]
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
•