Linux: inform the Zygote when it's waiting on a dead process

Linux: inform the Zygote when it's waiting on a dead process

If the browser calls ProcessDied() and asks the Zygote to wait (without blocking)
on a dead process, the kernel might not be done destroying it and the Zygote may
mistakenly claim that the process is alive.

We now inform the Zygote over the IPC that the process is already dead so
that it can wait synchroneously.

BUG=157458
NOTRY=true


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=171450

[jln (very slow on Chromium), 2012-11-30 01:51:16]

Adam, do you mind taking a look ?

I'm not super happy with it, but I really don't know what else to do.

[jln (very slow on Chromium), 2012-12-04 07:11:19]

On 2012/11/30 01:51:16, Julien Tinnes wrote:
> Adam, do you mind taking a look ?
> 
> I'm not super happy with it, but I really don't know what else to do.

Adam, friendly ping?

[agl, 2012-12-04 15:27:55]

Sorry, didn't notice the CL as well as the bug-thread.

I'm not a base/ OWNERS, but I feel bad about adding yet another wrapper around
waitpid.

The "should_be_dead" is a wart. I've checked the calls to GetTerminationStatus
to try and find ones where they won't already know that the process is dead.

There's a couple in unittests that immediately kill the process anyway if it's
not dead.

And then there's
browser/extensions/api/messaging/native_message_process_host_posix.cc, which
appears to be checking the process state on every message send? That just
appears to be a significant error.

