NaCl: Clean up how FDs are passed to nacl_helper instances on Linux

NaCl: Clean up how FDs are passed to nacl_helper instances on Linux

child_process_launcher.cc constructs a mapping from FD numbers to FDs,
and normal Chromium child processes receive this as a mapping.
However, before this change, when zygote_linux.cc passed these FDs to
nacl_helper, it stripped the keys from the mapping and only passed the
values.

This meant that if child_process_launcher.cc were changed to add more
key+value pairs to the FD mapping, nacl_helper_linux.cc would need to
be updated to use or skip over them.  Also, the order in which
child_process_launcher.cc adds the FDs to the mapping would be
significant, but only to nacl_helper, which would be weird.  This
would be particularly awkward for FDs that are added to the mapping
conditionally.

We clean this up by explicitly taking the one FD that nacl_helper
needs from the FD mapping, rather than passing across all of the
mapping's values.

The aim of this cleanup is to simplify this change --
https://codereview.chromium.org/22911027/ -- which changes
child_process_launcher.cc to add an FD conditionally.

Also make the #defines of the FD numbers clearer by moving them to
zygote_fork_delegate_linux.h, since that's the component that's
responsible for them.

BUG=none
TEST=NaCl tests in browser_tests

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

[jln (very slow on Chromium), 2013-09-30 19:15:39]

(Sorry for the long review delay)

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
File content/zygote/zygote_linux.cc (right):

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
content/zygote/zygote_linux.cc:320: fds.push_back(ipc_channel_fd);
I don't understand why this works. The size of fds is checked against a static
value in NaClForkDelegate::Fork() and we're increasing it here.

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
content/zygote/zygote_linux.cc:465: 
Do you want to add a comment here explaining a bit what this is?

[Mark Seaborn, 2013-10-01 22:32:53]

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
File content/zygote/zygote_linux.cc (right):

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
content/zygote/zygote_linux.cc:320: fds.push_back(ipc_channel_fd);
On 2013/09/30 19:15:39, jln wrote:
> I don't understand why this works. The size of fds is checked against a
> static value in NaClForkDelegate::Fork() and we're increasing it here.

I'm not increasing the number of FDs that is sent to nacl_helper in this change.
 It's 3 FDs before and after the change.

Before the change, 'fds' contains 1 FD when passed to Zygote::ForkWithRealPid(),
which adds 2 more.  After the change, Zygote::ForkWithRealPid() creates its own
'fds' vector and adds 3 FDs to it.

Do you think I should comment this as follows?

  fds.push_back(ipc_channel_fd);  // kNaClBrowserFDIndex
  fds.push_back(dummy_fd);        // kNaClDummyFDIndex
  fds.push_back(pipe_fds[0]);     // kNaClParentFDIndex

The problem with that is that this code path is nominally independent of NaCl
(even though it exists only for NaCl), and these comments "depend" on the
concept from nacl_helper_linux.h (by referring to it).

Maybe I should move kNaClBrowserFDIndex etc. to be defined in
content/public/common/zygote_fork_delegate_linux.h, and remove the "NaCl"
prefix?

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
content/zygote/zygote_linux.cc:465: 
On 2013/09/30 19:15:39, jln wrote:
> Do you want to add a comment here explaining a bit what this is?

Something like "// Unmarshal FD mapping"?  This part seems fairly
self-explanatory, and I'm not changing it in this change, so not sure what you
want here.

[jln (very slow on Chromium), 2013-10-01 22:51:46]

lgtm

Feel free to add more comments / interface documentation in general, as it's
generally lacking in this area.

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
File content/zygote/zygote_linux.cc (right):

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
content/zygote/zygote_linux.cc:320: fds.push_back(ipc_channel_fd);
On 2013/10/01 22:32:54, Mark Seaborn wrote:
> On 2013/09/30 19:15:39, jln wrote:
> > I don't understand why this works. The size of fds is checked against a
> > static value in NaClForkDelegate::Fork() and we're increasing it here.
> 
> I'm not increasing the number of FDs that is sent to nacl_helper in this
change.
>  It's 3 FDs before and after the change.

Ohh yeah of course, sorry I missed that.

> Before the change, 'fds' contains 1 FD when passed to
Zygote::ForkWithRealPid(),
> which adds 2 more.  After the change, Zygote::ForkWithRealPid() creates its
own
> 'fds' vector and adds 3 FDs to it.
> 
> Maybe I should move kNaClBrowserFDIndex etc. to be defined in
> content/public/common/zygote_fork_delegate_linux.h, and remove the "NaCl"
> prefix?

That could be a good additional change yeah. Feel free to add to this CL, or to
another one.
In general this is a very dark area, so cleanups and documentations are very
welcome!

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
content/zygote/zygote_linux.cc:465: 
On 2013/10/01 22:32:54, Mark Seaborn wrote:
> On 2013/09/30 19:15:39, jln wrote:
> > Do you want to add a comment here explaining a bit what this is?
> 
> Something like "// Unmarshal FD mapping"?  This part seems fairly
> self-explanatory, and I'm not changing it in this change, so not sure what you
> want here.

The whole thing is generally very poorly documented. But you're right that the
code is obvious, |mapping| could be documented in zygote_linux.h ?

Your call, I didn't deal with this code in a while and I don't remember well
where the documentation is the most lacking.

[Mark Seaborn, 2013-10-07 15:23:13]

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
File content/zygote/zygote_linux.cc (right):

https://chromiumcodereview.appspot.com/24449002/diff/1/content/zygote/zygote_...
content/zygote/zygote_linux.cc:320: fds.push_back(ipc_channel_fd);
On 2013/10/01 22:51:46, jln wrote:
> > Maybe I should move kNaClBrowserFDIndex etc. to be defined in
> > content/public/common/zygote_fork_delegate_linux.h, and remove the "NaCl"
> > prefix?
> 
> That could be a good additional change yeah. Feel free to add to this CL,
> or to another one.
> In general this is a very dark area, so cleanups and documentations are
> very welcome!

I've made that change.  Please take a look.

[jln (very slow on Chromium), 2013-10-07 18:12:06]

https://codereview.chromium.org/24449002/diff/12001/chrome/nacl/nacl_helper_l...
File chrome/nacl/nacl_helper_linux.cc (right):

https://codereview.chromium.org/24449002/diff/12001/chrome/nacl/nacl_helper_l...
chrome/nacl/nacl_helper_linux.cc:75: int parent_fd =
child_fds[content::ZygoteForkDelegate::kParentFDIndex];
Do you want to make these const?

https://codereview.chromium.org/24449002/diff/12001/chrome/nacl/nacl_helper_l...
chrome/nacl/nacl_helper_linux.cc:119: if
(content::ZygoteForkDelegate::kParentFDIndex + 1 != child_fds.size()) {
kNumPassedFds ? (see comment at the enum declaration)

https://codereview.chromium.org/24449002/diff/12001/components/nacl/zygote/na...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/24449002/diff/12001/components/nacl/zygote/na...
components/nacl/zygote/nacl_fork_delegate_linux.cc:261: DCHECK(fds.size() ==
kParentFDIndex + 1);
kNumPassedFds?

https://codereview.chromium.org/24449002/diff/12001/content/public/common/zyg...
File content/public/common/zygote_fork_delegate_linux.h (right):

https://codereview.chromium.org/24449002/diff/12001/content/public/common/zyg...
content/public/common/zygote_fork_delegate_linux.h:54: kParentFDIndex = 2
With MACRO_STYLE, in Chromium, it's typical to have a *_END member at the end to
be used as a count. In this case it could be kNumPassedFds or something similar.

(Also FYI, strictly speaking the Chromium style guide says to continue using
MACRO_STYLE for enums. In this case the enum is merely a way to define constants
and never used as a type, so I think your way is better).

[Mark Seaborn, 2013-10-08 18:31:06]

https://codereview.chromium.org/24449002/diff/12001/chrome/nacl/nacl_helper_l...
File chrome/nacl/nacl_helper_linux.cc (right):

https://codereview.chromium.org/24449002/diff/12001/chrome/nacl/nacl_helper_l...
chrome/nacl/nacl_helper_linux.cc:75: int parent_fd =
child_fds[content::ZygoteForkDelegate::kParentFDIndex];
On 2013/10/07 18:12:06, jln wrote:
> Do you want to make these const?

I don't normally do that for local variables, but it doesn't hurt, so OK.

https://codereview.chromium.org/24449002/diff/12001/chrome/nacl/nacl_helper_l...
chrome/nacl/nacl_helper_linux.cc:119: if
(content::ZygoteForkDelegate::kParentFDIndex + 1 != child_fds.size()) {
On 2013/10/07 18:12:06, jln wrote:
> kNumPassedFds ? (see comment at the enum declaration)

Done.

https://codereview.chromium.org/24449002/diff/12001/components/nacl/zygote/na...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/24449002/diff/12001/components/nacl/zygote/na...
components/nacl/zygote/nacl_fork_delegate_linux.cc:261: DCHECK(fds.size() ==
kParentFDIndex + 1);
On 2013/10/07 18:12:06, jln wrote:
> kNumPassedFds?

Done.

https://codereview.chromium.org/24449002/diff/12001/content/public/common/zyg...
File content/public/common/zygote_fork_delegate_linux.h (right):

https://codereview.chromium.org/24449002/diff/12001/content/public/common/zyg...
content/public/common/zygote_fork_delegate_linux.h:54: kParentFDIndex = 2
On 2013/10/07 18:12:06, jln wrote:
> With MACRO_STYLE, in Chromium, it's typical to have a *_END member at the end
to
> be used as a count. In this case it could be kNumPassedFds or something
similar.

Added kNumPassedFds.

> (Also FYI, strictly speaking the Chromium style guide says to continue using
> MACRO_STYLE for enums. In this case the enum is merely a way to define
constants
> and never used as a type, so I think your way is better).

OK, I've left it as the "k*" style.

[jln (very slow on Chromium), 2013-10-08 19:33:36]

Tiny, non mandatory nit.

https://codereview.chromium.org/24449002/diff/40001/content/public/common/zyg...
File content/public/common/zygote_fork_delegate_linux.h (right):

https://codereview.chromium.org/24449002/diff/40001/content/public/common/zyg...
content/public/common/zygote_fork_delegate_linux.h:55: kNumPassedFDs = 3
Small nit: I think the design is to not put the number explicitly here, so that
it stays "floating" and will always stay at the end (most of Chromium calls it
*_END).

You could even add a comment.

[Mark Seaborn, 2013-10-08 21:01:56]

https://codereview.chromium.org/24449002/diff/40001/content/public/common/zyg...
File content/public/common/zygote_fork_delegate_linux.h (right):

https://codereview.chromium.org/24449002/diff/40001/content/public/common/zyg...
content/public/common/zygote_fork_delegate_linux.h:55: kNumPassedFDs = 3
On 2013/10/08 19:33:36, jln wrote:
> Small nit: I think the design is to not put the number explicitly here, so
that
> it stays "floating" and will always stay at the end (most of Chromium calls it
> *_END).
> 
> You could even add a comment.

OK, I've removed the numbers and added a comment.

[Mark Seaborn, 2013-10-08 22:30:11]

+jam for OWNERS review of zygote_fork_delegate_linux.h.

[jam, 2013-10-10 04:54:54]

On 2013/10/08 22:30:11, Mark Seaborn wrote:
> +jam for OWNERS review of zygote_fork_delegate_linux.h.

I'm not really familiar with the zygote code, but I'm looking at this change and
wondering why chrome and components layers need to know about the internals of
what content is doing? can we hide that inside content, and make the enum's
values an implementation detail? i.e. why isn't the closing of file descriptors
of files that content opens happening inside content?

[jam, 2013-10-10 04:54:54]

On 2013/10/08 22:30:11, Mark Seaborn wrote:
> +jam for OWNERS review of zygote_fork_delegate_linux.h.

I'm not really familiar with the zygote code, but I'm looking at this change and
wondering why chrome and components layers need to know about the internals of
what content is doing? can we hide that inside content, and make the enum's
values an implementation detail? i.e. why isn't the closing of file descriptors
of files that content opens happening inside content?

[Mark Seaborn, 2013-10-10 16:56:18]

On 9 October 2013 21:54, <jam@chromium.org> wrote:

> On 2013/10/08 22:30:11, Mark Seaborn wrote:
>
>> +jam for OWNERS review of zygote_fork_delegate_linux.h.
>>
>
> I'm not really familiar with the zygote code, but I'm looking at this
> change and
> wondering why chrome and components layers need to know about the
> internals of
> what content is doing? can we hide that inside content, and make the enum's
> values an implementation detail? i.e. why isn't the closing of file
> descriptors
> of files that content opens happening inside content?
>

Are you referring to the close(dummy_fd) and close(parent_fd) calls in
nacl_helper_linux.cc?  These are passed across IPC so that the browser can
determine the new process's PID (to work around the Linux PID namespace
that the SUID sandbox uses).

The reason for the strangeness of this code is that nacl_helper_linux.cc
duplicated some of the zygote code when it was originally added.  The
zygote process does an IPC to the NaCl helper process to request spawning a
NaCl process.  It would be cleaner just to have two zygote processes (one
for NaCl and one for normal Chrome processes); I don't know why it wasn't
done like that originally.

Until we clean this up fully, is it OK if I commit this change?  It's an
incremental cleanup that I'm doing so that rmcilroy can commit his change (
https://codereview.chromium.org/22911027/) without having to contort
nacl_helper_linux.cc further.

Mark

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2013-10-11 16:13:32]

