1303755 - Flash code using GetKeyState crashes attempting IPC from non-main thread on Windows

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[David Parks [:handyman]]

STR:
0. Run Firefox 64-bit on Windows (I'm using Win10).

1. Visit a page with a relevant plugin.  For example:
http://www.tinywebgallery.com/en/tfu/web_demo1.php
or
http://imagevenue.com/

2. Choose "Add Files" (tinywebgallery.com) or "Browse" (inagevenue.com)

EXPECTED:

A file picker will appear.

ACTUAL:

CRASH!!!

---------------

The issue is some recent proxying code that tries to run GetKeyState in the chrome process.  The proxy code usually works fine.  It came from Bug 1180684 and, in the examples in that bug, the call happens on the main thread.  However, it happens on an ntdll worker thread in the pages listed above.

The fix forwards the request to the main thread and synchronously waits for its result using a semaphore.

Comment 1

[David Parks [:handyman]]

Attachment: Request GetKeyState run on main thread

This looks right to me...

Comment 2

[David Parks [:handyman]]

(Should have done this before...)

TESTS:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cf6f1dc787bd
https://treeherder.mozilla.org/#/jobs?repo=try&revision=50143e42d536

Comment 3

[Jim Mathies [:jimm]]

Comment on attachment 8792533 [details] [diff] [review]
Request GetKeyState run on main thread

Review of attachment 8792533 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/plugins/ipc/PluginModuleChild.cpp
@@ +2108,5 @@
> +        // Start a semaphore at 0.  We Release the semaphore (bringing its count to 1)
> +        // when the synchronous call is done.
> +        HANDLE semaphore = CreateSemaphore(NULL, 0, 1, NULL);
> +        if (semaphore == nullptr) {
> +            NS_RUNTIMEABORT("Failed to create semaphore.");

this seems a bit overkill, do we really want to terminate in this case? maybe we should just MOZ_ASSERT.

@@ +2122,5 @@
> +            return keyState;
> +        }
> +        PLUGIN_LOG_DEBUG(("Error while waiting for GetKeyState semaphore: %d",
> +                          GetLastError()));
> +        NS_RUNTIMEABORT("Problem while waiting for GetKeyState semaphore.");

same here.

Comment 4

[David Parks [:handyman]]

Attachment: Request GetKeyState run on main thread (r+ by jimmathies)

With MOZ_ASSERT instead of NS_RUNTIMEABORT

Comment 5

[David Parks [:handyman]]

r+ in comment 3

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ced14be69f76
Flash code using GetKeyState crashes attempting IPC from non-main thread. r=jimm

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ced14be69f76

Comment 8

[Gerry Chang [:gchang]]

This needs QE's help verify it in nightly first.

Comment 9

[Makoto Kato [:m_kato]]

:handyman, thanks for fixing this.  Could you request uplift for Aurora 51 since this is regression by bug 1180684?

Comment 10

[David Parks [:handyman]]

Comment on attachment 8792656 [details] [diff] [review]
Request GetKeyState run on main thread (r+ by jimmathies)

Approval Request Comment
[Feature/regressing bug #]: 1180684
[User impact if declined]: GetKeyState calls sometimes cause plugins to crash their process
[Describe test coverage new/current, TreeHerder]: Not covered
[Risks and why]: Minimal.  Expected to only affect behavior that would have crashed anyway.
[String/UUID change made/needed]: N/A

Comment 12

[Gerry Chang [:gchang]]

Hi Florin,
Can we have some QA's help to verify this?

Comment 13

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I investigated the issue on latest Nightly 52.0a1 (2016-09-27) (64-bit), using Windows 10 x64. 
With e10s on, there are no issues; but after switching e10s off and clicking the "Add Files" button (http://www.tinywebgallery.com/en/tfu/web_demo1.php) several times, the browser becomes unresponsive; the problem persists after restart and even after switching e10s back to "on" state. 
No problems came out using http://imagevenue.com/.

Comment 14

[Gerry Chang [:gchang]]

Hi :handyman,
Can you look at the issue found by QE? Is it a separated issue from this bug?

Comment 15

[David Parks [:handyman]]

Best I can tell, the issue QA found in comment 13 is due to Bug 1305490, which is a bug I initially reported when working on bug 1284897.

What I see:

* With this patch, QA reports a non-e10s issue but I am able to get the same failure with e10s active.  To see this w/ e10s on, I usually have to repeat pressing "Add Files" and then pressing "Cancel" several times (up to a dozen times -- it usually takes two tries before failure when e10s is not active).  In both the e10s and non-e10s cases, the failure is severe -- the chrome process hangs so the entire browser is trashed.

* Without the patch (using a 9/18 build), with e10s, I (usually) get the crash reported by this bug immediately.  Without e10s, however, the 9/18 build won't crash but will hang, showing the same behavior QA reported here for non-e10s with the patch.

* With the latest code (including this patch), running in the debugger, I immediately hit the ASSERT reported in bug 1305490.  The debugger cannot proceed from that point.  Removing the ASSERT and returning 'true' from that function (i.e. trying to skip over that bug to see what happens) leads to an IPC hang.

---

To sum up, I don't think that the issue found is related to this patch -- indeed, it existed in builds before the patch landed.  I believe the issue *is* related to bug 1305490.  I do not believe the patch exacerbates any failures that don't exist without it.

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8792656 [details] [diff] [review]
Request GetKeyState run on main thread (r+ by jimmathies)

Fix a crash. Take it in 51 aurora.

Comment 17

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

Thanks, :handyman for your answer! 
Based on comment 15, this bug is verified fixed on Nightly 52. I will set the corresponding flags.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b7499f30d9b2

Comment 19

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

The issue is verified fixed on latest Aurora 51.0a2 (2016-10-04), using Windows 10 x64.