Add support for getting the real process id from within the suid sandbox. The...

Add support for getting the real process id from within the suid sandbox. The browser processes gets the real process ids, so they look correct in the task manager. When it asks the zygote to reap a process, we use the process ids internal to the sandbox.

While we are at it, reap the sandbox process after it clones the zygote and figure out zygote's actual process id. Save the actual process id rather than that of the sandbox.

BUG=20012, 20714, 23072
TEST=Process IDs for renderers should be correct in the task manager and you should be able to use the end process button to kill them.

[Lei Zhang, 2009-10-08 09:24:15]

On some machines, in the suid sandbox, fork() returns pids like 3 and 9. I added
more plumbing so we can get the real pids.

[agl, 2009-10-08 17:47:29]

This isn't some random kernel bug: the SUID sandbox will install the zygote into
a new PID namespace if possible. This stops renderer processes from kill()ing
and ptrace()ing other processes on the system. (You should update the comment to
reflect that).

This isn't where I would have put the logic: one could also put the code in
browser_render_process_host and keep two PIDs: one for the browser's view and
one for the zygote's view. However, since we are going to start pushing for more
processes running via the zygote, maybe it's better this way.

However, the 'retry a few times' loop is obviously a no go. Please try disabling
the code in base/setproctitle_linux.c and see it removes the flakeyness. If so,
we'll have to nuke that code.

Also note that this changes the behaviour of a sandbox host crash. Previously,
renderers would freeze, now the browser will freeze too. Something to keep in
mind when triaging bugs.

(Once again, it would be very nice if UNIX had a process handle type. Sadly,
UNIX domain sockets are broken and cannot be used to translate PIDs either.)

http://codereview.chromium.org/262020/diff/1/3
File base/process_util_linux.cc (right):

