Calling clone(CLONE_NEWPID) results in the new pid namespace getting a new "init" process.

Calling clone(CLONE_NEWPID) results in the new pid namespace getting a new "init" process.
This process is now resposible for reaping all child processes that no longer have a
direct parent process.

Often, failure to do this goes unnoticed, because our sandbox'd processes don't often
fork other processes that then continue to turn into daemon processes. But there is no
reason, why they couldn't occasionally do so. And in fact, the seccomp sandbox does do
so for its trusted process.

In the past, this would result in us having lots of uncollected zombie processes that
only disappeared when the browser terminated.

BUG=109944
TEST=Run Chrome with both the suid sandbox and the seccomp sandbox, open and close a few tabs, verify that we don't produce any zombie processes
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=119746

[Markus (顧孟勤), 2012-01-27 02:51:15]

It looks as if we were using CLONE_NEWPID differently from how it was intended
to be used. In particular, we didn't have a dedicated "init" process.

This changelist fixes that.

Additionally, we are also supposed to mount our own private copy of "/proc". I
don't think, we need to do that. And that's probably a good thing, as adding
mount points creates all sorts of other problems with correct clean up.

This is a somewhat big change to rather subtle and security critical code.
Please review carefully. I am particularly concerned about interactions with
other parts of the system (e.g. breakpad). I think, we do the right thing here,
but an extra pair of eyes would help.

[agl, 2012-01-27 15:14:32]

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/init...
File sandbox/linux/suid/init_process.c (right):

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/init...
sandbox/linux/suid/init_process.c:143: sa.sa_flags = SA_RESTART | SA_SIGINFO;
you setup a sigaction struct, but I don't see where you do anything with it. The
manpage for sigwaitinfo also says that a typical program doesn't establish
handlers for the signal.

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/init...
sandbox/linux/suid/init_process.c:168: retry = false;
If you're assuming that waitpid will already return one or more children after
SIGCHLD, then retry is always false coming out of the for loop and this is
superfluous.

If you're not assuming that then the child will be a zombie (because waidpid
didn't catch it) and hasChildren will find that zombie, set retry to false and
we'll sigwaitinfo for a SIGCHLD that never comes.

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
File sandbox/linux/suid/sandbox.c (right):

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:205: fprintf(stderr, "Failed to snprintf\n");
I note that we fail to close sv[1] in this path.

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:219: fprintf(stderr, "Failed to snprintf\n");
... and this one.

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:238: close(fd);
this close isn't reflecting in the other error paths. (Maybe we don't care.)

[Markus (顧孟勤), 2012-01-27 17:44:09]

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/init...
File sandbox/linux/suid/init_process.c (right):

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/init...
sandbox/linux/suid/init_process.c:143: sa.sa_flags = SA_RESTART | SA_SIGINFO;
Good call. There was supposed to be call to sigaction() here. But even more
interestingly, I apparently don't need this code at all.

I originally used sigsuspend() instead of sigwaitinfo(). And it appears that
these two functions are subtly different. The former still needs the signal
handler, the latter doesn't.

I just removed all of the signal handling. That seems the correct solution.

On 2012/01/27 15:14:32, agl wrote:
> you setup a sigaction struct, but I don't see where you do anything with it.
The
> manpage for sigwaitinfo also says that a typical program doesn't establish
> handlers for the signal.

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/init...
sandbox/linux/suid/init_process.c:168: retry = false;
This code is subtle. If you can think of a better way to document it, please let
me know.

I am worried that we erroneously declare the namespace empty, while it still has
members. In particular, I am worried that while we are scanning "/proc", a
process might die in just the right way that we never notice it as one of our
child processes.

This is particularly tricky, because when scanning "/proc", we only see our
immediate children.

I am not 100% convinced this race condition is actually possible, but I don't
want to reason about how atomically the kernel updates "/proc" on a multi-CPU
machine.

I can convince myself though that it is safe to declare the namespace as empty,
if we were unable to find any immediate children *and* if we did not see any new
processes die while scanning for children.

So, this is what the "retry" logic does. When it cannot find any immediate
children, it once again calls waitpid(). In all likelihood, waitpid() will
return with "-1" because there really aren't any children left. We scan "/proc"
one more time to be absolutely sure and then we exit.

Unless we encounter the race condition that we are protecting from, the "retry"
logic only ever triggers exactly once right before the "init" process
terminates. So, the extra cost of a second iteration should be negligible.


On 2012/01/27 15:14:32, agl wrote:
> If you're assuming that waitpid will already return one or more children after
> SIGCHLD, then retry is always false coming out of the for loop and this is
> superfluous.
> 
> If you're not assuming that then the child will be a zombie (because waidpid
> didn't catch it) and hasChildren will find that zombie, set retry to false and
> we'll sigwaitinfo for a SIGCHLD that never comes.

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
File sandbox/linux/suid/sandbox.c (right):

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:205: fprintf(stderr, "Failed to snprintf\n");
On 2012/01/27 15:14:32, agl wrote:
> I note that we fail to close sv[1] in this path.

Done

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:219: fprintf(stderr, "Failed to snprintf\n");
On 2012/01/27 15:14:32, agl wrote:
> ... and this one.

Done

https://chromiumcodereview.appspot.com/9295005/diff/1/sandbox/linux/suid/sand...
sandbox/linux/suid/sandbox.c:238: close(fd);
On 2012/01/27 15:14:32, agl wrote:
> this close isn't reflecting in the other error paths. (Maybe we don't care.)

I don't know what happened. Sorry about that. That was a genuine (albeit
probably benign) bug.

[agl, 2012-01-30 22:10:57]

lgtm