Refactor NaClProcessHost. Reduce chances to leak the resource.

components/nacl/browser/nacl_process_host.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/nacl/browser/nacl_process_host.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
 #include "base/base_switches.h"
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/files/file_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/move.h"
 #include "base/path_service.h"
 #include "base/process/launch.h"
 #include "base/process/process_iterator.h"
 #include "base/rand_util.h"
+#include "base/scoped_generic.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/threading/sequenced_worker_pool.h"
 #include "base/win/windows_version.h"
 #include "build/build_config.h"
 #include "components/nacl/browser/nacl_browser.h"
 #include "components/nacl/browser/nacl_browser_delegate.h"
 #include "components/nacl/browser/nacl_host_message_filter.h"
 #include "components/nacl/common/nacl_cmd_line.h"
 #include "components/nacl/common/nacl_host_messages.h"
 #include "components/nacl/common/nacl_messages.h"
 #include "components/nacl/common/nacl_process_type.h"
 #include "components/nacl/common/nacl_switches.h"
 #include "content/public/browser/browser_child_process_host.h"
 #include "content/public/browser/browser_ppapi_host.h"
 #include "content/public/browser/child_process_data.h"
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/sandboxed_process_launcher_delegate.h"
 #include "ipc/ipc_channel.h"
+#include "ipc/ipc_channel_handle.h"
+#include "ipc/ipc_platform_file.h"
 #include "ipc/ipc_switches.h"
 #include "native_client/src/shared/imc/nacl_imc_c.h"
 #include "net/base/net_util.h"
 #include "net/socket/tcp_listen_socket.h"
 #include "ppapi/host/host_factory.h"
 #include "ppapi/host/ppapi_host.h"
 #include "ppapi/proxy/ppapi_messages.h"
 #include "ppapi/shared_impl/ppapi_constants.h"
 #include "ppapi/shared_impl/ppapi_nacl_plugin_args.h"
 
