Reduce resource leak code for StartNaClExecution.

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 55bbaf8301d162afc07e8c9ae41b852b1c4d607b..40173fbe2da738b552fde21a9c25652ccebccf34 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -194,8 +194,102 @@ void SetCloseOnExec(NaClHandle fd) {
 #endif
 }
 
-void CloseFile(base::File file) {
+void PostCloseFile(base::File file) {
+  if (!file.IsValid()) {
+    return;
+  }
+
   // The base::File destructor will close the file for us.
+  content::BrowserThread::GetBlockingPool()->PostTask(
+      FROM_HERE,
+      base::Bind(&ignore_result<base::File>, base::Passed(&file)));
+}
+
+// The maximum number of resource file handles NaClProcessMsg_Start message
+// can have. Currently IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage
+// is 128 and NaCl sends 5 handles for other purposes, hence 123.
+const size_t kMaxPreOpenResourceFiles = 123;
+
+#if defined(OS_POSIX)
+static_assert(kMaxPreOpenResourceFiles ==
+              IPC::MessageAttachmentSet::kMaxDescriptorsPerMessage - 5,
+              "kMaxPreOpenResourceFiles is not up to date");
+#endif
+
+void BatchOpenResourceFiles(
+    const std::vector<NaClResourcePrefetchRequest>& request_list,
+    const base::FilePath& profile_directory,
+    base::ProcessHandle target_process,
+    std::vector<NaClResourcePrefetchResult>* result_list) {
+  if (!result_list) {
+    // If output buffer is null, which means there is no resource to be opened.
+    // Do nothing, then.
+    DCHECK(request_list.empty());
+    return;
+  }
+
+  NaClBrowserDelegate* browser_delegate = NaClBrowser::GetDelegate();
+  for (size_t i = 0;
+       i < request_list.size() &&
+           result_list->size() < kMaxPreOpenResourceFiles;
+       ++i) {
+    base::FilePath file_path;
+    if (!browser_delegate->MapUrlToLocalFilePath(
+            GURL(request_list[i].resource_url),
+            true,  // Use blocking API.
+            profile_directory,
+            &file_path)) {
+      continue;
+    }
+
+    base::File file = OpenNaClReadExecImpl(file_path, true /* executable */);
+    if (!file.IsValid())
+      continue;
+
+    // Note: this runs only in Non-SFI mode. So, do not pass the file path;
+    // there's no validation caching in that case, so it's unnecessary.
+    // Moreover, it would expose the file path to the plugin.
+    result_list->push_back(NaClResourcePrefetchResult(
+        IPC::TakeFileHandleForProcess(file.Pass(), target_process),
+        base::FilePath(),
+        request_list[i].file_key));
+  }
+}
+
+base::File ReopenNexeFile(
+    base::File original_nexe_file, base::FilePath* nexe_file_path) {
+  // If no path is specified, use the original |nexe_file|.
+  if (!nexe_file_path || nexe_file_path->empty())
+    return original_nexe_file.Pass();
+
+  // Reopen the nexe file.
+  base::File reopened_nexe_file = OpenNaClReadExecImpl(
+      *nexe_file_path, true /* executable */);
+  if (!reopened_nexe_file.IsValid()) {
+    // On fail, clear the path, which will eventually passed to the loader.
+    nexe_file_path->clear();
+    return original_nexe_file.Pass();
+  }
+
+  // Note that, the |original_nexe_file| will be closed automatically.
+  return reopened_nexe_file.Pass();
+}
+
+// StartNaClExecution needs file operations. This function takes it, and
+// should run on blocking pool.
+base::File ResolveNaClFile(
+    base::FilePath* nexe_file_path,
+    base::File nexe_file,
+    const std::vector<NaClResourcePrefetchRequest>&
+        resource_prefetch_request_list,
+    const base::FilePath& profile_directory,
+    base::ProcessHandle target_process,
+    std::vector<NaClResourcePrefetchResult>* prefetched_resource_files) {
+  BatchOpenResourceFiles(resource_prefetch_request_list,
+                         profile_directory,
+                         target_process,
+                         prefetched_resource_files);
+  return ReopenNexeFile(nexe_file.Pass(), nexe_file_path);
 }
 
 }  // namespace
