Split NaCl SFI and non-SFI helpers into separate processes

components/nacl/zygote/nacl_fork_delegate_linux.cc

Index: components/nacl/zygote/nacl_fork_delegate_linux.cc
diff --git a/components/nacl/zygote/nacl_fork_delegate_linux.cc b/components/nacl/zygote/nacl_fork_delegate_linux.cc
index 0e5f8e7eccc23c1a44a5a718c34164aa1271385e..347ace47b4b7c4431ec23a1b3bbbd70aa17874ae 100644
--- a/components/nacl/zygote/nacl_fork_delegate_linux.cc
+++ b/components/nacl/zygote/nacl_fork_delegate_linux.cc
@@ -27,6 +27,7 @@
 #include "base/process/kill.h"
 #include "base/process/launch.h"
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
+#include "build/build_config.h"
 #include "components/nacl/common/nacl_paths.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "components/nacl/loader/nacl_helper_linux.h"
@@ -42,6 +43,13 @@ const char kNaClHelperReservedAtZero[] =
     "--reserved_at_zero=0xXXXXXXXXXXXXXXXX";
 const char kNaClHelperRDebug[] = "--r_debug=0xXXXXXXXXXXXXXXXX";
 
+// We only enable non-SFI mode by default on ChromeOS/ARMEL.
+#if defined(OS_CHROMEOS) || defined(ARCH_CPU_ARMEL)

[Mark Seaborn, 2014/05/10 00:10:40]

I think Julien wanted to de-duplicate this condition, so once Elijah's change is
committed, maybe you could rebase and move the definition of this constant to a
header file?

[jln (very slow on Chromium), 2014/05/10 00:44:50]

We could even make it a function that also queries the command line and return
whether or not to enable nonsfi. WDYT?

[mdempsky, 2014/05/10 01:58:52]

Looking at Elijah's pending CL, it doesn't look straight forward to me to
reconcile, but I can still give it a shot once his change has landed.

In the mean time, I restructured this code a bit to try to make it clearer.

[mdempsky, 2014/05/12 22:38:30]

Done: I've moved the IsNonSFIModeEnabled() function from nacl/renderer into
nacl/common, and I'm now also using it in nacl/zygote.

+const bool kEnableNonSFIByDefault = true;
+#else
+const bool kEnableNonSFIByDefault = false;
+#endif
+
 #if defined(ARCH_CPU_X86)
 bool NonZeroSegmentBaseIsSlow() {
   base::CPU cpuid;
@@ -108,14 +116,24 @@ bool SendIPCRequestAndReadReply(int ipc_channel,
 
 }  // namespace.
 
-NaClForkDelegate::NaClForkDelegate()
-    : status_(kNaClHelperUnused),
-      fd_(-1) {}
+NaClForkDelegate::NaClForkDelegate(bool nonsfi_mode)
+    : nonsfi_mode_(nonsfi_mode), status_(kNaClHelperUnused), fd_(-1) {
+}
 
 void NaClForkDelegate::Init(const int sandboxdesc,
                             const bool enable_layer1_sandbox) {
   VLOG(1) << "NaClForkDelegate::Init()";
-  int fds[2];
+
+  if (nonsfi_mode_) {
+    // Only launch the non-SFI helper process if we support non-SFI by default
+    // for this build target, or it's explicitly enabled via the command line.
+    const bool enable_nonsfi = kEnableNonSFIByDefault ||
+                               CommandLine::ForCurrentProcess()->HasSwitch(
+                                   switches::kEnableNaClNonSfiMode);
+    if (!enable_nonsfi) {
+      return;
+    }
+  }
 
   scoped_ptr<sandbox::SetuidSandboxClient> setuid_sandbox_client(
       sandbox::SetuidSandboxClient::Create());
@@ -127,7 +145,8 @@ void NaClForkDelegate::Init(const int sandboxdesc,
   // Confirm a hard-wired assumption.
   DCHECK_EQ(sandboxdesc, nacl_sandbox_descriptor);
 
-  CHECK(socketpair(PF_UNIX, SOCK_SEQPACKET, 0, fds) == 0);
+  int fds[2];
+  PCHECK(0 == socketpair(PF_UNIX, SOCK_SEQPACKET, 0, fds));
   base::FileHandleMappingVector fds_to_map;
   fds_to_map.push_back(std::make_pair(fds[1], kNaClZygoteDescriptor));
   fds_to_map.push_back(std::make_pair(sandboxdesc, nacl_sandbox_descriptor));
@@ -262,7 +281,8 @@ void NaClForkDelegate::Init(const int sandboxdesc,
 void NaClForkDelegate::InitialUMA(std::string* uma_name,
                                   int* uma_sample,
                                   int* uma_boundary_value) {
-  *uma_name = "NaCl.Client.Helper.InitState";
+  *uma_name = nonsfi_mode_ ? "NaCl.Client.HelperNonSFI.InitState"
+                           : "NaCl.Client.Helper.InitState";
   *uma_sample = status_;
   *uma_boundary_value = kNaClHelperStatusBoundary;
 }
@@ -279,10 +299,14 @@ bool NaClForkDelegate::CanHelp(const std::string& process_type,
                                std::string* uma_name,
                                int* uma_sample,
                                int* uma_boundary_value) {
-  if (process_type != switches::kNaClLoaderProcess &&
-      process_type != switches::kNaClLoaderNonSfiProcess)
+  // We can only help with a specific process type depending on nonsfi_mode_.
+  const char* helpable_process_type = nonsfi_mode_
+                                          ? switches::kNaClLoaderNonSfiProcess
+                                          : switches::kNaClLoaderProcess;
+  if (process_type != helpable_process_type)
     return false;
-  *uma_name = "NaCl.Client.Helper.StateOnFork";
+  *uma_name = nonsfi_mode_ ? "NaCl.Client.HelperNonSFI.StateOnFork"
+                           : "NaCl.Client.Helper.StateOnFork";
   *uma_sample = status_;
   *uma_boundary_value = kNaClHelperStatusBoundary;
   return true;
@@ -303,11 +327,7 @@ pid_t NaClForkDelegate::Fork(const std::string& process_type,
   // First, send a remote fork request.
   Pickle write_pickle;
   write_pickle.WriteInt(nacl::kNaClForkRequest);
-  // TODO(hamaji): When we split the helper binary for non-SFI mode
-  // from nacl_helper, stop sending this information.
-  const bool uses_nonsfi_mode =
-    process_type == switches::kNaClLoaderNonSfiProcess;
-  write_pickle.WriteBool(uses_nonsfi_mode);
+  write_pickle.WriteBool(nonsfi_mode_);
   write_pickle.WriteString(channel_id);
 
   char reply_buf[kNaClMaxIPCMessageLength];