Simplify ZygoteForkDelegate API further

Simplify ZygoteForkDelegate API further

This patch makes three changes:

  1. Removes the AckChild() delegate method used to send a custom
     message to the child process.
  2. Instead, the parent always writes the child's PID (as seen by the
     browser) over the pipe.  (Exception: When writing to a NaCl child
     process, we instead send 0 to avoid leaking the real PID into the
     NaCl address space.)
  3. Makes the Fork() delegate method responsible for sending the IPC
     channel ID to the child process.

This is in preparation for the next patch which will switch the pipe
direction to make the child responsible for discovering its own PID
and sending it to the parent process.  By removing AckChild(), this
simplifies the protocol and makes this change easier to implement.

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

[jln (very slow on Chromium), 2014-04-17 17:53:03]

lgtm, thanks for the cleanup!

[mdempsky, 2014-04-17 18:01:42]

piman: Please review the small change in
content/public/common/zygote_fork_delegate_linux.h for OWNERS approval.

mseaborn: Please review the changes in components/nacl for OWNERS approval.

Thanks!

[Mark Seaborn, 2014-04-17 18:32:11]

https://codereview.chromium.org/240673002/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/240673002/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:127: HANDLE_EINTR(read(parent_fd,
&real_pid, sizeof(real_pid)));
So you're sending the real PID to the NaCl process even though it doesn't use
it?

It would be nice to hide the PID from Non-SFI NaCl processes, because the PID
leaks some information about system state.  We talked about this a bit here:
https://docs.google.com/a/chromium.org/document/d/1wL5SCBdbXFGpqNkmzxN8o2w2KP...

It would actually be better not to send the PID.

Alternatively, you could zero real_pid here after receiving it (with a comment).
 But this might need to be a volatile write to be effective at the end of a
function.

BTW, what does the real PID get used for in non-NaCl subprocesses?

https://codereview.chromium.org/240673002/diff/1/content/zygote/zygote_linux.cc
File content/zygote/zygote_linux.cc (right):

https://codereview.chromium.org/240673002/diff/1/content/zygote/zygote_linux....
content/zygote/zygote_linux.cc:413: ssize_t written =
Maybe we could zero out real_pid if use_helper is true?

[jln (very slow on Chromium), 2014-04-17 18:40:05]

https://codereview.chromium.org/240673002/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/240673002/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:127: HANDLE_EINTR(read(parent_fd,
&real_pid, sizeof(real_pid)));
On 2014/04/17 18:32:12, Mark Seaborn wrote:
> So you're sending the real PID to the NaCl process even though it doesn't use
> it?
> 
> It would be nice to hide the PID from Non-SFI NaCl processes, because the PID
> leaks some information about system state.  We talked about this a bit here:
>
https://docs.google.com/a/chromium.org/document/d/1wL5SCBdbXFGpqNkmzxN8o2w2KP...
> 
> It would actually be better not to send the PID.
> 
> Alternatively, you could zero real_pid here after receiving it (with a
comment).
>  But this might need to be a volatile write to be effective at the end of a
> function.
> 
> BTW, what does the real PID get used for in non-NaCl subprocesses?

We do use the real pid to reap processes no?

The NaCl helper, the Zygote and the browser reference pids through their
(browser-level) global PID.

Unfortunately, with 3 namespaces, it's the only common ground. We could (in
another change), make sure that this "common ground" has nothing to do with the
real pid on the system, but is it worth it?

[jln (very slow on Chromium), 2014-04-17 18:42:32]

https://codereview.chromium.org/240673002/diff/1/components/nacl/loader/nacl_...
File components/nacl/loader/nacl_helper_linux.cc (right):

https://codereview.chromium.org/240673002/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:127: HANDLE_EINTR(read(parent_fd,
&real_pid, sizeof(real_pid)));
On 2014/04/17 18:40:06, jln wrote:
> On 2014/04/17 18:32:12, Mark Seaborn wrote:
> > So you're sending the real PID to the NaCl process even though it doesn't
use
> > it?
> > 
> > It would be nice to hide the PID from Non-SFI NaCl processes, because the
PID
> > leaks some information about system state.  We talked about this a bit here:
> >
>
https://docs.google.com/a/chromium.org/document/d/1wL5SCBdbXFGpqNkmzxN8o2w2KP...
> > 
> > It would actually be better not to send the PID.
> > 
> > Alternatively, you could zero real_pid here after receiving it (with a
> comment).
> >  But this might need to be a volatile write to be effective at the end of a
> > function.
> > 
> > BTW, what does the real PID get used for in non-NaCl subprocesses?
> 
> We do use the real pid to reap processes no?
> 
> The NaCl helper, the Zygote and the browser reference pids through their
> (browser-level) global PID.
> 
> Unfortunately, with 3 namespaces, it's the only common ground. We could (in
> another change), make sure that this "common ground" has nothing to do with
the
> real pid on the system, but is it worth it?

(err, disregard, you're talking about ChildNaClLoaderInit, which runs in the
child).

[mdempsky, 2014-04-17 19:31:53]

mseaborn: PTAL.  I've revised the patch as we discussed on IRC to avoid leaking
the NaCl child process's real PID into its address space, and I'll update my
followup patch similarly.

[Mark Seaborn, 2014-04-17 19:42:56]

LGTM, thanks

[piman, 2014-04-17 23:35:53]

lgtm

[mdempsky, 2014-04-18 00:07:58]

The CQ bit was checked by mdempsky@chromium.org

[commit-bot: I haz the power, 2014-04-18 00:09:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mdempsky@chromium.org/240673002/20001

[commit-bot: I haz the power, 2014-04-18 10:25:36]

Change committed as 264764