Child content process doesn't log anything when running with the Windows sandbox enabled.

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bobowen, Assigned: bobowen)

Tracking

(Depends on 1 bug)

unspecified
mozilla32
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

When pushing to try with the --enable-content-sandbox build config option and the --e10s test option, there is no logging from the child process.

The tests with the same build downloaded from the build server run fine locally.

I have the loan of a test slave to investigate the issue.
It looks like the tests are running and the child process is starting OK, but the output form the child process isn't being piped back to the main process / stdout, so it doesn't end up in the log files.
Sadly this is something I've seen happening before, it just wasn't relevant to what I was testing, so I didn't realise what was going on. :-(

I'm assuming that the the sandbox is stopping this, I tried to allow pipes on all paths, but with no joy.
I'll investigate further tomorrow.
The problem was that we weren't inheriting stdout and stderr from the parent process, so the logs just got lost.

The create process part in LaunchApp [1], for normal e10s, gets the handles directly.
In BrokerServicesBase::SpawnTarget [2] for the sandbox, it gets the handles from the policy, which defaults them to INVALID_HANDLE_VALUE.

This patch sets stdout and stderr at the same place the other policy settings are made.
Not sure if we would want to make this conditional in some way.
This will need to be reflected in bug 1007971, so I'll add this as a dependency.

This doesn't work for Windows XP, not sure what we'd want to do about that.
The standard e10s process would have the same problem, I believe.

Here's the try push:
https://tbpl.mozilla.org/?tree=Try&rev=04e7981384f1

Looks like it times out on a failure.
For Windows 7 there are 29 failures, but 52730 passes. :-)

[1] http://dxr.mozilla.org/mozilla-central/source/ipc/chromium/src/base/process_util_win.cc#303
[2] http://dxr.mozilla.org/mozilla-central/source/security/sandbox/win/src/broker_services.cc#340
Summary: Child process doesn't appear to start when running with Windows sandbox enabled. → Child content process doesn't log anything when running with the Windows sandbox enabled.
Comment on attachment 8422488 [details] [diff] [review]
Bug 1009452: inherit stdout and stderr into the content process to allow logging

See comment 2 for details and some thoughts/questions.
Attachment #8422488 - Flags: review?(aklotz)
Comment on attachment 8422488 [details] [diff] [review]
Bug 1009452: inherit stdout and stderr into the content process to allow logging

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

r=me with the added return value checks.

(In reply to Bob Owen (:bobowen) from comment #2)
> This doesn't work for Windows XP, not sure what we'd want to do about that.
> The standard e10s process would have the same problem, I believe.

Windows XP sucks with respect to HANDLE inheritance. Individual HANDLEs are marked inheritable, and then bInheritHandles passed into CreateProcess determines whether those marked handles are passed to the child process. Unless the parent process has been meticulous about tracking which handles have been marked as inheritable, it's risky to set bInheritHandles = TRUE, lest something gets inherited that we didn't want to be.

::: security/sandbox/win/src/sandboxbroker/sandboxBroker.cpp
@@ +59,5 @@
>    mPolicy->SetAlternateDesktop(false);
>  
> +  // Set stdout and stderr, to allow inheritance for logging.
> +  mPolicy->SetStdoutHandle(::GetStdHandle(STD_OUTPUT_HANDLE));
> +  mPolicy->SetStderrHandle(::GetStdHandle(STD_ERROR_HANDLE));

This looks fine, but can you please check the return values of these calls? I've filed bug 1011658 to add checks to the other policy settings in this function.
Attachment #8422488 - Flags: review?(aklotz) → review+
(In reply to Aaron Klotz [:aklotz] from comment #4)
> Comment on attachment 8422488 [details] [diff] [review]

Thanks for the review.
So, I guess we leave the XP case ... thanks for the explanation.
 
> This looks fine, but can you please check the return values of these calls?
> I've filed bug 1011658 to add checks to the other policy settings in this
> function.

I'll add this, but I'm unsure as to what I should do.

Unlike the other policy settings, where failure might affect the security of the sandbox, for these it just affects the logging.
Do you think I should just put out some sort of warning?
I need to work out how I'd do that, not sure if I can just use NS_WARNING here.
Flags: needinfo?(aklotz)
(In reply to Bob Owen (:bobowen) from comment #5)
> > This looks fine, but can you please check the return values of these calls?
> > I've filed bug 1011658 to add checks to the other policy settings in this
> > function.
> 
> I'll add this, but I'm unsure as to what I should do.
> 
> Unlike the other policy settings, where failure might affect the security of
> the sandbox, for these it just affects the logging.
> Do you think I should just put out some sort of warning?
> I need to work out how I'd do that, not sure if I can just use NS_WARNING
> here.

A fair point. I'm fine with leaving these as-is since they're not critical.
Flags: needinfo?(aklotz)
(In reply to Aaron Klotz [:aklotz] from comment #6)
> (In reply to Bob Owen (:bobowen) from comment #5)

> > Do you think I should just put out some sort of warning?
> > I need to work out how I'd do that, not sure if I can just use NS_WARNING
> > here.
> 
> A fair point. I'm fine with leaving these as-is since they're not critical.

OK, thanks.
It's probably fairly unlikely to fail anyway, but I think some sort of warning would be good.
I need to work out how we are going to log from the chromium sandbox code anyway.
So, I'll raise a bug for that and another follow-up one for this that depends on it, to add a warning if these fail.

Here's a limited try push as this should only affect people compiling with the sandbox:
https://tbpl.mozilla.org/?tree=Try&rev=de4180da78eb
Here's a limited try push as this should only affect people compiling with the sandbox:
https://tbpl.mozilla.org/?tree=Try&rev=de4180da78eb

Thanks.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/75ae09718c5c
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Depends on: 1014407
You need to log in before you can comment on or make changes to this bug.