Tell nacl_helper to use non SFI mode in HandleForkRequest

Tell nacl_helper to use non SFI mode in HandleForkRequest

Before this patch, we were passing this info by the first IPC
to nacl_helper (NaClProcessMsg_Start). This timing is too late
for seccomp sandbox initialization.

This patch introduces a new process type, nacl-loader-nonsfi.
For now, nacl_helper says it can handle both nacl-loader and
nacl-loader-nonsfi. Once we have splitted nacl_helper into two
binaries, we will probably create two NaClForkDelegate
instances and let each of them to focus on a single process
type.

Also removed uses_nonsfi_mode from NaClStartParams. This is
unnecessary anymore.

BUG=https://code.google.com/p/nativeclient/issues/detail?id=3734
TEST=out/Release/browser_tests --gtest_filter='NaCl*' and trybot
R=jln@chromium.org, jochen@chromium.org, mseaborn@chromium.org

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

[Mark Seaborn, 2014-03-28 15:38:42]

https://codereview.chromium.org/216603002/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/216603002/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:596:
switches::kNaClNonSfiLoaderProcess :
Have you checked all the places that use kNaClLoaderProcess to see if they also
need to check for the Non-SFI variant?

I found one:
AdjustLinuxOOMScore() in chrome/app/chrome_main_delegate.cc

https://codereview.chromium.org/216603002/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:837: if (!kNonSFIModeSupported ||
This sanity check now happens after we've launched the process with
uses_nonsfi_mode=true.  It would be better to move it to happen earlier, just in
case.  (Also it would be good to comment it as a sanity check.)

https://codereview.chromium.org/216603002/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_switches.cc (right):

https://codereview.chromium.org/216603002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_switches.cc:42: // Value for --type that causes the
process to run as a NativeClient loader.
Add "for SFI Mode"?

https://codereview.chromium.org/216603002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_switches.cc:47: const char
kNaClNonSfiLoaderProcess[]       = "nacl-nonsfi-loader";
Nit: I'd have a slight preference for NaClLoaderNonSfiProcess[] =
"nacl-loader-nonsfi", since we've often used "nonsfi" as a suffix.

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

https://codereview.chromium.org/216603002/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:68:
listener.set_uses_nonsfi_mode(uses_nonsfi_mode);
Nit: why in the middle of the two existing calls?  When uses_nonsfi_mode is
true, it makes the other two settings irrelevant, so it would make sense to put
it at the start or the end.

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

https://codereview.chromium.org/216603002/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_listener.cc:270: // TODO(hamaji): Add
NaClChromeMainInitForNonSfi and use it when
Can you remove this comment?  See other review...

https://codereview.chromium.org/216603002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/216603002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:266: uses_nonsfi_mode_ =
process_type == switches::kNaClNonSfiLoaderProcess;
Having CanHelp() store state that's later used by Fork() doesn't seem like a
good idea.  That creates a very subtle gotcha.  The interface in
content/public/common/zygote_fork_delegate_linux.h doesn't say that CanHelp()
must be called immediately after Fork() for the same subprocess request.

Maybe add a process_type argument to Fork()?