I'll LGTM in the current state if you want, but I don't think a base/ OWNERs
will.

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/bro...
File content/browser/browser_child_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/bro...
content/browser/browser_child_process_host_impl.cc:197: return
child_process_->GetChildTerminationStatus(false, exit_code);
comment the bool?

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/chi...
File content/browser/child_process_launcher.h (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/chi...
content/browser/child_process_launcher.h:57: // it.
looks like "it" fits on the previous line.

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/chi...
content/browser/child_process_launcher.h:58: // |known_dead| can be true if we
do already know the process is dead as it
s/do //

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:897:
ProcessDied(false);
comment bool?

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/zyg...
File content/browser/zygote_host/zygote_host_impl_linux.h (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/zyg...
content/browser/zygote_host/zygote_host_impl_linux.h:41: // is already dead
without waiting synchroneously for it.
synchronously

[jln (very slow on Chromium), 2012-12-04 20:01:25]

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/bro...
File content/browser/browser_child_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/bro...
content/browser/browser_child_process_host_impl.cc:197: return
child_process_->GetChildTerminationStatus(false, exit_code);
On 2012/12/04 15:27:55, agl wrote:
> comment the bool?

Done.

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/chi...
File content/browser/child_process_launcher.h (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/chi...
content/browser/child_process_launcher.h:57: // it.
On 2012/12/04 15:27:55, agl wrote:
> looks like "it" fits on the previous line.

Done.

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/chi...
content/browser/child_process_launcher.h:58: // |known_dead| can be true if we
do already know the process is dead as it
On 2012/12/04 15:27:55, agl wrote:
> s/do //

Done.

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/ren...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/ren...
content/browser/renderer_host/render_process_host_impl.cc:897:
ProcessDied(false);
On 2012/12/04 15:27:55, agl wrote:
> comment bool?

Done.

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/zyg...
File content/browser/zygote_host/zygote_host_impl_linux.h (right):

https://chromiumcodereview.appspot.com/11316261/diff/7002/content/browser/zyg...
content/browser/zygote_host/zygote_host_impl_linux.h:41: // is already dead
without waiting synchroneously for it.
On 2012/12/04 15:27:55, agl wrote:
> synchronously

Done.

[jln (very slow on Chromium), 2012-12-04 20:08:41]

On 2012/12/04 15:27:55, agl wrote:
> Sorry, didn't notice the CL as well as the bug-thread.
> 
> I'm not a base/ OWNERS, but I feel bad about adding yet another wrapper around
> waitpid.
>
> The "should_be_dead" is a wart. I've checked the calls to GetTerminationStatus
> to try and find ones where they won't already know that the process is dead.
> 
> There's a couple in unittests that immediately kill the process anyway if it's
> not dead.
> 
> And then there's
> browser/extensions/api/messaging/native_message_process_host_posix.cc, which
> appears to be checking the process state on every message send? That just
> appears to be a significant error.

So you're suggesting that we could try to transform GetTerminationStatus in
WaitTerminationStatus at all times?

Remember, all platforms have GetTerminationStatus, I don't know if they can all
support WaitTerminationStatus easily.

Moreover, even if we *know* a process is dead, a lot could go wrong if we always
use WaitTerminationStatus instead. Especially because of our security model. I
don't know this area very well, but a compromised renderer could lie (or close
its IPC socket) to get a browser thread wait synchronously on it and make the
browser non responsive.

> I'll LGTM in the current state if you want, but I don't think a base/ OWNERs
> will.

If your only remaining issue is the new API in base/ for POSIX, please let me
know and I'll add OWNERS to discuss it.

[agl, 2012-12-04 22:20:44]

On 2012/12/04 20:08:41, Julien Tinnes wrote:
> Moreover, even if we *know* a process is dead, a lot could go wrong if we
always
> use WaitTerminationStatus instead. Especially because of our security model. I
> don't know this area very well, but a compromised renderer could lie (or close
> its IPC socket) to get a browser thread wait synchronously on it and make the
> browser non responsive.

(I'm assuming that we'll SIGKILL it as this patch currently does.)

> If your only remaining issue is the new API in base/ for POSIX, please let me
> know and I'll add OWNERS to discuss it.

LGTM then.

I don't think my unease is due to this CL specifically. I think the whole area
is a bit of a mess. I caused some of that, but it's certainly gotten worse over
time.


Cheers

AGL

[jln (very slow on Chromium), 2012-12-04 23:30:52]

sky: would you mind taking a look as owner of content/browser ?
brettw: would you mind taking a look at base/ ?

[sky, 2012-12-05 00:47:08]

LGTM

[brettw, 2012-12-05 19:09:49]

base lgtm rubberstamp based on agl's review

[jln (very slow on Chromium), 2012-12-05 19:36:11]

On 2012/12/05 19:09:49, brettw wrote:
> base lgtm rubberstamp based on agl's review

Thanks Brett.

Just double checking: did you notice in the thread that Adam foresaw an issue
with adding a new POSIX API to base/ and that we deferred to base/ OWNERS?

[brettw, 2012-12-05 20:31:35]

I don't think it's a security hole if a compromised renderer can hang the
browser process, this is probably already possible.

[jln (very slow on Chromium), 2012-12-05 20:36:17]

On 2012/12/05 20:31:35, brettw wrote:
> I don't think it's a security hole if a compromised renderer can hang the
> browser process, this is probably already possible.

At the moment we do need to support both though. The "FastShutdownIfPossible()"
mechanism does require to wait on a non dead process. Maybe this could be
re-worked, but I don't know if it's worth it.

I think adding WaitForTerminationStatus() to base/ is something be might need at
some point anyway, and the cost of adding it seems low.

So if you're not horrified about this API addition, I'll commit as-is.

[commit-bot: I haz the power, 2012-12-06 00:08:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/11316261/1011

[commit-bot: I haz the power, 2012-12-06 05:01:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/11316261/1011

[commit-bot: I haz the power, 2012-12-06 07:41:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jln@chromium.org/11316261/1011

[commit-bot: I haz the power, 2012-12-06 08:31:54]

Change committed as 171450

[agl, 2013-09-16 22:56:07]

I long ago lost track of the dozen different wrappings of waitpid I'm afraid.
Throwing to Mark, who will know the details of OS X that I don't.