http://codereview.chromium.org/262020/diff/1/3#newcode98
Line 98: ProcessId GetProcessWithChannelID(const std::wstring& executable_name,
This doesn't belong in base: it's very Chrome specific. Can you move it into
sandbox_host_linux.cc?

http://codereview.chromium.org/262020/diff/1/5
File chrome/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/262020/diff/1/5#newcode249
Line 249: // There's some race while reading /proc/pid/cmdline where the
Holy crap :(

This is probably caused by mdm's argv shenanigans that he committed against my
objections. Try taking it out (base/setproctitle_linux.c) and see if the
flakeyness goes away.

http://codereview.chromium.org/262020/diff/1/5#newcode278
Line 278: // fork() returns the 'wrong' process ids in the sandbox. Thus we need
Update comment.

[Lei Zhang, 2009-10-08 22:34:50]

http://codereview.chromium.org/262020/diff/1/3
File base/process_util_linux.cc (right):

http://codereview.chromium.org/262020/diff/1/3#newcode98
Line 98: ProcessId GetProcessWithChannelID(const std::wstring& executable_name,
On 2009/10/08 17:47:30, agl wrote:
> This doesn't belong in base: it's very Chrome specific. Can you move it into
> sandbox_host_linux.cc?

No longer needed in patch set 2.

http://codereview.chromium.org/262020/diff/1/5
File chrome/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/262020/diff/1/5#newcode249
Line 249: // There's some race while reading /proc/pid/cmdline where the
On 2009/10/08 17:47:30, agl wrote:
> Holy crap :(
> 
> This is probably caused by mdm's argv shenanigans that he committed against my
> objections. Try taking it out (base/setproctitle_linux.c) and see if the
> flakeyness goes away.

Actually, without setproctitle(), I cannot identify which process has the
channel id, since they're all just of --type=zygotes. The race happens right
after the fork, where th child may run before the parent had a chance to call
setproctitle().

This code has been scrapped. Instead, it asks the sandbox host for the zygote's
children's pids. Then it goes through the list and find the first one that is
not in the internal list.

http://codereview.chromium.org/262020/diff/1/5#newcode278
Line 278: // fork() returns the 'wrong' process ids in the sandbox. Thus we need
On 2009/10/08 17:47:30, agl wrote:
> Update comment.

Done.

[agl, 2009-10-08 23:53:19]

I think it would be better to enumerate the children of the current process.
That still leaves some corner cases (like utility processing forking), but it's
better.

LGTM, because it's better than what we have now.

http://codereview.chromium.org/262020/diff/1004/2003
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/262020/diff/1004/2003#newcode264
Line 264: base::NamedProcessIterator proc_iter(L"chrome", NULL);
What happens when we're called chromium, not chrome? What happens when there are
two chrome processes on the system, each creating new children? What about when
we fork off utility processes etc? (In the long term they should be forking via
the zygote, but that's not the case right now.)

[Lei Zhang, 2009-10-09 00:03:36]

http://codereview.chromium.org/262020/diff/1004/2003
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/262020/diff/1004/2003#newcode264
Line 264: base::NamedProcessIterator proc_iter(L"chrome", NULL);
On 2009/10/08 23:53:19, agl wrote:
> What happens when we're called chromium, not chrome? What happens when there
are
> two chrome processes on the system, each creating new children? What about
when
> we fork off utility processes etc? (In the long term they should be forking
via
> the zygote, but that's not the case right now.)

Not sure about utility processes, I need to test that. I will also find where
the executable name is defined and use that rather than hard code "chrome".

We handle the case of two users both running chrome, because their zygotes have
different pids, which they pass in as |parent_id| here.

[agl, 2009-10-09 00:11:21]

http://codereview.chromium.org/262020/diff/1004/2003
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/262020/diff/1004/2003#newcode264
Line 264: base::NamedProcessIterator proc_iter(L"chrome", NULL);
On 2009/10/09 00:03:36, Lei Zhang wrote:
> We handle the case of two users both running chrome, because their zygotes
have
> different pids, which they pass in as |parent_id| here.

Sorry, I didn't notice that. I believe that all zygotes will report the same PID
(2 or 3). And, from the point of view of the parent, it won't find any children
of that process. It should look for children of the browser process I believe.

[Lei Zhang, 2009-10-09 18:56:36]

On 2009/10/09 00:11:21, agl wrote:
> http://codereview.chromium.org/262020/diff/1004/2003
> File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):
> 
> http://codereview.chromium.org/262020/diff/1004/2003#newcode264
> Line 264: base::NamedProcessIterator proc_iter(L"chrome", NULL);
> On 2009/10/09 00:03:36, Lei Zhang wrote:
> > We handle the case of two users both running chrome, because their zygotes
> have
> > different pids, which they pass in as |parent_id| here.
> 
> Sorry, I didn't notice that. I believe that all zygotes will report the same
PID
> (2 or 3). And, from the point of view of the parent, it won't find any
children
> of that process. It should look for children of the browser process I believe.

The parent of the renderers is the zygote. When we're using the suid sandbox,
the browser runs the sandbox, which then forks off the zygote. The sandbox then
exits, but doesn't get reaped, thus we get bug 23072.

The process id for the sandbox gets returned to the browser in
zygote_host_linux.cc, but this is the wrong process id. The actual zygote
process has a different pid, but it's hard to figure out because the browser
process doesn't get it back, and the zygote doesn't know its own pids since it's
sandboxed.

My solution is to launch the zygote with a unique id, and then look for it in
/proc. To prevent a race condition from happening here like it did with the
renderers before, the browser waits for the zygote to send it a magic string.

Once we know the zygote's id, the zygote can ask the sandbox host for a list of
its own children, and figure out what the real pid of the newly forked renderer
is.


FYI, this patch will fail the try job because it depends on a small CL:
http://codereview.chromium.org/271031/show.

[Lei Zhang, 2009-10-21 08:11:53]

Ok, finally got around to rewriting this to identify processes by searching for
a socket with a given inode number.

This depends on a few other CLs that also need to be reviewed:
http://codereview.chromium.org/312002/show
http://codereview.chromium.org/312003/show
http://codereview.chromium.org/297011/show

[agl, 2009-10-23 18:15:15]

LGTM with trepidation.

This is probably the best design, but that doesn't mean that I don't want to cry
a little inside :(

http://codereview.chromium.org/262020/diff/9006/9010
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/262020/diff/9006/9010#newcode329
Line 329: RenderSandboxHostLinux::RenderSandboxHostLinux() : init_(false) {
Style: new line and indent four spaces before the ':'

http://codereview.chromium.org/262020/diff/9006/9009
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/262020/diff/9006/9009#newcode50
Line 50: ZygoteHost::ZygoteHost() : pid_(-1),
Style: new line and indent four spaces before the colon.

http://codereview.chromium.org/262020/diff/9006/9009#newcode126
Line 126: dummy_fd = socket(PF_UNIX, SOCK_SEQPACKET, 0);
If this is just a dummy fd, might as well make it a DGRAM. BSD doesn't have
SEQPACKET.

http://codereview.chromium.org/262020/diff/9006/9009#newcode127
Line 127: CHECK(dummy_fd != 0);
Zero is a valid file descriptor. DCHECK_GE(dummy_fd, 0);

http://codereview.chromium.org/262020/diff/9006/9009#newcode148
Line 148: uint64_t inode = 0;
ino_t

http://codereview.chromium.org/262020/diff/9006/9008
File chrome/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/262020/diff/9006/9008#newcode75
Line 75: CHECK(r) << "Sending zygote magic failed";
CHECK_EQ(r, sizeof(kZygoteMagic))

http://codereview.chromium.org/262020/diff/9006/9008#newcode222
Line 222: dummy_fd = socket(PF_UNIX, SOCK_SEQPACKET, 0);
Again, make it a DGRAM

[Lei Zhang, 2009-10-29 21:23:41]

Still need a bit more work. Currently, you can disable the sandbox by just
removing the sandbox binary, and Chrome continues to work. With this patch,
that's no longer the case.

http://codereview.chromium.org/262020/diff/9006/9010
File chrome/browser/renderer_host/render_sandbox_host_linux.cc (right):

http://codereview.chromium.org/262020/diff/9006/9010#newcode329
Line 329: RenderSandboxHostLinux::RenderSandboxHostLinux() : init_(false) {
On 2009/10/23 18:15:15, agl wrote:
> Style: new line and indent four spaces before the ':'

Done.

http://codereview.chromium.org/262020/diff/9006/9009
File chrome/browser/zygote_host_linux.cc (right):

http://codereview.chromium.org/262020/diff/9006/9009#newcode50
Line 50: ZygoteHost::ZygoteHost() : pid_(-1),
On 2009/10/23 18:15:15, agl wrote:
> Style: new line and indent four spaces before the colon.

Done.

http://codereview.chromium.org/262020/diff/9006/9009#newcode126
Line 126: dummy_fd = socket(PF_UNIX, SOCK_SEQPACKET, 0);
On 2009/10/23 18:15:15, agl wrote:
> If this is just a dummy fd, might as well make it a DGRAM. BSD doesn't have
> SEQPACKET.

Done.

http://codereview.chromium.org/262020/diff/9006/9009#newcode127
Line 127: CHECK(dummy_fd != 0);
On 2009/10/23 18:15:15, agl wrote:
> Zero is a valid file descriptor. DCHECK_GE(dummy_fd, 0);

Done, kept this as a CHECK().

http://codereview.chromium.org/262020/diff/9006/9009#newcode148
Line 148: uint64_t inode = 0;
On 2009/10/23 18:15:15, agl wrote:
> ino_t

Done.

http://codereview.chromium.org/262020/diff/9006/9008
File chrome/browser/zygote_main_linux.cc (right):

http://codereview.chromium.org/262020/diff/9006/9008#newcode75
Line 75: CHECK(r) << "Sending zygote magic failed";
On 2009/10/23 18:15:15, agl wrote:
> CHECK_EQ(r, sizeof(kZygoteMagic))

SendMsg() returns a bool.
There is no CHECK_EQ() BTW.

http://codereview.chromium.org/262020/diff/9006/9008#newcode222
Line 222: dummy_fd = socket(PF_UNIX, SOCK_SEQPACKET, 0);
On 2009/10/23 18:15:15, agl wrote:
> Again, make it a DGRAM

Done.

[Lei Zhang, 2009-10-29 22:40:57]

Ok, fixed the stupid error.

[agl, 2009-10-30 00:06:57]

LGTM