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 5634c6f97c4c33c3ae17fb985bb6bbe8aba8b512..f040d6279b6773f12f2ad428bc2c9e47f93160ec 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -194,8 +194,98 @@ 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 the browser process accepts. Use
+// 200 because ARC's nmf has ~128 resource files as of May 2015. This prevents
+// untrusted code filling the FD/handle table.
+const size_t kMaxPreOpenResourceFiles = 200;
+
+void BatchOpenResourceFiles(
+    const std::vector<NaClResourcePrefetchRequest>& request_list,
+    const base::FilePath& profile_directory,
+    base::ProcessHandle target_process,
+    bool uses_nonsfi_mode,
+    std::vector<NaClResourcePrefetchResult>* result_list) {
+  if (!result_list) {
+    // If output buffer is null, it 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;
+
+    // There's no validation caching in Non-SFI mode, so |file_path_metadata|
+    // is unnecessary, and would expose the file path to the plugin.
+    result_list->push_back(NaClResourcePrefetchResult(
+        IPC::TakeFileHandleForProcess(file.Pass(), target_process),
+        uses_nonsfi_mode ? base::FilePath() : file_path,
+        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,
+    bool uses_nonsfi_mode,
+    std::vector<NaClResourcePrefetchResult>* prefetched_resource_files) {
+  BatchOpenResourceFiles(resource_prefetch_request_list,
+                         profile_directory,
+                         target_process,
+                         uses_nonsfi_mode,
+                         prefetched_resource_files);
+  return ReopenNexeFile(nexe_file.Pass(), nexe_file_path);
 }
 
 }  // namespace
@@ -273,7 +363,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,
@@ -284,7 +375,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),
@@ -317,6 +408,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;
@@ -332,22 +427,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 POSIX platforms, 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.
@@ -865,6 +944,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)
@@ -882,21 +968,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
@@ -911,94 +983,117 @@ 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(
+          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.
+    }
   }
 
-  // Pass the pre-opened resource files to the loader. We do not have to reopen
-  // resource files here even for SFI mode because the descriptors are not from
-  // a renderer.
-  for (size_t i = 0; i < prefetched_resource_files_.size(); ++i) {
-    process_->Send(new NaClProcessMsg_AddPrefetchedResource(
-        NaClResourcePrefetchResult(
-            prefetched_resource_files_[i].file,
-            // For the same reason as the comment below, always use an empty
-            // base::FilePath for non-SFI mode.
-            (uses_nonsfi_mode_ ? base::FilePath() :
-             prefetched_resource_files_[i].file_path_metadata),
-            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();
 
-  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.
-  } 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()) {
+    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,
+                       uses_nonsfi_mode_,
+                       prefetched_resource_files.get()),
+            base::Bind(&NaClProcessHost::StartNaClExecutionAfterFileResolved,
+                       weak_factory_.GetWeakPtr(),
+                       params,
+                       base::Passed(&nexe_file_path),
+                       base::Passed(&prefetched_resource_files)))) {
+      return true;
     }
   }
 
-  StartNaClFileResolved(params, base::FilePath(), base::File());
+  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 pre-opened resource files to the loader.
+  if (prefetched_resource_files.get()) {
+    for (size_t i = 0; i < prefetched_resource_files->size(); ++i) {
+      process_->Send(new NaClProcessMsg_AddPrefetchedResource(
+          (*prefetched_resource_files)[i]));
+    }
   }
 
 #if defined(OS_LINUX)
@@ -1063,6 +1158,13 @@ void NaClProcessHost::StartNaClFileResolved(
   }
 #endif
 
+  // 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;
+  }
+
   process_->Send(new NaClProcessMsg_Start(params));
 }