Non-SFI: move socketpair() from plugin process to browser process.

components/nacl/browser/nacl_process_host.cc

Index: components/nacl/browser/nacl_process_host.cc
diff --git a/components/nacl/browser/nacl_process_host.cc b/components/nacl/browser/nacl_process_host.cc
index 22f2f5f4554a000f4c508c60e9d18064e604e507..352f73b8b014b765115460930f5c20ed8d18067d 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -672,37 +672,31 @@ bool NaClProcessHost::LaunchSelLdr() {
 }
 
 bool NaClProcessHost::OnMessageReceived(const IPC::Message& msg) {
-  bool handled = true;
   if (uses_nonsfi_mode_) {
-    // IPC messages relating to NaCl's validation cache must not be exposed

[Mark Seaborn, 2015/05/12 23:24:06]

Please keep this comment, and add it after "// In Non-SFI mode, no message is
expected."

[hidehiko, 2015/05/13 06:39:20]

Done.

-    // in Non-SFI Mode, otherwise a Non-SFI nexe could use
-    // SetKnownToValidate to create a hole in the SFI sandbox.
-    IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
-      IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelsCreated,
-                          OnPpapiChannelsCreated)
-      IPC_MESSAGE_UNHANDLED(handled = false)
-    IPC_END_MESSAGE_MAP()
-  } else {
-    IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_QueryKnownToValidate,
-                          OnQueryKnownToValidate)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_SetKnownToValidate,
-                          OnSetKnownToValidate)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileToken,
-                          OnResolveFileToken)
+    // In Non-SFI mode, no message is expected.
+    return false;
+  }
+
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(NaClProcessHost, msg)
+    IPC_MESSAGE_HANDLER(NaClProcessMsg_QueryKnownToValidate,
+                        OnQueryKnownToValidate)
+    IPC_MESSAGE_HANDLER(NaClProcessMsg_SetKnownToValidate,
+                        OnSetKnownToValidate)
+    IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileToken,
+                        OnResolveFileToken)
 
 #if defined(OS_WIN)
-      IPC_MESSAGE_HANDLER_DELAY_REPLY(
-          NaClProcessMsg_AttachDebugExceptionHandler,
-          OnAttachDebugExceptionHandler)
-      IPC_MESSAGE_HANDLER(NaClProcessHostMsg_DebugStubPortSelected,
-                          OnDebugStubPortSelected)
+    IPC_MESSAGE_HANDLER_DELAY_REPLY(
+        NaClProcessMsg_AttachDebugExceptionHandler,
+        OnAttachDebugExceptionHandler)
+    IPC_MESSAGE_HANDLER(NaClProcessHostMsg_DebugStubPortSelected,
+                        OnDebugStubPortSelected)
 #endif
-      IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelsCreated,
-                          OnPpapiChannelsCreated)
-      IPC_MESSAGE_UNHANDLED(handled = false)
-    IPC_END_MESSAGE_MAP()
-  }
+    IPC_MESSAGE_HANDLER(NaClProcessHostMsg_PpapiChannelsCreated,
+                        OnPpapiChannelsCreated)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
   return handled;
 }
 
@@ -872,6 +866,12 @@ bool NaClProcessHost::StartNaClExecution() {
       base::ProcessId pid = base::GetProcId(process_->GetData().handle);
       LOG(WARNING) << "nonsfi nacl plugin running in " << pid;
     }
+
+    DCHECK(params.enable_ipc_proxy);
+    if (!params.enable_ipc_proxy) {

[Mark Seaborn, 2015/05/12 23:24:06]

This is basically input validation of what the renderer passed to the browser
process.  It would be better to do this earlier, in Launch().

[hidehiko, 2015/05/13 06:39:20]

Done.

+      LOG(ERROR) << "In Non-SFI mode, PPAPI proxy must be enabled.";
+      return false;
+    }
   } else {
     params.validation_cache_enabled = nacl_browser->ValidationCacheIsEnabled();
     params.validation_cache_key = nacl_browser->GetValidationCacheKey();
@@ -975,9 +975,7 @@ bool NaClProcessHost::StartNaClExecution() {
     }
   }
 
-  params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
-                                                   process_->GetData().handle);
-  process_->Send(new NaClProcessMsg_Start(params));
+  StartNaClFileResolved(params, base::FilePath(), base::File());
   return true;
 }
 
@@ -998,45 +996,108 @@ void NaClProcessHost::StartNaClFileResolved(
     params.nexe_file = IPC::TakeFileHandleForProcess(
         nexe_file_.Pass(), process_->GetData().handle);
   }
+
+#if defined(OS_LINUX)
+  // In Non-SFI mode, create socket pairs for IPC channels, here, unlike in

[Mark Seaborn, 2015/05/12 23:24:06]

Grammar nit: could remove comma before "here"