@@ -207,7 +301,8 @@ NaClProcessHost::NaClProcessHost(
     const GURL& manifest_url,
     base::File nexe_file,
     const NaClFileToken& nexe_token,
-    const std::vector<NaClResourcePrefetchResult>& prefetched_resource_files,
+    const std::vector<NaClResourcePrefetchRequest>&
+        resource_prefetch_request_list,
     ppapi::PpapiPermissions permissions,
     int render_view_id,
     uint32 permission_bits,
@@ -218,7 +313,7 @@ NaClProcessHost::NaClProcessHost(
     : manifest_url_(manifest_url),
       nexe_file_(nexe_file.Pass()),
       nexe_token_(nexe_token),
-      prefetched_resource_files_(prefetched_resource_files),
+      resource_prefetch_request_list_(resource_prefetch_request_list),
       permissions_(permissions),
 #if defined(OS_WIN)
       process_launched_by_broker_(false),
@@ -251,6 +346,10 @@ NaClProcessHost::NaClProcessHost(
 }
 
 NaClProcessHost::~NaClProcessHost() {
+  PostCloseFile(nexe_file_.Pass());
+  PostCloseFile(socket_for_renderer_.Pass());
+  PostCloseFile(socket_for_sel_ldr_.Pass());
+
   // Report exit status only if the process was successfully started.
   if (process_->GetData().handle != base::kNullProcessHandle) {
     int exit_code = 0;
@@ -266,23 +365,6 @@ NaClProcessHost::~NaClProcessHost() {
     NaClBrowser::GetInstance()->OnProcessEnd(process_->GetData().id);
   }
 
-  // Note: this does not work on Windows, though we currently support this
-  // prefetching feature only on Non-SFI mode, which is supported only on
-  // Linux/ChromeOS, so it should be ok.
-#if defined(OS_WIN)
-  DCHECK(prefetched_resource_files_.empty());
-#else
-  for (size_t i = 0; i < prefetched_resource_files_.size(); ++i) {
-    // The process failed to launch for some reason. Close resource file
-    // handles.
-    base::File file(IPC::PlatformFileForTransitToFile(
-        prefetched_resource_files_[i].file));
-    content::BrowserThread::GetBlockingPool()->PostTask(
-        FROM_HERE,
-        base::Bind(&CloseFile, base::Passed(file.Pass())));
-  }
-#endif
-
   if (reply_msg_) {
     // The process failed to launch for some reason.
     // Don't keep the renderer hanging.
@@ -799,6 +881,13 @@ bool NaClProcessHost::StartNaClExecution() {
   params.process_type = process_type_;
   bool enable_nacl_debug = enable_debug_stub_ &&
       NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
+#if defined(OS_MACOSX)
+  base::ScopedFD memory_fd;
+#endif  // defined(OS_MACOSX)
+#if defined(OS_POSIX)
+  base::ScopedFD debug_stub_server_bound_socket;
+#endif  // defined(OS_POSIX)
+
   if (uses_nonsfi_mode_) {
     // Currently, non-SFI mode is supported only on Linux.
 #if defined(OS_LINUX)
@@ -816,21 +905,7 @@ bool NaClProcessHost::StartNaClExecution() {
     params.version = NaClBrowser::GetDelegate()->GetVersionString();
     params.enable_debug_stub = enable_nacl_debug;
 
-    const ChildProcessData& data = process_->GetData();
-    params.imc_bootstrap_handle =
-        IPC::TakeFileHandleForProcess(socket_for_sel_ldr_.Pass(), data.handle);
-    if (params.imc_bootstrap_handle == IPC::InvalidPlatformFileForTransit()) {
-      return false;
-    }
-
-    const base::File& irt_file = nacl_browser->IrtFile();
-    CHECK(irt_file.IsValid());
-    // Send over the IRT file handle.  We don't close our own copy!
-    params.irt_handle = IPC::GetFileHandleForProcess(
-        irt_file.GetPlatformFile(), data.handle, false);
-    if (params.irt_handle == IPC::InvalidPlatformFileForTransit()) {
-      return false;
-    }
+    CHECK(nacl_browser->IrtFile().IsValid());
 
 #if defined(OS_MACOSX)
     // For dynamic loading support, NaCl requires a file descriptor that
@@ -845,94 +920,124 @@ bool NaClProcessHost::StartNaClExecution() {
       DLOG(ERROR) << "Failed to allocate memory buffer";
       return false;
     }
-    base::ScopedFD memory_fd(dup(memory_buffer.handle().fd));
+    memory_fd.reset(dup(memory_buffer.handle().fd));
     if (!memory_fd.is_valid()) {
       DLOG(ERROR) << "Failed to dup() a file descriptor";
       return false;
     }
-    params.mac_shm_fd = IPC::GetFileHandleForProcess(
-        memory_fd.release(), data.handle, true);
 #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 = IPC::GetFileHandleForProcess(
-            server_bound_socket, data.handle, true);
-      }
-    }
+    if (params.enable_debug_stub)
+      debug_stub_server_bound_socket.reset(GetDebugStubSocketHandle());
 #endif
   }
 
-  if (!crash_info_shmem_.ShareToProcess(process_->GetData().handle,
-                                        &params.crash_info_shmem_handle)) {
-    DLOG(ERROR) << "Failed to ShareToProcess() a shared memory buffer";
-    return false;
+  // Transfer resources to |params|.
+  // Hereafter, we should never return false, and should always send an IPC
+  // to NaCl plugin process. Otherwise, the resources passed to |params| may
+  // be leaked. Note that these are resources for Plugin process, so we
+  // cannot properly release those in this (browser) process.
+  {
+    const ChildProcessData& data = process_->GetData();
+    if (!uses_nonsfi_mode_) {
+      params.imc_bootstrap_handle = IPC::TakeFileHandleForProcess(
+          socket_for_sel_ldr_.Pass(), data.handle);
+      // Send over the IRT file handle.  We don't close our own copy!
+      params.irt_handle = IPC::GetFileHandleForProcess(

[Mark Seaborn, 2015/05/08 00:02:02]

So you're changing the browser process to no longer handle errors if these
various GetFileHandleForProcess()/ShareToProcess() calls fail.

If one of these does fail, what do you expect to happen?  Are you expecting
nacl_listener.cc (in the loader process) to either exit if the FD/handle is
missing or carry on without the FD/handle?

[hidehiko, 2015/05/11 16:52:49]

My intention is "deferring the error handling to the peer (nacl_listener)".
Currently it handles errors with CHECK so that it would be aborted on failure.

[Mark Seaborn, 2015/05/25 19:48:55]

OK.  If you want to say that the NaClProcessMsg_Start message might be
incomplete and that nacl_listener.cc ought to check for that, then I think you
should make that clearer in nacl_listener.cc by putting the checks together,
with a comment.  (Similar to the checks you added earlier in
nonsfi/nonsfi_listener.cc.)


However, I've been thinking about this, and I think that cleaning up the IPC
layer to transfer Windows handles more cleanly might not be too hard.

Could we get the IPC layer to do the DuplicateHandle() calls *after* we do
channel->Send(message)?  This would involve changing:

 * ipc_message_attachment.h: Unconditionalise TakePlatformFile().

 * ipc/ipc_platform_file_attachment_posix.cc: Add a version for Windows handles.

 * ipc_message_attachment_set.h: Add Windows equivalent of
AddDescriptorsToOwn().

 * ipc_message_utils.cc: Add Windows equivalent of
ParamTraits<base::FileDescriptor>::Write() -- perhaps
ParamTraits<base::File>::Write().  We would allow putting base::Files in IPC
messages and have these transferred automatically without the user of the IPC
layer having to call IPC::Get/TakeFileHandleForProcess() or see
IPC::PlatformFileForTransits.

 * ipc_channel_win.cc: Add logic to tack duplicated handles onto a message,
similar to the code that uses msg->attachment_set()->PeekDescriptors() in
ipc_channel_posix.cc.  To start with, we'd just handle the case of sending
messages from the browser process to sandboxed processes.


This would make the error handling easier because all the DuplicateHandle()
calls would be done in one loop.  If one call fails, we can easily free the
remaining handles.

That would be easier to reason about than putting workarounds into users of the
IPC layer (such as nacl_process_host.cc), which is rather fragile because the
error paths don't get tested.

What do you think about that idea?

[hidehiko, 2015/05/26 14:32:59]

Done.
IIUC, this has same problem; if DuplicateHandle() fails, after some
DuplicateHandle() succeeds, in ipc_channel_win, it means some HANDLE is already
opened in the callee process.

I'm happy to continue the discussion if you'd like, but it should be separated
from this CL, IMO, because such a change needs to be applied to all IPC layers.
Probably, it's better to involve IPC experts and Win experts in the discussion.

On the other hand, the goal of this CL is to complete the yusukes@'s remaining
task for pre-FD-open on Win. I prefer making changes step-by-step.

+          nacl_browser->IrtFile().GetPlatformFile(), data.handle, false);
+
+#if defined(OS_MACOSX)
+      params.mac_shm_fd = IPC::GetFileHandleForProcess(
+          memory_fd.release(), data.handle, true);
+#endif
+#if defined(OS_POSIX)
+      params.debug_stub_server_bound_socket = IPC::GetFileHandleForProcess(
+          debug_stub_server_bound_socket.release(), data.handle, true);
+#endif
+    }
+
+    if (!crash_info_shmem_.ShareToProcess(data.handle,
+                                          &params.crash_info_shmem_handle)) {
+      DLOG(ERROR) << "Failed to ShareToProcess() a shared memory buffer";
+      // Do not return here.
+    }
   }
 
-  base::FilePath file_path;
-  if (uses_nonsfi_mode_) {
-    // 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_.size(); ++i) {
-      params.prefetched_resource_files.push_back(NaClResourcePrefetchResult(
-          prefetched_resource_files_[i].file,
-          base::FilePath(),
-          prefetched_resource_files_[i].file_key));
+  // 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.
+  // 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.
+  scoped_ptr<base::FilePath> nexe_file_path;
+  if (!uses_nonsfi_mode_) {
+    nexe_file_path.reset(new base::FilePath);
+    if (!NaClBrowser::GetInstance()->GetFilePath(
+            nexe_token_.lo, nexe_token_.hi, nexe_file_path.get())) {
+      // Failed. Reset the pointer.
+      nexe_file_path.reset();
     }
-    prefetched_resource_files_.clear();
-  } else {
-    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(
-              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;
-      }
+  }
+
+  // Pass the pre-opened resource files to the loader.
+  scoped_ptr<std::vector<NaClResourcePrefetchResult> >
+      prefetched_resource_files;
+  if (!resource_prefetch_request_list_.empty()) {
+    // TODO(yusukes): Handle |resource_prefetch_request_list_| for SFI-NaCl.
+    DCHECK(uses_nonsfi_mode_);
+    prefetched_resource_files.reset(
+        new std::vector<NaClResourcePrefetchResult>);
+  }
+
+  // If file operation is necessary, run it on a blocking pool, where file
+  // operations are allowed.
+  if (nexe_file_path.get() || prefetched_resource_files.get()) {
+    if (base::PostTaskAndReplyWithResult(
+            content::BrowserThread::GetBlockingPool(),
+            FROM_HERE,
+            base::Bind(&ResolveNaClFile,
+                       nexe_file_path.get(),
+                       base::Passed(&nexe_file_),
+                       resource_prefetch_request_list_,
+                       profile_directory_,
+                       process_->GetData().handle,
+                       prefetched_resource_files.get()),
+            base::Bind(&NaClProcessHost::StartNaClExecutionAfterFileResolved,
+                       weak_factory_.GetWeakPtr(),
+                       params,
+                       base::Passed(&nexe_file_path),
+                       base::Passed(&prefetched_resource_files)))) {
+      return true;
     }
-    // TODO(yusukes): Handle |prefetched_resource_files_| for SFI-NaCl.
-    DCHECK(prefetched_resource_files_.empty());
   }
 
-  params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
-                                                   process_->GetData().handle);
-  process_->Send(new NaClProcessMsg_Start(params));
+  StartNaClExecutionAfterFileResolved(
+      params,
+      scoped_ptr<base::FilePath>(),
+      scoped_ptr<std::vector<NaClResourcePrefetchResult> >(),
+      nexe_file_.Pass());
   return true;
 }
 
-void NaClProcessHost::StartNaClFileResolved(
+void NaClProcessHost::StartNaClExecutionAfterFileResolved(
     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())));
-    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);
+    scoped_ptr<base::FilePath> nexe_file_path,
+    scoped_ptr<std::vector<NaClResourcePrefetchResult> >
+        prefetched_resource_files,
+    base::File nexe_file) {
+  // Pass the nexe file and its path to params.
+  params.nexe_file = IPC::TakeFileHandleForProcess(
+      nexe_file.Pass(), process_->GetData().handle);
+  if (nexe_file_path.get()) {
+    params.nexe_file_path_metadata = *nexe_file_path;
+  }
+
+  // Pass prefetched resources to params.
+  if (prefetched_resource_files.get()) {
+    prefetched_resource_files->swap(params.prefetched_resource_files);
   }
+
   process_->Send(new NaClProcessMsg_Start(params));
 }