It's probably better not to add a use_nonsfi_mode argument to Fork(), because
content's ZygoteForkDelegate interface is maintaining a pretence that it doesn't
depend on NaCl (which isn't entirely true :-) ).

+jln for this aspect.

[Mark Seaborn, 2014-03-28 15:47:45]

+jln for my previous comment about nacl_fork_delegate_linux.cc.

[hamaji, 2014-03-30 03:31:35]

https://codereview.chromium.org/216603002/diff/1/components/nacl/browser/nacl...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/216603002/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:596:
switches::kNaClNonSfiLoaderProcess :
On 2014/03/28 15:38:42, Mark Seaborn wrote:
> Have you checked all the places that use kNaClLoaderProcess to see if they
also
> need to check for the Non-SFI variant?
> 
> I found one:
> AdjustLinuxOOMScore() in chrome/app/chrome_main_delegate.cc

Sorry, I forgot to check this. It seems the one you mentioned is the only one we
need to modify. I couldn't understand nacl_broker_listener.cc well, but it seems
this is compiled only on windows.

https://codereview.chromium.org/216603002/diff/1/components/nacl/browser/nacl...
components/nacl/browser/nacl_process_host.cc:837: if (!kNonSFIModeSupported ||
On 2014/03/28 15:38:42, Mark Seaborn wrote:
> This sanity check now happens after we've launched the process with
> uses_nonsfi_mode=true.  It would be better to move it to happen earlier, just
in
> case.  (Also it would be good to comment it as a sanity check.)

Done. I've moved this to Launch(). It seems there are some other sanity checks
and the intention of the check seems to be clear as I've added
SendErrorToRenderer, so I didn't add any comments.

https://codereview.chromium.org/216603002/diff/1/components/nacl/common/nacl_...
File components/nacl/common/nacl_switches.cc (right):

https://codereview.chromium.org/216603002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_switches.cc:42: // Value for --type that causes the
process to run as a NativeClient loader.
On 2014/03/28 15:38:42, Mark Seaborn wrote:
> Add "for SFI Mode"?

Done.

https://codereview.chromium.org/216603002/diff/1/components/nacl/common/nacl_...
components/nacl/common/nacl_switches.cc:47: const char
kNaClNonSfiLoaderProcess[]       = "nacl-nonsfi-loader";
On 2014/03/28 15:38:42, Mark Seaborn wrote:
> Nit: I'd have a slight preference for NaClLoaderNonSfiProcess[] =
> "nacl-loader-nonsfi", since we've often used "nonsfi" as a suffix.

Done.

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

https://codereview.chromium.org/216603002/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_helper_linux.cc:68:
listener.set_uses_nonsfi_mode(uses_nonsfi_mode);
On 2014/03/28 15:38:42, Mark Seaborn wrote:
> Nit: why in the middle of the two existing calls?  When uses_nonsfi_mode is
> true, it makes the other two settings irrelevant, so it would make sense to
put
> it at the start or the end.

Done. I inserted this flag in a random position of NaClListener and I used the
random order here. I have also re-ordered fields of NaClListener as well.

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

https://codereview.chromium.org/216603002/diff/1/components/nacl/loader/nacl_...
components/nacl/loader/nacl_listener.cc:270: // TODO(hamaji): Add
NaClChromeMainInitForNonSfi and use it when
On 2014/03/28 15:38:42, Mark Seaborn wrote:
> Can you remove this comment?  See other review...

OK, it's easier :) I'll withdraw the patch for native_client repository.

https://codereview.chromium.org/216603002/diff/1/components/nacl/zygote/nacl_...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/216603002/diff/1/components/nacl/zygote/nacl_...
components/nacl/zygote/nacl_fork_delegate_linux.cc:266: uses_nonsfi_mode_ =
process_type == switches::kNaClNonSfiLoaderProcess;
On 2014/03/28 15:38:42, Mark Seaborn wrote:
> Having CanHelp() store state that's later used by Fork() doesn't seem like a
> good idea.  That creates a very subtle gotcha.  The interface in
> content/public/common/zygote_fork_delegate_linux.h doesn't say that CanHelp()
> must be called immediately after Fork() for the same subprocess request.
> 
> Maybe add a process_type argument to Fork()?
> 
> It's probably better not to add a use_nonsfi_mode argument to Fork(), because
> content's ZygoteForkDelegate interface is maintaining a pretence that it
doesn't
> depend on NaCl (which isn't entirely true :-) ).
> 
> +jln for this aspect.

Thanks for the suggestion! Yeah, as I've written in the seccomp patch, I was not
sure if this is a good idea. I was not also sure if changing content/ makes
sense for NaCl (though it seemed ZygoteForkDelegate is only for NaCl).

Done, by adding process_type argument.

[Mark Seaborn, 2014-03-31 17:19:59]

> This patch introduces a new process type, nacl-nonsfi-loader.

Nit: can you update the commit message?

LGTM

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:110: uses_nonsfi_mode_(false)
{}
Not used

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:259: uses_nonsfi_mode_ =
process_type == switches::kNaClLoaderNonSfiProcess;
Ditto

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:280: // We decide whether we
will use SFI mode or non-SFI based on the
This might be considered obvious from the code, so you could omit it.

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.h (right):

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.h:55: bool uses_nonsfi_mode_;
Not used any more

[hidehiko, 2014-03-31 18:56:17]

FYI:

https://codereview.chromium.org/216603002/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/216603002/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:611:
switches::kNaClLoaderNonSfiProcess :
nit: indent?

[Mark Seaborn, 2014-03-31 19:35:56]

https://codereview.chromium.org/216603002/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/216603002/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:611:
switches::kNaClLoaderNonSfiProcess :
On 2014/03/31 18:56:17, hidehiko wrote:
> nit: indent?

Or '()' brackets, i.e.
foo(blah,
    (blah ?
     blah : blah));

[hamaji, 2014-04-01 06:53:09]

https://codereview.chromium.org/216603002/diff/20001/components/nacl/browser/...
File components/nacl/browser/nacl_process_host.cc (right):

https://codereview.chromium.org/216603002/diff/20001/components/nacl/browser/...
components/nacl/browser/nacl_process_host.cc:611:
switches::kNaClLoaderNonSfiProcess :
On 2014/03/31 19:35:56, Mark Seaborn wrote:
> On 2014/03/31 18:56:17, hidehiko wrote:
> > nit: indent?
> 
> Or '()' brackets, i.e.
> foo(blah,
>     (blah ?
>      blah : blah));

Done.

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.cc (right):

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:110: uses_nonsfi_mode_(false)
{}
On 2014/03/31 17:19:59, Mark Seaborn wrote:
> Not used

Oops, removed.

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:259: uses_nonsfi_mode_ =
process_type == switches::kNaClLoaderNonSfiProcess;
On 2014/03/31 17:19:59, Mark Seaborn wrote:
> Ditto

Done.

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.cc:280: // We decide whether we
will use SFI mode or non-SFI based on the
On 2014/03/31 17:19:59, Mark Seaborn wrote:
> This might be considered obvious from the code, so you could omit it.

Done.

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
File components/nacl/zygote/nacl_fork_delegate_linux.h (right):

https://codereview.chromium.org/216603002/diff/20001/components/nacl/zygote/n...
components/nacl/zygote/nacl_fork_delegate_linux.h:55: bool uses_nonsfi_mode_;
On 2014/03/31 17:19:59, Mark Seaborn wrote:
> Not used any more

Done.

[hamaji, 2014-04-01 06:56:25]

Adding jochen@ for content/ and chrome/ approval.

[jochen (gone - plz use gerrit), 2014-04-01 11:28:18]

lgtm

[hamaji, 2014-04-01 11:58:22]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-04-01 11:58:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/40001

[commit-bot: I haz the power, 2014-04-01 12:36:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 12:36:57]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[hamaji, 2014-04-01 15:20:47]

On 2014/04/01 12:36:57, I haz the power (commit-bot) wrote:
> Retried try job too often on chromium_presubmit for step(s) presubmit
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

Oops, I needed an approval from a security person. Julien, could you check this?

[hamaji, 2014-04-01 15:40:18]

Handled a Mark's comment in https://codereview.chromium.org/218633011/

Now uses_nonsfi_mode_ is always available. On Mac and Windows, its value should
be always false. I have added a CHECK at the beginning of OnStart, just in case.

[jln (very slow on Chromium), 2014-04-01 21:04:16]

That's a much better model, thanks!

Also components/nacl/common/nacl_messages.h lgtm

[Mark Seaborn, 2014-04-01 21:21:14]

The CQ bit was checked by mseaborn@chromium.org

[commit-bot: I haz the power, 2014-04-01 22:08:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/60001

[commit-bot: I haz the power, 2014-04-01 22:18:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-01 22:18:41]

Internal error: failed to checkout. Please try again.

[Mark Seaborn, 2014-04-01 23:36:00]

The CQ bit was checked by mseaborn@chromium.org

[commit-bot: I haz the power, 2014-04-01 23:36:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/60001

[commit-bot: I haz the power, 2014-04-02 00:19:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 00:19:55]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Mark Seaborn, 2014-04-02 00:22:22]

https://codereview.chromium.org/216603002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (left):

https://codereview.chromium.org/216603002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:289: #if defined(OS_LINUX)
You can't remove these #ifs.  This code won't compile/link on Mac or Windows.

[hamaji, 2014-04-02 03:20:43]

https://codereview.chromium.org/216603002/diff/60001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (left):

https://codereview.chromium.org/216603002/diff/60001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:289: #if defined(OS_LINUX)
On 2014/04/02 00:22:22, Mark Seaborn wrote:
> You can't remove these #ifs.  This code won't compile/link on Mac or Windows.

Ah, I forgot sending a try job. Thanks for checking this.

[hamaji, 2014-04-02 03:20:48]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-04-02 03:20:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/80001

[commit-bot: I haz the power, 2014-04-02 04:28:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 04:28:03]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_compile_dbg

[Mark Seaborn, 2014-04-02 05:45:15]

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (left):

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:423: #if defined(OS_LINUX)
You can't remove this #if/#endif either.

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:295: #endif
You can't move the #endif to here.

[hamaji, 2014-04-02 08:38:39]

I might be too relying on the commit queue :(

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (left):

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:423: #if defined(OS_LINUX)
On 2014/04/02 05:45:16, Mark Seaborn wrote:
> You can't remove this #if/#endif either.

Done.

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
File components/nacl/loader/nacl_listener.cc (right):

https://codereview.chromium.org/216603002/diff/80001/components/nacl/loader/n...
components/nacl/loader/nacl_listener.cc:295: #endif
On 2014/04/02 05:45:16, Mark Seaborn wrote:
> You can't move the #endif to here.

Done.

[hamaji, 2014-04-02 08:38:51]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-04-02 08:41:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/90001

[commit-bot: I haz the power, 2014-04-02 16:31:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:31:07]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[hamaji, 2014-04-02 16:39:53]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:40:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/90001

[hamaji, 2014-04-02 16:41:26]

The CQ bit was unchecked by hamaji@chromium.org

[hamaji, 2014-04-02 16:44:55]

I thought this is failing due to a flakiness, but this could be a real issue. I
found a line like

[2548:2692:0402/024300:ERROR:nacl_process_host.cc(312)] NaCl process exited with
status 1 (0x1)

Looking...

[hamaji, 2014-04-02 16:54:07]

Hmm not sure how this can break one win builder. I'd cq again.

[hamaji, 2014-04-02 16:54:14]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-04-02 16:54:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/90001

[commit-bot: I haz the power, 2014-04-02 23:36:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-02 23:36:58]

Retried try job too often on android_aosp for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...

[hamaji, 2014-04-03 02:29:34]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-04-03 02:35:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/hamaji@chromium.org/216603002/90001

[hamaji, 2014-04-03 04:42:05]

Committed patchset #6 manually as r261279 (presubmit successful).

[hamaji, 2014-04-03 04:44:19]

On 2014/04/03 04:42:05, hamaji wrote:
> Committed patchset #6 manually as r261279 (presubmit successful).

So, android_aosp is not working right now. Other chrome devs around me also said
it's annoying. I don't know why cq does not retry win_swarm_triggered. I'll
watch the tree for a while.