[hidehiko, 2015/05/13 06:39:20]

Done.

+  // SFI-mode, in which those channels are created in nacl_listener.cc.
+  // This is for security hardening. We can then prohibit the socketpair()
+  // system call in nacl_helper and nacl_helper_nonsfi.
+  if (uses_nonsfi_mode_) {
+    // Note: here, because some resources for the plugin process are already

[Mark Seaborn, 2015/05/12 23:24:06]

Nit: "plugin process" -> "NaCl loader process", x2

Also "resources" -> "FDs" to be more specific

[hidehiko, 2015/05/13 06:39:20]

Done.

+    // opened, they must be sent to plugin process because these cannot be

[Mark Seaborn, 2015/05/12 23:24:06]

"cannot be closed in this process" could be misleading for someone who doesn't
understand the background.  (They *could* be closed on Unix, but making the same
logic work on Windows adds further complication.)

A better explanation might be something like "...are transferred to NaCl loader
process even if an error occurs first, because this is the simplest way to
ensure that these FDs don't get leaked and that the NaCl loader process will
exit properly".

[hidehiko, 2015/05/13 06:39:20]

Done.

+    // closed in this process properly.
+    bool has_error = false;
+
+    // The case this condition is not satisfied should be handled in

[Mark Seaborn, 2015/05/12 23:24:06]

Nit: "The case where this condition..." perhaps?  Or you could say: "Note that
this check is redundant; we check this earlier".

[hidehiko, 2015/05/13 06:39:20]

Done.

+    // StartNaClExecution().
+    DCHECK(params.enable_ipc_proxy);
+
+    ScopedChannelHandle ppapi_browser_server_channel_handle;

[Mark Seaborn, 2015/05/12 23:24:06]

I wonder if it would it be worth doing:

typedef std::pair<ScopedChannelHandle, ScopedChannelHandle> ChannelHandlePair;
bool CreateChannelHandlePair(ChannelHandlePair* pair);

That would make this a bit less repetitive.

[hidehiko, 2015/05/13 06:39:20]

I do not want to introduce such a pair. Indeed it removes 8 lines from here, but
instead we need to distribute .first and .second.

+    ScopedChannelHandle ppapi_browser_client_channel_handle;
+    ScopedChannelHandle ppapi_renderer_server_channel_handle;
+    ScopedChannelHandle ppapi_renderer_client_channel_handle;
+    ScopedChannelHandle trusted_service_server_channel_handle;
+    ScopedChannelHandle trusted_service_client_channel_handle;
+    ScopedChannelHandle manifest_service_server_channel_handle;
+    ScopedChannelHandle manifest_service_client_channel_handle;
+
+    if (!CreateChannelHandlePair(&ppapi_browser_server_channel_handle,
+                                 &ppapi_browser_client_channel_handle) ||
+        !CreateChannelHandlePair(&ppapi_renderer_server_channel_handle,
+                                 &ppapi_renderer_client_channel_handle) ||
+        !CreateChannelHandlePair(&trusted_service_server_channel_handle,
+                                 &trusted_service_client_channel_handle) ||
+        !CreateChannelHandlePair(&manifest_service_server_channel_handle,
+                                 &manifest_service_client_channel_handle)) {
+      SendErrorToRenderer("Failed to create socket pairs.");
+      has_error = true;
+    }
+
+    if (!has_error &&
+        !StartPPAPIProxy(ppapi_browser_client_channel_handle.Pass())) {
+      SendErrorToRenderer("Failed to start browser PPAPI proxy.");
+      has_error = true;
+    }
+
+    if (!has_error) {
+      // On success, send back a success message to the renderer process,
+      // and, transfer the channel handles for the plugin process to |params|.

[Mark Seaborn, 2015/05/12 23:24:06]

Nit: remove comma after "and"

Also "plugin process" -> "NaCl loader process"

However, isn't this comment just echoing the code below?  You could omit it.

[hidehiko, 2015/05/13 06:39:20]

Done.

+      ReplyToRenderer(ppapi_renderer_client_channel_handle.Pass(),
+                      trusted_service_client_channel_handle.Pass(),
+                      manifest_service_client_channel_handle.Pass());
+      params.ppapi_browser_channel_handle =
+          ppapi_browser_server_channel_handle.release();
+      params.ppapi_renderer_channel_handle =
+          ppapi_renderer_server_channel_handle.release();
+      params.trusted_service_channel_handle =
+          trusted_service_server_channel_handle.release();
+      params.manifest_service_channel_handle =
+          manifest_service_server_channel_handle.release();
+    }
+  }
+#endif
+
   process_->Send(new NaClProcessMsg_Start(params));
 }
 
