Don't fork Zygote as a background process

Don't fork Zygote as a background process

On Linux, with the setuid sandbox, the Zygote would become a background
process of sort because the setuid sandbox would exit.

The problem is that the Chrome process tree would be broken because the
Zygote would be reparented to init.

In turn, this could create issues with the browser not being able to ptrace()
the Zygote if certain kernel restrictions are in place (e.g. Yama).

BUG=125225
TEST=
NOTRY=true


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=140104

[jln (very slow on Chromium), 2012-06-01 02:12:38]

Have the setuid sandbox wait() on Zygote to avoid re-parenting.

[agl, 2012-06-01 13:13:57]

LGTM.

You should also check that, with this patch running and the sandbox active (i.e.
check about:sandbox), that the "Stats for Nerds" page in the Shift-Esc dialog
can correct identify the additional sandbox limbo process.

I also suspect that several magic numbers in some tests will have to be updated
to account for the extra process, but tryserver will tell you about that.

https://chromiumcodereview.appspot.com/10447135/diff/4001/sandbox/linux/suid/...
File sandbox/linux/suid/sandbox.c (right):

https://chromiumcodereview.appspot.com/10447135/diff/4001/sandbox/linux/suid/...
sandbox/linux/suid/sandbox.c:242: if (DropRoot()) {
Preferably exceptional conditions should be handled first, i.e.

if (!DropRoot()) {
  FatalError("Could not drop privileges")
}

and then the mainline code.

https://chromiumcodereview.appspot.com/10447135/diff/4001/sandbox/linux/suid/...
sandbox/linux/suid/sandbox.c:250: if (send(sync_fds[1], "C", 1, MSG_NOSIGNAL) !=
1)
HANDLE_EINTR

https://chromiumcodereview.appspot.com/10447135/diff/4001/sandbox/linux/suid/...
sandbox/linux/suid/sandbox.c:271: if (read(sync_fds[0], &should_continue, 1) !=
1)
HANDLE_EINTR

[Jorge Lucangeli Obes, 2012-06-01 15:08:07]

lgtm with a couple of nits in comments and error messages.

Besides using the trybots for compile issues, are there any publicly accessible
trybots already running with the setuid sandbox? Is the sandbox binary compiled
every time or does it have to be copied?

https://chromiumcodereview.appspot.com/10447135/diff/4001/content/common/zygo...
File content/common/zygote_commands_linux.h (right):

https://chromiumcodereview.appspot.com/10447135/diff/4001/content/common/zygo...
content/common/zygote_commands_linux.h:23: // This number must be kept in sync
in sandbox/linux/suid/sandbox.c
"in" -> "with"

https://chromiumcodereview.appspot.com/10447135/diff/4001/sandbox/linux/suid/...
File sandbox/linux/suid/sandbox.c (right):

https://chromiumcodereview.appspot.com/10447135/diff/4001/sandbox/linux/suid/...
sandbox/linux/suid/sandbox.c:45: // This number must be kept in sync in with
common/zygote_commands_linux.h
"in with" -> "with"

https://chromiumcodereview.appspot.com/10447135/diff/4001/sandbox/linux/suid/...
sandbox/linux/suid/sandbox.c:266: FatalError("close ; shutdown");
The "close() || shutdown()" comments are not terribly consistent (one says
"Could not close..." and the other says "close; shutdown"

[jln (very slow on Chromium), 2012-06-01 19:11:01]

On 2012/06/01 13:13:57, agl wrote:
> LGTM.
> 
> You should also check that, with this patch running and the sandbox active
(i.e.
> check about:sandbox), that the "Stats for Nerds" page in the Shift-Esc dialog
> can correct identify the additional sandbox limbo process.

A few revisions ago the setuid sandbox was forking a new init process
(privileged). I moved this to the Zygote, and neither are known by about:memory.

I opened https://code.google.com/p/chromium/issues/detail?id=130786 to take a
look.

> I also suspect that several magic numbers in some tests will have to be
updated
> to account for the extra process, but tryserver will tell you about that.

Unfortunately the try bots don't update the setuid sandbox. It's very new (last
week) that they even run with the setuid sandbox.

I'll ping the infrastructure guys to update it, but I want to add a --api= flag
first to enforce particular setuid sandbox versions.

[commit-bot: I haz the power, 2012-06-01 20:00:33]

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

[commit-bot: I haz the power, 2012-06-01 20:00:38]

Presubmit check for 10447135-11001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/common

[jln (very slow on Chromium), 2012-06-01 20:07:29]

John, would you mind approving the new one-line comment in /content ?

Thanks,

Julien

[Lei Zhang, 2012-06-01 20:53:54]

On 2012/06/01 19:11:01, Julien Tinnes wrote:
> On 2012/06/01 13:13:57, agl wrote:
> > LGTM.
> > 
> > You should also check that, with this patch running and the sandbox active
> (i.e.
> > check about:sandbox), that the "Stats for Nerds" page in the Shift-Esc
dialog
> > can correct identify the additional sandbox limbo process.
> 
> A few revisions ago the setuid sandbox was forking a new init process
> (privileged). I moved this to the Zygote, and neither are known by
about:memory.
> 
> I opened https://code.google.com/p/chromium/issues/detail?id=130786 to take a
> look.
> 
> > I also suspect that several magic numbers in some tests will have to be
> updated
> > to account for the extra process, but tryserver will tell you about that.
> 
> Unfortunately the try bots don't update the setuid sandbox. It's very new
(last
> week) that they even run with the setuid sandbox.
> 
> I'll ping the infrastructure guys to update it, but I want to add a --api=
flag
> first to enforce particular setuid sandbox versions.

LGTM++

Running with the patched sandbox locally, I have the opposite problem - I see
two entries for the same zygote. I will append to the bug.

[Avi (use Gerrit), 2012-06-01 20:56:58]

lgtm

content stamp

[commit-bot: I haz the power, 2012-06-01 21:40:43]

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

[commit-bot: I haz the power, 2012-06-01 21:41:45]

Change committed as 140104