@@ skipping 84 matching lines @@
   return (base::win::OSInfo::GetInstance()->wow64_status() ==
           base::win::OSInfo::WOW64_ENABLED);
 }
 
 }  // namespace
 
 #endif  // defined(OS_WIN)
 
 namespace {
 
+// Usually, NaClProcessHost is working on IO thread, where file operation is

[Mark Seaborn, 2015/04/17 22:13:45]

As an aside: I am sceptical that close() or CloseHandle() really block.  I think
this might be overzealousness on the part of the blocking checks in base/.  But
I don't have any hard data about this.

[hidehiko, 2015/04/30 16:33:01]

According to my coworker who is the expert of Windows, CloseHandle does in most
cases practically (no spec), though close() on, e.g., linux, do not. Though, it
depends on what resource we're releasing.

+// not allowed. So, even just for closing the file, it is necessary to post
+// the task to blocking pool. Note that, it is safer to use this from
+// NaClProcessHost's destructor runnign on IO thread, because BlockingPool

[Mark Seaborn, 2015/04/17 22:13:45]

"running"

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+// is ensured to be alive while the IO thread is alive.
+void CloseFile(base::File file) {

[Mark Seaborn, 2015/04/17 22:13:45]

CloseFile() is a rather generic name.  Maybe CloseFileInBlockingPool() or
CloseFileOnFileThread()?

[hidehiko, 2015/04/30 16:33:01]

Then, probably PostCloseFile etc. would be better.
In chrome tree, OnSomeThread suffix is used for functions which run on the
thread actually, rather than a function to post a task to the target thread, so
maybe it is better to avoid using it here.

Reflected in another CL.

+  if (!file.IsValid()) {
+    return;
+  }
+
+  content::BrowserThread::GetBlockingPool()->PostTask(
+      FROM_HERE,
+      base::Bind(&(ignore_result<base::File>), base::Passed(&file)));

[Mark Seaborn, 2015/04/17 22:13:45]

Nit: is it possible to omit the ()s in "&(ignore_result<base::File>)"?  (I'm not
sure.)

[hidehiko, 2015/04/30 16:33:02]

Reflected in another CL. I just misunderstood it must be needed.

+}
+
+// Define "Scoped" handles as utilities to avoid resource leaks.
+struct PlatformFileForTransitTraits {
+  static IPC::PlatformFileForTransit InvalidValue() {
+    return IPC::InvalidPlatformFileForTransit();
+  }
+
+  static void Free(const IPC::PlatformFileForTransit& file) {
+    CloseFile(IPC::PlatformFileForTransitToFile(file).Pass());

[Mark Seaborn, 2015/04/17 22:13:45]

I think this is unsafe on Windows, because this is a handle in another process. 
See my comment below about ScopedSharedMemoryHandle.

[hidehiko, 2015/04/30 16:33:01]

You're right. Fixed.

+  }
+};
+
+typedef base::ScopedGeneric<
+    IPC::PlatformFileForTransit, PlatformFileForTransitTraits>
+    ScopedPlatformFileForTransit;
+
+struct SharedMemoryHandleTraits {
+  static base::SharedMemoryHandle InvalidValue() {
+    return base::SharedMemory::NULLHandle();
+  }
+
+  static void Free(const base::SharedMemoryHandle& handle) {
+    DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::IO));
+    base::SharedMemory::CloseHandle(handle);
+  }
+};
+
+typedef base::ScopedGeneric<
+    base::SharedMemoryHandle, SharedMemoryHandleTraits>
+    ScopedSharedMemoryHandle;
+
 // NOTE: changes to this class need to be reviewed by the security team.
 class NaClSandboxedProcessLauncherDelegate
     : public content::SandboxedProcessLauncherDelegate {
  public:
   NaClSandboxedProcessLauncherDelegate(ChildProcessHost* host)
 #if defined(OS_POSIX)
       : ipc_fd_(host->TakeClientFileDescriptor())
 #endif
   {}
 
@@ skipping 25 matching lines @@
 
 void SetCloseOnExec(NaClHandle fd) {
 #if defined(OS_POSIX)
   int flags = fcntl(fd, F_GETFD);
   CHECK_NE(flags, -1);
   int rc = fcntl(fd, F_SETFD, flags | FD_CLOEXEC);
   CHECK_EQ(rc, 0);
 #endif
 }
 
-bool ShareHandleToSelLdr(
-    base::ProcessHandle processh,
-    NaClHandle sourceh,
-    bool close_source,
-    std::vector<nacl::FileDescriptor> *handles_for_sel_ldr) {
-#if defined(OS_WIN)
-  HANDLE channel;
-  int flags = DUPLICATE_SAME_ACCESS;
-  if (close_source)
-    flags |= DUPLICATE_CLOSE_SOURCE;
-  if (!DuplicateHandle(GetCurrentProcess(),
-                       reinterpret_cast<HANDLE>(sourceh),
-                       processh,
-                       &channel,
-                       0,  // Unused given DUPLICATE_SAME_ACCESS.
-                       FALSE,
-                       flags)) {
-    LOG(ERROR) << "DuplicateHandle() failed";
-    return false;
-  }
-  handles_for_sel_ldr->push_back(
-      reinterpret_cast<nacl::FileDescriptor>(channel));
-#else
-  nacl::FileDescriptor channel;
-  channel.fd = sourceh;
-  channel.auto_close = close_source;
-  handles_for_sel_ldr->push_back(channel);
-#endif
-  return true;
-}
-
-void CloseFile(base::File file) {
-  // The base::File destructor will close the file for us.
-}
-
 }  // namespace
 
 unsigned NaClProcessHost::keepalive_throttle_interval_milliseconds_ =
     ppapi::kKeepaliveThrottleIntervalDefaultMilliseconds;
 
+// Unfortunately, we cannot use ScopedGeneric directly for IPC::ChannelHandle,
+// because there is neither operator== nor operator!= definition for

[Mark Seaborn, 2015/04/17 22:13:45]

Did you consider defining those operators on IPC::ChannelHandle?  (I'm not sure
what the trade off is here.)

[hidehiko, 2015/04/30 16:33:02]

Defining operator== or operator!= needs to be in the base or IPC namespace,
because of ScopedGeneric implementation, even we have ADL. Our usage is subset
of ChannelHandle now, and our implementation depends on that. So, I prefer not
to use it to avoid making other namespace dirtier.