On 2013/10/10 16:56:18, Mark Seaborn wrote:
> On 9 October 2013 21:54, <mailto:jam@chromium.org> wrote:
> 
> > On 2013/10/08 22:30:11, Mark Seaborn wrote:
> >
> >> +jam for OWNERS review of zygote_fork_delegate_linux.h.
> >>
> >
> > I'm not really familiar with the zygote code, but I'm looking at this
> > change and
> > wondering why chrome and components layers need to know about the
> > internals of
> > what content is doing? can we hide that inside content, and make the enum's
> > values an implementation detail? i.e. why isn't the closing of file
> > descriptors
> > of files that content opens happening inside content?
> >
> 
> Are you referring to the close(dummy_fd) and close(parent_fd) calls in
> nacl_helper_linux.cc?  These are passed across IPC so that the browser can
> determine the new process's PID (to work around the Linux PID namespace
> that the SUID sandbox uses).
> 
> The reason for the strangeness of this code is that nacl_helper_linux.cc
> duplicated some of the zygote code when it was originally added.  The
> zygote process does an IPC to the NaCl helper process to request spawning a
> NaCl process.  It would be cleaner just to have two zygote processes (one
> for NaCl and one for normal Chrome processes); I don't know why it wasn't
> done like that originally.
> 
> Until we clean this up fully, is it OK if I commit this change?  It's an
> incremental cleanup that I'm doing so that rmcilroy can commit his change (
> https://codereview.chromium.org/22911027/) without having to contort
> nacl_helper_linux.cc further.

ok, lgtm

[commit-bot: I haz the power, 2013-10-11 16:26:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mseaborn@chromium.org/24449002/48001

[commit-bot: I haz the power, 2013-10-11 19:39:42]

Change committed as 228228