1146874 - Windows process sandbox should check that the process has launched successfully.

Status: VERIFIED FIXED

(Core :: Security: Process Sandboxing, defect)

Description

[Bob Owen (:bobowen)]

The sandbox broker should check that the process has launched successfully before attempting to resuming the process.

Comment 1

[Bob Owen (:bobowen)]

I have a fix for the sandboxBroker.cpp, but it seems that the rest of the content IPC set-up doesn't handle the failure very well and crashes elsewhere.

So, I need to find a fix for that as well.

Comment 2

[Bob Owen (:bobowen)]

Attachment: Part 1: Check that Windows sandboxed process starts correctly.

This used to crash if SpawnTarget() failed because targetInfo isn't initialised and we attempted to close a bogus handle.

I've initialised it even though we're now checking the return value for SpawnTarget.

Comment 3

[Bob Owen (:bobowen)]

Attachment: Part 2: Add new nspr logging in GMPParent when process fails to launch.

Once the sandbox broker is fixed EME seems to handle this OK now.
As in, the video fails to play, but Firefox doesn't crash.

All I've done here is add a bit of extra logging and change some of the current logging to better reflect what's going on.

I think we should get this and part 1 uplifted to 38.0.5, 39 and possibly esr38, while we're looking into fixing the child process start-up problems.

One thing I noticed was that it didn't appear to attempt to relaunch the child process when the video was retried, even in a new tab.

Comment 4

[Bob Owen (:bobowen)]

Attachment: Part 3: Add LaunchSubprocess function to ContentParent to remove fallible code from constructor.

In non-e10s mode - this at least stops the thumbnail content process from crashing Firefox if it fails to start.

In e10s mode - while Firefox doesn't crash, obviously no pages get rendered.
We'll need a follow-up to handle this scenario better, possibly by triggering the standard child crash behaviour.
The same possibly goes for the GMP case.

Comment 5