+// IPC::ChannelHandle. Instead, define a simple wrapper for IPC::ChannelHandle,
+// here.
+class NaClProcessHost::ScopedChannelHandle {
+  MOVE_ONLY_TYPE_FOR_CPP_03(ScopedChannelHandle, RValue);
+ public:
+  ScopedChannelHandle() {
+  }
+  ScopedChannelHandle(const IPC::ChannelHandle& handle) : handle_(handle) {
+  }
+  ScopedChannelHandle(RValue other) : handle_(other.object->handle_) {
+    other.object->handle_ = IPC::ChannelHandle();
+  }
+  ~ScopedChannelHandle() {
+    CloseIfNecessary();
+  }
+
+  const IPC::ChannelHandle& get() const { return handle_; }
+  IPC::ChannelHandle release() WARN_UNUSED_RESULT {
+    IPC::ChannelHandle result = handle_;
+    handle_ = IPC::ChannelHandle();
+    return result;
+  }
+  void reset(const IPC::ChannelHandle& handle = IPC::ChannelHandle()) {
+    // It is not allowed to reset with the same (non-invalid) handle.

[Mark Seaborn, 2015/04/17 22:13:45]

Can you say why?

[hidehiko, 2015/04/30 16:33:02]

Done.

+#if defined(OS_WIN)
+    CHECK(handle.pipe.handle == NULL ||
+          handle.pipe.handle != handle_.pipe.handle);
+#elif defined(OS_POSIX)
+    CHECK(handle.socket.fd == -1 ||
+          !handle.socket.auto_close ||

[Mark Seaborn, 2015/04/17 22:13:45]

Why check auto_close as part of the comparison?

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+          handle.socket.fd != handle_.socket.fd);
+#endif

[Mark Seaborn, 2015/04/17 22:13:45]

"#else
#error Unknown platform"?

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+    CloseIfNecessary();
+    handle_ = handle;
+  }
+
+ private:
+  void CloseIfNecessary() {
+#if defined(OS_WIN)

[Mark Seaborn, 2015/04/17 22:13:45]

How about adding a Close() method to IPC::ChannelHandle so that you don't need
conditionalisation here?

Getting an ipc/ owners review for adding that would actually be quite useful. 
It would be good for an ipc/ owner to look at this change. :-)

[hidehiko, 2015/04/30 16:33:01]

Again, our code depends on some constraint, so having such an API in
ChannelHandle does not make sense.

+    // Defer closing task to the ScopedHandle.
+    base::win::ScopedHandle(handle_.pipe.handle);

[Mark Seaborn, 2015/04/17 22:13:45]

Strangely, this *isn't* going through BlockingPool.

It's kind of odd that you go to the trouble of using BlockingPool in some cases
but not others.  I realise that this is probably an artefact of some Chromium
APIs being (unnecessarily?) stricter than others.

Maybe this warrants a comment to explain the apparent inconsistency?

[hidehiko, 2015/04/30 16:33:02]

Acknowledged.

+#elif defined(OS_POSIX)
+    if (handle_.socket.auto_close) {
+      // Defer closing task to the ScopedFD.
+      base::ScopedFD(handle_.socket.fd);
+    }
+#endif

[Mark Seaborn, 2015/04/17 22:13:45]

Ditto: add an #else #error?

[hidehiko, 2015/04/30 16:33:01]

Acknowledged.

+  }
+
+  IPC::ChannelHandle handle_;
+};
+
 NaClProcessHost::NaClProcessHost(
     const GURL& manifest_url,
     base::File nexe_file,
     const NaClFileToken& nexe_token,
     const std::vector<
       nacl::NaClResourceFileInfo>& prefetched_resource_files_info,
     ppapi::PpapiPermissions permissions,
     int render_view_id,
     uint32 permission_bits,
     bool uses_nonsfi_mode,
@@ skipping 29 matching lines @@
   // for this use case.
   process_->SetName(net::FormatUrl(manifest_url_, std::string()));
 
   enable_debug_stub_ = base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableNaClDebug);
   DCHECK(process_type_ != kUnknownNaClProcessType);
   enable_crash_throttling_ = process_type_ != kNativeNaClProcessType;
 }
 
 NaClProcessHost::~NaClProcessHost() {
+  CloseFile(nexe_file_.Pass());
+
   // Report exit status only if the process was successfully started.
   if (process_->GetData().handle != base::kNullProcessHandle) {
     int exit_code = 0;
     process_->GetTerminationStatus(false /* known_dead */, &exit_code);
     std::string message =
         base::StringPrintf("NaCl process exited with status %i (0x%x)",
                            exit_code, exit_code);
     if (exit_code == 0) {
       VLOG(1) << message;
     } else {
       LOG(ERROR) << message;
     }
     NaClBrowser::GetInstance()->OnProcessEnd(process_->GetData().id);
   }
 
   for (size_t i = 0; i < prefetched_resource_files_info_.size(); ++i) {
     // The process failed to launch for some reason. Close resource file
     // handles.
-    base::File file(IPC::PlatformFileForTransitToFile(
-        prefetched_resource_files_info_[i].file));
-    content::BrowserThread::GetBlockingPool()->PostTask(
-        FROM_HERE,
-        base::Bind(&CloseFile, base::Passed(file.Pass())));
+    CloseFile(IPC::PlatformFileForTransitToFile(
+        prefetched_resource_files_info_[i].file).Pass());
   }
 
   if (reply_msg_) {
     // The process failed to launch for some reason.
     // Don't keep the renderer hanging.
     reply_msg_->set_reply_error();
     nacl_host_message_filter_->Send(reply_msg_);
   }
 #if defined(OS_WIN)
   if (process_launched_by_broker_) {
@@ skipping 356 matching lines @@
 void NaClProcessHost::OnResourcesReady() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   if (!nacl_browser->IsReady()) {
     SendErrorToRenderer("could not acquire shared resources needed by NaCl");
     delete this;
   } else if (!StartNaClExecution()) {
     delete this;
   }
 }
 
-bool NaClProcessHost::ReplyToRenderer(
-    const IPC::ChannelHandle& ppapi_channel_handle,
-    const IPC::ChannelHandle& trusted_channel_handle,
-    const IPC::ChannelHandle& manifest_service_channel_handle) {
+void NaClProcessHost::ReplyToRenderer(
+    ScopedChannelHandle ppapi_channel_handle,
+    ScopedChannelHandle trusted_channel_handle,
+    ScopedChannelHandle manifest_service_channel_handle) {
 #if defined(OS_WIN)
   // If we are on 64-bit Windows, the NaCl process's sandbox is
   // managed by a different process from the renderer's sandbox.  We
   // need to inform the renderer's sandbox about the NaCl process so
   // that the renderer can send handles to the NaCl process using
   // BrokerDuplicateHandle().
   if (RunningOnWOW64()) {
     if (!content::BrokerAddTargetPeer(process_->GetData().handle)) {
       SendErrorToRenderer("BrokerAddTargetPeer() failed");
-      return false;
+      return;
     }
   }
 #endif
 
-  FileDescriptor imc_handle_for_renderer;
-#if defined(OS_WIN)
-  // Copy the handle into the renderer process.
-  HANDLE handle_in_renderer;
-  if (!DuplicateHandle(base::GetCurrentProcessHandle(),
-                       socket_for_renderer_.TakePlatformFile(),
-                       nacl_host_message_filter_->PeerHandle(),
-                       &handle_in_renderer,
-                       0,  // Unused given DUPLICATE_SAME_ACCESS.
-                       FALSE,
-                       DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
-    SendErrorToRenderer("DuplicateHandle() failed");
-    return false;
+  ScopedPlatformFileForTransit imc_handle_for_renderer(
+      IPC::TakeFileHandleForProcess(socket_for_renderer_.Pass(),
+                                    nacl_host_message_filter_->PeerHandle()));
+  if (!imc_handle_for_renderer.is_valid()) {
+    SendErrorToRenderer("TakeFileHandleForProcess() failed");
+    return;
   }
-  imc_handle_for_renderer = reinterpret_cast<FileDescriptor>(
-      handle_in_renderer);
-#else
-  // No need to dup the imc_handle - we don't pass it anywhere else so
-  // it cannot be closed.
-  FileDescriptor imc_handle;
-  imc_handle.fd = socket_for_renderer_.TakePlatformFile();
-  imc_handle.auto_close = true;
-  imc_handle_for_renderer = imc_handle;
-#endif
+
+  ScopedSharedMemoryHandle crash_info_shmem_renderer_handle;

[Mark Seaborn, 2015/04/17 22:13:45]

I think this is unsafe on Windows.

Remember, on Windows, ShareToProcess() gives you a HANDLE that's an index into
another process's handle table.  If you call CloseHandle() on it, potentially
you're closing an unrelated handle in the current process, which might be
exploitable.

Unfortunately, SharedMemory::CloseHandle() doesn't check the return value of
CloseHandle(), which is rather dangerous.

On Windows, I think your options for error handling are:

 * Let the handle leak in the destination process.
 * Use DuplicateHandle() + DUPLICATE_CLOSE_SOURCE to delete the handle from the
destination process's handle table.  Does Chromium actually do this anywhere on
an error-handling code path?  This is somewhat risky given that error code paths
are under-tested.

[hidehiko, 2015/04/30 16:33:02]

You're right. Fixed.

+  {
+    base::SharedMemoryHandle raw_handle = base::SharedMemory::NULLHandle();
+    if (crash_info_shmem_.ShareToProcess(
+            nacl_host_message_filter_->PeerHandle(), &raw_handle)) {
+      crash_info_shmem_renderer_handle.reset(raw_handle);
+    }
+  }
+  if (!crash_info_shmem_renderer_handle.is_valid()) {
+    SendErrorToRenderer("ShareToProcess() failed");
+    return;
+  }
 
   const ChildProcessData& data = process_->GetData();
-  base::SharedMemoryHandle crash_info_shmem_renderer_handle;
-  if (!crash_info_shmem_.ShareToProcess(nacl_host_message_filter_->PeerHandle(),
-                                        &crash_info_shmem_renderer_handle)) {
-    SendErrorToRenderer("ShareToProcess() failed");
-    return false;
+  if (SendMessageToRenderer(
+          NaClLaunchResult(imc_handle_for_renderer.get(),
+                           ppapi_channel_handle.get(),
+                           trusted_channel_handle.get(),
+                           manifest_service_channel_handle.get(),
+                           base::GetProcId(data.handle),
+                           data.id,
+                           crash_info_shmem_renderer_handle.get()),
+          std::string() /* error_message */)) {
+    // On success, the NaClLaunchResult is sent to renderer, so that the
+    // handles have been delegated to the message.
+    ignore_result(imc_handle_for_renderer.release());
+    ignore_result(ppapi_channel_handle.release());
+    ignore_result(trusted_channel_handle.release());
+    ignore_result(manifest_service_channel_handle.release());
+    ignore_result(crash_info_shmem_renderer_handle.release());
   }
 
-  SendMessageToRenderer(
-      NaClLaunchResult(imc_handle_for_renderer,
-                       ppapi_channel_handle,
-                       trusted_channel_handle,
-                       manifest_service_channel_handle,
-                       base::GetProcId(data.handle),
-                       data.id,
-                       crash_info_shmem_renderer_handle),
-      std::string() /* error_message */);
-
   // Now that the crash information shmem handles have been shared with the
   // plugin and the renderer, the browser can close its handle.
   crash_info_shmem_.Close();
-  return true;
 }
 
 void NaClProcessHost::SendErrorToRenderer(const std::string& error_message) {
   LOG(ERROR) << "NaCl process launch failed: " << error_message;
   SendMessageToRenderer(NaClLaunchResult(), error_message);
 }
 
-void NaClProcessHost::SendMessageToRenderer(
+bool NaClProcessHost::SendMessageToRenderer(
     const NaClLaunchResult& result,
     const std::string& error_message) {
   DCHECK(nacl_host_message_filter_.get());
   DCHECK(reply_msg_);
-  if (nacl_host_message_filter_.get() != NULL && reply_msg_ != NULL) {
-    NaClHostMsg_LaunchNaCl::WriteReplyParams(
-        reply_msg_, result, error_message);
-    nacl_host_message_filter_->Send(reply_msg_);
-    nacl_host_message_filter_ = NULL;
-    reply_msg_ = NULL;
+  if (nacl_host_message_filter_.get() == NULL || reply_msg_ == NULL) {
+    return false;
   }
+  NaClHostMsg_LaunchNaCl::WriteReplyParams(reply_msg_, result, error_message);
+  nacl_host_message_filter_->Send(reply_msg_);
+  nacl_host_message_filter_ = NULL;
+  reply_msg_ = NULL;
+  return true;
 }
 
 void NaClProcessHost::SetDebugStubPort(int port) {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   nacl_browser->SetProcessGdbDebugStubPort(process_->GetData().id, port);
 }
 
 #if defined(OS_POSIX)
 // TCP port we chose for NaCl debug stub. It can be any other number.
 static const uint16_t kInitialDebugStubPort = 4014;
@@ skipping 38 matching lines @@
 bool NaClProcessHost::StartNaClExecution() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   NaClStartParams params;
 
   // Enable PPAPI proxy channel creation only for renderer processes.
   params.enable_ipc_proxy = enable_ppapi_proxy();
   params.process_type = process_type_;
   bool enable_nacl_debug = enable_debug_stub_ &&
       NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
+
+  // Because this function may return early on failure, so keep the resources
+  // in local variables, and pass them to params later.
+  ScopedPlatformFileForTransit socket_for_sel_ldr;
+  ScopedPlatformFileForTransit irt_file;
+#if defined(OS_MACOSX)
+  base::ScopedFD memory_fd;
+#endif
+#if defined(OS_POSIX)
+  base::ScopedFD debug_stub_server_bound_socket;
+#endif
+
   if (uses_nonsfi_mode_) {
     // Currently, non-SFI mode is supported only on Linux.
 #if defined(OS_LINUX)
     // In non-SFI mode, we do not use SRPC. Make sure that the socketpair is
     // not created.
     DCHECK(!socket_for_sel_ldr_.IsValid());
 #endif
     if (enable_nacl_debug) {
       base::ProcessId pid = base::GetProcId(process_->GetData().handle);
       LOG(WARNING) << "nonsfi nacl plugin running in " << pid;
     }
   } else {
     params.validation_cache_enabled = nacl_browser->ValidationCacheIsEnabled();
     params.validation_cache_key = nacl_browser->GetValidationCacheKey();
     params.version = NaClBrowser::GetDelegate()->GetVersionString();
     params.enable_debug_stub = enable_nacl_debug;
     params.enable_mojo = base::CommandLine::ForCurrentProcess()->HasSwitch(
         switches::kEnableNaClMojo);
 
     const ChildProcessData& data = process_->GetData();
-    if (!ShareHandleToSelLdr(data.handle,
-                             socket_for_sel_ldr_.TakePlatformFile(),
-                             true,
-                             &params.handles)) {
+    socket_for_sel_ldr.reset(
+        IPC::TakeFileHandleForProcess(socket_for_sel_ldr_.Pass(),
+                                      data.handle));
+    if (!socket_for_sel_ldr.is_valid()) {
       return false;
     }
 
-    const base::File& irt_file = nacl_browser->IrtFile();
-    CHECK(irt_file.IsValid());
+    const base::File& original_irt_file = nacl_browser->IrtFile();
+    CHECK(original_irt_file.IsValid());
     // Send over the IRT file handle.  We don't close our own copy!
-    if (!ShareHandleToSelLdr(data.handle, irt_file.GetPlatformFile(), false,
-                             &params.handles)) {
+    irt_file.reset(IPC::GetFileHandleForProcess(
+        original_irt_file.GetPlatformFile(), data.handle, false));
+    if (!irt_file.is_valid()) {
+      DLOG(ERROR) << "Failed to create irt_file handler for plugin.";
       return false;
     }
 
 #if defined(OS_MACOSX)
     // For dynamic loading support, NaCl requires a file descriptor that
     // was created in /tmp, since those created with shm_open() are not
     // mappable with PROT_EXEC.  Rather than requiring an extra IPC
     // round trip out of the sandbox, we create an FD here.
     base::SharedMemory memory_buffer;
     base::SharedMemoryCreateOptions options;
     options.size = 1;
     options.executable = true;
     if (!memory_buffer.Create(options)) {
       DLOG(ERROR) << "Failed to allocate memory buffer";
       return false;
     }
-    FileDescriptor memory_fd;
-    memory_fd.fd = dup(memory_buffer.handle().fd);
-    if (memory_fd.fd < 0) {
+    memory_fd.reset(dup(memory_buffer.handle().fd));
+    if (!memory_fd.is_valid()) {
       DLOG(ERROR) << "Failed to dup() a file descriptor";
       return false;
     }
-    memory_fd.auto_close = true;
-    params.handles.push_back(memory_fd);
 #endif
 
 #if defined(OS_POSIX)
     if (params.enable_debug_stub) {
       net::SocketDescriptor server_bound_socket = GetDebugStubSocketHandle();
       if (server_bound_socket != net::kInvalidSocket) {
-        params.debug_stub_server_bound_socket =
-            FileDescriptor(server_bound_socket, true);
+        debug_stub_server_bound_socket.reset(server_bound_socket);
       }
     }
 #endif
   }
 
-  if (!crash_info_shmem_.ShareToProcess(process_->GetData().handle,
-                                        &params.crash_info_shmem_handle)) {
+  ScopedSharedMemoryHandle crash_info_shmem_handle;
+  {
+    base::SharedMemoryHandle raw_handle;
+    if (crash_info_shmem_.ShareToProcess(
+            process_->GetData().handle, &raw_handle)) {
+      crash_info_shmem_handle.reset(raw_handle);
+    }
+  }
+  if (!crash_info_shmem_handle.is_valid()) {
     DLOG(ERROR) << "Failed to ShareToProcess() a shared memory buffer";
     return false;
   }
 
-  base::FilePath file_path;
+  // Hereafter, NaClProcessMsg_Start must be (eventually) sent.
+  // Delegate the resources to NaClStartParams.
+  params.crash_info_shmem_handle = crash_info_shmem_handle.release();
   if (uses_nonsfi_mode_) {
+    DCHECK(!socket_for_sel_ldr.is_valid());
+    DCHECK(!irt_file.is_valid());
+#if defined(OS_MACOSX)
+    DCHECK(!memory_fd.is_valid());
+#endif
+
     // Don't retrieve the file path when using nonsfi mode; there's no
     // validation caching in that case, so it's unnecessary work, and would
     // expose the file path to the plugin.
 
     // Pass the pre-opened resource files to the loader. For the same reason
     // as above, use an empty base::FilePath.
     for (size_t i = 0; i < prefetched_resource_files_info_.size(); ++i) {
       params.prefetched_resource_files.push_back(
           NaClResourceFileInfo(prefetched_resource_files_info_[i].file,
                                base::FilePath(),
                                prefetched_resource_files_info_[i].file_key));
     }
     prefetched_resource_files_info_.clear();
   } else {
+    // TODO(yusukes): Handle |prefetched_resource_files_info_| for SFI-NaCl.
+    DCHECK(prefetched_resource_files_info_.empty());
+
+    params.handles.push_back(socket_for_sel_ldr.release());
+    params.handles.push_back(irt_file.release());
+    // Note that on OS_POSIX (including OS_MACOSX), IPC::PlatformFileForTransit
+    // is the alias of base::FileDescriptor.
+#if defined(OS_MACOSX)
+    params.handles.push_back(base::FileDescriptor(memory_fd.release(), true));
+#endif
+#if defined(OS_POSIX)
+    if (debug_stub_server_bound_socket.is_valid()) {
+      params.debug_stub_server_bound_socket =
+          base::FileDescriptor(debug_stub_server_bound_socket.release(), true);
+    }
+#endif
+  }
+
+  if (!uses_nonsfi_mode_) {
+    base::FilePath file_path;
     if (NaClBrowser::GetInstance()->GetFilePath(nexe_token_.lo,
                                                 nexe_token_.hi,
                                                 &file_path)) {
       // We have to reopen the file in the browser process; we don't want a
       // compromised renderer to pass an arbitrary fd that could get loaded
       // into the plugin process.
       if (base::PostTaskAndReplyWithResult(

[hidehiko, 2015/04/17 07:25:32]

Note: this code also has a potential resource leak.
However, fixing this also needs bigger refactoring. In this CL, it is out of
scope.

               content::BrowserThread::GetBlockingPool(),
               FROM_HERE,
               base::Bind(OpenNaClReadExecImpl,
                          file_path,
                          true /* is_executable */),
               base::Bind(&NaClProcessHost::StartNaClFileResolved,
                          weak_factory_.GetWeakPtr(),
                          params,
                          file_path))) {
         return true;
       }
     }
-    // TODO(yusukes): Handle |prefetched_resource_files_info_| for SFI-NaCl.
-    DCHECK(prefetched_resource_files_info_.empty());
   }
 
   params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
                                                    process_->GetData().handle);
   process_->Send(new NaClProcessMsg_Start(params));
   return true;
 }
 
 void NaClProcessHost::StartNaClFileResolved(
     NaClStartParams params,
     const base::FilePath& file_path,
     base::File checked_nexe_file) {
   if (checked_nexe_file.IsValid()) {
-    // Release the file received from the renderer. This has to be done on a
-    // thread where IO is permitted, though.
-    content::BrowserThread::GetBlockingPool()->PostTask(
-        FROM_HERE,
-        base::Bind(&CloseFile, base::Passed(nexe_file_.Pass())));
+    // Release the file received from the renderer.
+    CloseFile(nexe_file_.Pass());
     params.nexe_file_path_metadata = file_path;
     params.nexe_file = IPC::TakeFileHandleForProcess(
         checked_nexe_file.Pass(), process_->GetData().handle);
   } else {
     params.nexe_file = IPC::TakeFileHandleForProcess(
         nexe_file_.Pass(), process_->GetData().handle);
   }
   process_->Send(new NaClProcessMsg_Start(params));
 }
 
 // This method is called when NaClProcessHostMsg_PpapiChannelCreated is
 // received.
 void NaClProcessHost::OnPpapiChannelsCreated(
-    const IPC::ChannelHandle& browser_channel_handle,
-    const IPC::ChannelHandle& ppapi_renderer_channel_handle,
-    const IPC::ChannelHandle& trusted_renderer_channel_handle,
-    const IPC::ChannelHandle& manifest_service_channel_handle) {
+    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(IPC::ChannelHandle(),
-                    trusted_renderer_channel_handle,
-                    manifest_service_channel_handle);
+    ReplyToRenderer(ScopedChannelHandle(),
+                    trusted_renderer_channel_handle.Pass(),
+                    manifest_service_channel_handle.Pass());
     return;
   }
 
   if (!ipc_proxy_channel_.get()) {
     DCHECK_EQ(PROCESS_TYPE_NACL_LOADER, process_->GetData().process_type);
 
     ipc_proxy_channel_ =
-        IPC::ChannelProxy::Create(browser_channel_handle,
+        IPC::ChannelProxy::Create(browser_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(
         ipc_proxy_channel_.get(),  // sender
         permissions_,
         process_->GetData().handle,
         ipc_proxy_channel_.get(),
@@ skipping 24 matching lines @@
 
     ppapi_host_->GetPpapiHost()->AddHostFactoryFilter(
         scoped_ptr<ppapi::host::HostFactory>(
             NaClBrowser::GetDelegate()->CreatePpapiHostFactory(
                 ppapi_host_.get())));
 
     // Send a message to initialize the IPC dispatchers in the NaCl plugin.
     ipc_proxy_channel_->Send(new PpapiMsg_InitializeNaClDispatcher(args));
 
     // Let the renderer know that the IPC channels are established.
-    ReplyToRenderer(ppapi_renderer_channel_handle,
-                    trusted_renderer_channel_handle,
-                    manifest_service_channel_handle);
+    ReplyToRenderer(ppapi_renderer_channel_handle.Pass(),
+                    trusted_renderer_channel_handle.Pass(),
+                    manifest_service_channel_handle.Pass());
   } else {
     // Attempt to open more than 1 browser channel is not supported.
     // Shut down the NaCl process.
     process_->GetHost()->ForceShutdown();
   }
 }
 
 bool NaClProcessHost::StartWithLaunchedProcess() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
@@ skipping 165 matching lines @@
         process.Pass(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl