Non-SFI Mode: Build base/ library by PNaCL toolchain for nacl_helper_nonsfi.

Non-SFI Mode: Build base/ library by PNaCL toolchain for nacl_helper_nonsfi.

This CL is to build base/ for nacl_helper_nonsfi.
The library is similar to base_nacl, but slightly different:
- For rand_util, rand_util_posix should be used, instead of
  rand_util_nacl, because nacl_helper_nonsfi will be running under
  Linux directly.
- MessageLoopForIO should be based on MessagePumpLibevent rather than
  MessagePumpDefault, to support IPC.
- GetKnownDeadTerminationStatus, GetTerminationStatus,
  UnixDomainSocket::SendMsg and RecvMsg are included, as these are used
  to implement nacl_helper_nonsfi binary.
- GLIB is not supported. It is unnecessary for nacl_helper_nonsfi.
Note that this library is not used yet from any binary, because this CL
is just a preparation, but the library is built actually.

BUG=358465
TEST=Ran trybot. Implement nacl_helper_nonsfi on top of this CL, and made sure
it is working.

Committed: https://crrev.com/2b720d21260715614abea6f241874cc779f01332
Cr-Commit-Position: refs/heads/master@{#300075}

[hidehiko, 2014-10-14 19:45:58]

hidehiko@chromium.org changed reviewers:
+ mseaborn@chromium.org, thestig@chromium.org

[hidehiko, 2014-10-14 19:45:59]

Lei, Mark, could you take a look?
This is the refined version of the CL, which I shared with you a few days ago.
Now, the CLs depended on by this are landed, so I'd like to start the actual
code review for this.

Thank you for review in advance,
- hidehiko

[Lei Zhang, 2014-10-14 21:32:24]

Do you need to make GN changes as well?

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp
File base/base_nacl.gyp (right):

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode76
base/base_nacl.gyp:76: '<(DEPTH)/native_client/src/public/linux_syscalls',
It's a bit weird for base to depend on things, but I see base_nacl.gyp already
does this.

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode106
base/base_nacl.gyp:106: # rand_util_posix for random implementation, intead of
typo: "intead"

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode116
base/base_nacl.gyp:116: 'rand_util_nacl.cc',
I don't think you need this because rand_util_nacl.cc is not in sources above.

[hidehiko, 2014-10-15 02:20:55]

Thank you for quick review!
Could you take another look?

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp
File base/base_nacl.gyp (right):

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode76
base/base_nacl.gyp:76: '<(DEPTH)/native_client/src/public/linux_syscalls',
On 2014/10/14 21:32:23, Lei Zhang wrote:
> It's a bit weird for base to depend on things, but I see base_nacl.gyp already
> does this.

The PNaCl toolchain does not provide some header files for syscalls by default,
such as <sys/socket.h>. This is the include path to use it.

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode106
base/base_nacl.gyp:106: # rand_util_posix for random implementation, intead of
On 2014/10/14 21:32:23, Lei Zhang wrote:
> typo: "intead"

Done.

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode116
base/base_nacl.gyp:116: 'rand_util_nacl.cc',
On 2014/10/14 21:32:23, Lei Zhang wrote:
> I don't think you need this because rand_util_nacl.cc is not in sources above.

rand_util_nacl.cc is actually included in base.gypi, and is not excluded by
suffix rule (described in build/filename_rules.gypi). So here, we need to
exclude it manually.

[hidehiko, 2014-10-15 02:22:34]

> Do you need to make GN changes as well?
At the moment, the PNaCl toolchain build only supports GYP build. So I think we
can now skip GN changes.
Thank you for pointing this out.

- hidehiko


On 2014/10/15 02:20:55, hidehiko wrote:
> Thank you for quick review!
> Could you take another look?
> 
> https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp
> File base/base_nacl.gyp (right):
> 
> https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode76
> base/base_nacl.gyp:76: '<(DEPTH)/native_client/src/public/linux_syscalls',
> On 2014/10/14 21:32:23, Lei Zhang wrote:
> > It's a bit weird for base to depend on things, but I see base_nacl.gyp
already
> > does this.
> 
> The PNaCl toolchain does not provide some header files for syscalls by
default,
> such as <sys/socket.h>. This is the include path to use it.
> 
> https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode106
> base/base_nacl.gyp:106: # rand_util_posix for random implementation, intead of
> On 2014/10/14 21:32:23, Lei Zhang wrote:
> > typo: "intead"
> 
> Done.
> 
> https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode116
> base/base_nacl.gyp:116: 'rand_util_nacl.cc',
> On 2014/10/14 21:32:23, Lei Zhang wrote:
> > I don't think you need this because rand_util_nacl.cc is not in sources
above.
> 
> rand_util_nacl.cc is actually included in base.gypi, and is not excluded by
> suffix rule (described in build/filename_rules.gypi). So here, we need to
> exclude it manually.

[Lei Zhang, 2014-10-15 06:04:02]

lgtm, but please let mseaborn take a look too.

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp
File base/base_nacl.gyp (right):

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode116
base/base_nacl.gyp:116: 'rand_util_nacl.cc',
On 2014/10/15 02:20:55, hidehiko wrote:
> On 2014/10/14 21:32:23, Lei Zhang wrote:
> > I don't think you need this because rand_util_nacl.cc is not in sources
above.
> 
> rand_util_nacl.cc is actually included in base.gypi, and is not excluded by
> suffix rule (described in build/filename_rules.gypi). So here, we need to
> exclude it manually.

Ah, I see the "sources" above is a variable. That's rather confusing. I applied
your patch locally and I looked at the native_client/ gyp files. Now I better
understand what's going on and this makes sense.

[hidehiko, 2014-10-15 06:34:39]

Thank you for review, Lei!

Mark, friendly ping?
- hidehiko

[Mark Seaborn, 2014-10-16 23:03:09]

LGTM

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp
File base/base_nacl.gyp (right):

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode76
base/base_nacl.gyp:76: '<(DEPTH)/native_client/src/public/linux_syscalls',
On 2014/10/15 02:20:55, hidehiko wrote:
> On 2014/10/14 21:32:23, Lei Zhang wrote:
> > It's a bit weird for base to depend on things, but I see base_nacl.gyp
already
> > does this.
> 
> The PNaCl toolchain does not provide some header files for syscalls by
default,
> such as <sys/socket.h>. This is the include path to use it.

As an aside:  I suppose this could be added to include_dirs automatically by
NaCl's untrusted.gypi (for "build_nonsfi_helper" builds).  Do you think that
would be worthwhile doing?

https://codereview.chromium.org/659513004/diff/20001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/659513004/diff/20001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:159: // MSG_TRUNC nor MSG_CTRUNC.
Grammar nit: "nor" -> "or"

(It's either "doesn't support X or Y" or "supports neither X nor Y".)

[hidehiko, 2014-10-17 04:44:57]

Thank you for review.
I rebased it (build_common.gyp diff comes from the rebase).
Submitting.

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp
File base/base_nacl.gyp (right):

https://codereview.chromium.org/659513004/diff/1/base/base_nacl.gyp#newcode76
base/base_nacl.gyp:76: '<(DEPTH)/native_client/src/public/linux_syscalls',
On 2014/10/16 23:03:09, Mark Seaborn wrote:
> On 2014/10/15 02:20:55, hidehiko wrote:
> > On 2014/10/14 21:32:23, Lei Zhang wrote:
> > > It's a bit weird for base to depend on things, but I see base_nacl.gyp
> already
> > > does this.
> > 
> > The PNaCl toolchain does not provide some header files for syscalls by
> default,
> > such as <sys/socket.h>. This is the include path to use it.
> 
> As an aside:  I suppose this could be added to include_dirs automatically by
> NaCl's untrusted.gypi (for "build_nonsfi_helper" builds).  Do you think that
> would be worthwhile doing?

I think, yes. Please let me send out a separate CL for it.

https://codereview.chromium.org/659513004/diff/20001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/659513004/diff/20001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:159: // MSG_TRUNC nor MSG_CTRUNC.
On 2014/10/16 23:03:09, Mark Seaborn wrote:
> Grammar nit: "nor" -> "or"
> 
> (It's either "doesn't support X or Y" or "supports neither X nor Y".)

Done. Thank you!

[hidehiko, 2014-10-17 04:45:03]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-10-17 04:45:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/659513004/40001

[hidehiko, 2014-10-17 06:33:19]

The CQ bit was unchecked by hidehiko@chromium.org

[hidehiko, 2014-10-17 06:33:35]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2014-10-17 06:33:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/659513004/40001

[commit-bot: I haz the power, 2014-10-17 06:40:37]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-17 06:41:45]

Patchset 3 (id:??) landed as
https://crrev.com/2b720d21260715614abea6f241874cc779f01332
Cr-Commit-Position: refs/heads/master@{#300075}

[mdempsky, 2014-10-25 00:48:23]

mdempsky@chromium.org changed reviewers:
+ mdempsky@chromium.org

[mdempsky, 2014-10-25 00:48:23]

https://codereview.chromium.org/659513004/diff/40001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/659513004/diff/40001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:159: // MSG_TRUNC or MSG_CTRUNC.
Wait, what?  MSG_TRUNC and MSG_CTRUNC are POSIX-standard features (since at
least SUSv2), and this has security implications: a process that receives a
message over an AF_UNIX socket needs to be able to know whether it allocated
enough space for the message, or whether the message was truncated.

I can understand PNaCl not supporting Linux-isms like
SO_PASSCRED/SCM_CREDENTIALS, but omitting this part seems like a mistake.

[hidehiko, 2014-10-27 08:38:44]

https://codereview.chromium.org/659513004/diff/40001/base/posix/unix_domain_s...
File base/posix/unix_domain_socket_linux.cc (right):

https://codereview.chromium.org/659513004/diff/40001/base/posix/unix_domain_s...
base/posix/unix_domain_socket_linux.cc:159: // MSG_TRUNC or MSG_CTRUNC.
On 2014/10/25 00:48:23, mdempsky wrote:
> Wait, what?  MSG_TRUNC and MSG_CTRUNC are POSIX-standard features (since at
> least SUSv2), and this has security implications: a process that receives a
> message over an AF_UNIX socket needs to be able to know whether it allocated
> enough space for the message, or whether the message was truncated.
> 
> I can understand PNaCl not supporting Linux-isms like
> SO_PASSCRED/SCM_CREDENTIALS, but omitting this part seems like a mistake.

Thank you for pointing this out.
Mark, can we add MSG_TRUNC/MSG_CTRUNC into PNaCl toolchain (i.e.,
native_client/src/public/linux_syscalls/sys/socket.h)?

[Mark Seaborn, 2014-10-27 18:16:41]

On 27 October 2014 01:38, <hidehiko@chromium.org> wrote:

>
> https://codereview.chromium.org/659513004/diff/40001/base/
> posix/unix_domain_socket_linux.cc
> File base/posix/unix_domain_socket_linux.cc (right):
>
> https://codereview.chromium.org/659513004/diff/40001/base/
> posix/unix_domain_socket_linux.cc#newcode159
> base/posix/unix_domain_socket_linux.cc:159: // MSG_TRUNC or MSG_CTRUNC.
> On 2014/10/25 00:48:23, mdempsky wrote:
>
>> Wait, what?  MSG_TRUNC and MSG_CTRUNC are POSIX-standard features (since
>> at
>
>  least SUSv2), and this has security implications: a process that receives
>> a
>
>  message over an AF_UNIX socket needs to be able to know whether it
>> allocated
>
>  enough space for the message, or whether the message was truncated.
>>
>
>  I can understand PNaCl not supporting Linux-isms like
>> SO_PASSCRED/SCM_CREDENTIALS, but omitting this part seems like a mistake.
>
>
> Thank you for pointing this out.
> Mark, can we add MSG_TRUNC/MSG_CTRUNC into PNaCl toolchain (i.e.,
> native_client/src/public/linux_syscalls/sys/socket.h)?


Yes, that would be good.

Cheers,
Mark

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

[mdempsky, 2014-10-27 19:57:59]

Sounds good.  I've filed crbug.com/427625 to track this.  Thanks!

On Mon, Oct 27, 2014 at 11:16 AM, Mark Seaborn <mseaborn@chromium.org>
wrote:

> On 27 October 2014 01:38, <hidehiko@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/659513004/diff/40001/base/
>> posix/unix_domain_socket_linux.cc
>> File base/posix/unix_domain_socket_linux.cc (right):
>>
>> https://codereview.chromium.org/659513004/diff/40001/base/
>> posix/unix_domain_socket_linux.cc#newcode159
>> base/posix/unix_domain_socket_linux.cc:159: // MSG_TRUNC or MSG_CTRUNC.
>> On 2014/10/25 00:48:23, mdempsky wrote:
>>
>>> Wait, what?  MSG_TRUNC and MSG_CTRUNC are POSIX-standard features (since
>>> at
>>
>>  least SUSv2), and this has security implications: a process that
>>> receives a
>>
>>  message over an AF_UNIX socket needs to be able to know whether it
>>> allocated
>>
>>  enough space for the message, or whether the message was truncated.
>>>
>>
>>  I can understand PNaCl not supporting Linux-isms like
>>> SO_PASSCRED/SCM_CREDENTIALS, but omitting this part seems like a mistake.
>>
>>
>> Thank you for pointing this out.
>> Mark, can we add MSG_TRUNC/MSG_CTRUNC into PNaCl toolchain (i.e.,
>> native_client/src/public/linux_syscalls/sys/socket.h)?
>
>
> Yes, that would be good.
>
> Cheers,
> Mark
>
>

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