-// This method is called when NaClProcessHostMsg_PpapiChannelCreated is
-// received.
-void NaClProcessHost::OnPpapiChannelsCreated(
-    const IPC::ChannelHandle& raw_browser_channel_handle,
-    const IPC::ChannelHandle& raw_ppapi_renderer_channel_handle,
-    const IPC::ChannelHandle& raw_trusted_renderer_channel_handle,
-    const IPC::ChannelHandle& raw_manifest_service_channel_handle) {
-  ScopedChannelHandle browser_channel_handle(raw_browser_channel_handle);
-  ScopedChannelHandle ppapi_renderer_channel_handle(
-      raw_ppapi_renderer_channel_handle);
-  ScopedChannelHandle trusted_renderer_channel_handle(
-      raw_trusted_renderer_channel_handle);
-  ScopedChannelHandle manifest_service_channel_handle(
-      raw_manifest_service_channel_handle);
-
-  if (!enable_ppapi_proxy()) {
-    ReplyToRenderer(ScopedChannelHandle(),
-                    trusted_renderer_channel_handle.Pass(),
-                    manifest_service_channel_handle.Pass());
-    return;
+#if defined(OS_LINUX)
+// static
+bool NaClProcessHost::CreateChannelHandlePair(
+    ScopedChannelHandle* channel_handle1,
+    ScopedChannelHandle* channel_handle2) {
+  DCHECK(channel_handle1);
+  DCHECK(channel_handle2);
+
+  int fd1 = -1;
+  int fd2 = -1;
+  if (!IPC::SocketPair(&fd1, &fd2)) {
+    return false;
   }
 
+  IPC::ChannelHandle handle = IPC::Channel::GenerateVerifiedChannelID("nacl");
+  handle.socket = base::FileDescriptor(fd1, true);
+  channel_handle1->reset(handle);
+  handle.socket = base::FileDescriptor(fd2, true);
+  channel_handle2->reset(handle);
+  return true;
+}
+#endif
+
+bool NaClProcessHost::StartPPAPIProxy(ScopedChannelHandle channel_handle) {
   if (ipc_proxy_channel_.get()) {
     // Attempt to open more than 1 browser channel is not supported.
     // Shut down the NaCl process.
     process_->GetHost()->ForceShutdown();
-    return;
+    return false;
   }
 
   DCHECK_EQ(PROCESS_TYPE_NACL_LOADER, process_->GetData().process_type);
 
-  ipc_proxy_channel_ =
-      IPC::ChannelProxy::Create(browser_channel_handle.release(),
-                                IPC::Channel::MODE_CLIENT,
-                                NULL,
-                                base::MessageLoopProxy::current().get());
+  ipc_proxy_channel_ = IPC::ChannelProxy::Create(
+      channel_handle.release(),
+      IPC::Channel::MODE_CLIENT,
+      NULL,
+      base::MessageLoopProxy::current().get());
   // Create the browser ppapi host and enable PPAPI message dispatching to the
   // browser process.
   ppapi_host_.reset(content::BrowserPpapiHost::CreateExternalPluginProcess(
@@ -1076,6 +1137,38 @@ void NaClProcessHost::OnPpapiChannelsCreated(
 
   // Send a message to initialize the IPC dispatchers in the NaCl plugin.
   ipc_proxy_channel_->Send(new PpapiMsg_InitializeNaClDispatcher(args));
+  return true;
+}
+
+// This method is called when NaClProcessHostMsg_PpapiChannelCreated is
+// received.
+void NaClProcessHost::OnPpapiChannelsCreated(
+    const IPC::ChannelHandle& raw_ppapi_browser_channel_handle,
+    const IPC::ChannelHandle& raw_ppapi_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_trusted_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_manifest_service_channel_handle) {
+  ScopedChannelHandle ppapi_browser_channel_handle(
+      raw_ppapi_browser_channel_handle);
+  ScopedChannelHandle ppapi_renderer_channel_handle(
+      raw_ppapi_renderer_channel_handle);
+  ScopedChannelHandle trusted_renderer_channel_handle(
+      raw_trusted_renderer_channel_handle);
+  ScopedChannelHandle manifest_service_channel_handle(
+      raw_manifest_service_channel_handle);
+
+  if (enable_ppapi_proxy()) {
+    if (!StartPPAPIProxy(ppapi_browser_channel_handle.Pass())) {
+      SendErrorToRenderer("Browser PPAPI proxy could not start.");
+      return;
+    }
+  } else {
+    // If PPAPI proxy is disabled, channel handles should be invalid.
+    DCHECK(ppapi_browser_channel_handle.get().name.empty());
+    DCHECK(ppapi_renderer_channel_handle.get().name.empty());
+    // Invalidate, just in case.
+    ppapi_browser_channel_handle.reset();
+    ppapi_renderer_channel_handle.reset();
+  }
 
   // Let the renderer know that the IPC channels are established.
   ReplyToRenderer(ppapi_renderer_channel_handle.Pass(),