[Bob Owen (:bobowen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ffc5628a8927

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d036289fac1
https://hg.mozilla.org/integration/mozilla-inbound/rev/c049d77c06fd
https://hg.mozilla.org/integration/mozilla-inbound/rev/61a905db2071

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d036289fac1
https://hg.mozilla.org/mozilla-central/rev/c049d77c06fd
https://hg.mozilla.org/mozilla-central/rev/61a905db2071

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8603357 [details] [diff] [review]
Part 1: Check that Windows sandboxed process starts correctly.

Approval Request Comment
[Feature/regressing bug #]:
Since the sandbox broker landed, so bug 925571.
Wasn't used in release until bug 985252 for the GMP sandbox.

[User impact if declined]:
Any failure in the start-up / initialisation of a sandboxed child process will probably cause Firefox to crash.
This should only be for GMP processes outside of Nightly.
To be clear this bug doesn't address any potential sandboxed child start-up issues, it just tries to prevent them from totally crashing Firefox.

[Describe test coverage new/current, TreeHerder]:
GMP (for openh264 and EME) has mochitests.
Have done fairly simple manual tests with results as described in comment 3.
I caused the child process to fail by simply renaming plugin-container.exe.

[Risks and why]: 
low to medium: fairly small return value checking and logging changes, but it might cause other unforeseen issues as the child process has failed to start.
However, it won't crash Firefox.

[String/UUID change made/needed]:
None

Comment 9

[Bob Owen (:bobowen)]

Comment on attachment 8603362 [details] [diff] [review]
Part 2: Add new nspr logging in GMPParent when process fails to launch.

See comment 8.

To be clear, I don't think it is worth uplifting Part 3, as no Windows content processes should be sandboxed outside of Nightly.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8603357 [details] [diff] [review]
Part 1: Check that Windows sandboxed process starts correctly.

Approved for uplift to aurora and let's see how plugin stability is affected. We have a short beta cycle for 39, but could still uplift this for early beta next week.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Bob, I know you say unforseen problems, but do you think this might break by only breaking plugin content, like a video, or failing to render a whole page? What do you think we can do here for manual testing or verification for QE to try to break this?

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8603362 [details] [diff] [review]
Part 2: Add new nspr logging in GMPParent when process fails to launch.

Approved for uplift to aurora

Comment 13

[Bob Owen (:bobowen)]

(In reply to Liz Henry (:lizzard) from comment #11)
> Bob, I know you say unforseen problems, but do you think this might break by
> only breaking plugin content, like a video, or failing to render a whole
> page? What do you think we can do here for manual testing or verification
> for QE to try to break this?

It should only break the playing of the GMP content and only in a situation where we would have crashed completely before this fix.

The easiest way to test this is by temporarily renaming the plugin-container.exe file.
It lives in the same directory as firefox.exe (something like "C:\Program Files (x86)\Firefox Developer Edition\").
(Obviously, you can't use this trick with e10s turned on in Nightly. :-) )
This will cause the child process start-up to fail.
Before this fix Firefox will crash, afterwards just the video will fail.

I don't think that the fact that the child has failed to start is likely to cause further problems (other than for GMP).
However, Chris might be in a better situation to answer that.
(It's already the weekend in NZ, so he may not get back to us before Monday.)

Comment 14

[Bob Owen (:bobowen)]

Oh, one other thing.

If the thumbnail content process happens to kick-in (I think this can only happen on a new tab page) while you've got plugin-container.exe renamed, then that will cause Firefox to crash.

I don't think that happens very often though.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Bob Owen (:bobowen) from comment #13)
> (In reply to Liz Henry (:lizzard) from comment #11)
> > Bob, I know you say unforseen problems, but do you think this might break by
> > only breaking plugin content, like a video, or failing to render a whole
> > page?

If this causes a regression, it should only break the stuff in <video>s feed by EME or OpenH264 plugins, I doubt it could possibly break much else. I think this is fairly low risk, and that we should uplift it to 39.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cbf89fb08ec7
https://hg.mozilla.org/releases/mozilla-aurora/rev/add2083dc0ff

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8603362 [details] [diff] [review]
Part 2: Add new nspr logging in GMPParent when process fails to launch.

Parts 1 and 2 approved for uplift to beta (39).

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8603357 [details] [diff] [review]
Part 1: Check that Windows sandboxed process starts correctly.

Approved for uplift to beta (39) to help improve crash issues.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6f78a86e9179
https://hg.mozilla.org/releases/mozilla-beta/rev/3cc15b5aa2cd

Comment 20

[Catalin Varga [QA][:VarCat]]

I've tried reproducing this bug by modifying plugin-container.exe for FF 39 Aurora Build Id: 20150511004005 and tried playing h264 videos on Flash, for example http://www.flashvideofactory.com/test/demofullscreen345.html . 
But I never managed to make Firefox crash, the video is just not playing. Did I missed something? is there a more reliable way to reproduce the bug?

Comment 21

[Bob Owen (:bobowen)]

(In reply to Catalin Varga [QA][:VarCat] from comment #20)
> I've tried reproducing this bug by modifying plugin-container.exe for FF 39
> Aurora Build Id: 20150511004005 and tried playing h264 videos on Flash, for
> example http://www.flashvideofactory.com/test/demofullscreen345.html . 
> But I never managed to make Firefox crash, the video is just not playing.
> Did I missed something? is there a more reliable way to reproduce the bug?

Hi, we don't use our sandbox for Flash at the moment, so this bug didn't affect Flash.
(Although it's good to know that it doesn't crash Firefox when tested in this way.)

You need to start a Gecko Media Plugin (GMP) process, which for openh264 you can do via:

http://mozilla.github.io/webrtc-landing/pc_test.html
Ticking the "Require H.264 video" checkbox.

For EME I think you should be able to use:

http://people.mozilla.org/~cpearce/mse-clearkey/

Comment 22

[Catalin Varga [QA][:VarCat]]

Thanks a lot for the quick help.
I verified the bug using the following environment:

FF 39.0b1
Build Id: 20150520170903
Os: Win 7 x64, Win 8.1 x86

Comment 23

[Camelia Badau [:cbadau], Desktop Test Engineering]

Verified fixed on latest Aurora 41.0a2 (buildID: 20150729004002) and Firefox 40 Beta 8 (buildID:  	20150727174134).