Non-SFI NaCl: Batch-open resource files

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 e273204194852c306dbadd64996e59d12ffdbd7c..d7e2b8983b7fa9f0a2c0f9bb504db124981a01c0 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -235,19 +235,22 @@ void CloseFile(base::File file) {
 unsigned NaClProcessHost::keepalive_throttle_interval_milliseconds_ =
     ppapi::kKeepaliveThrottleIntervalDefaultMilliseconds;
 
-NaClProcessHost::NaClProcessHost(const GURL& manifest_url,
-                                 base::File nexe_file,
-                                 const NaClFileToken& nexe_token,
-                                 ppapi::PpapiPermissions permissions,
-                                 int render_view_id,
-                                 uint32 permission_bits,
-                                 bool uses_nonsfi_mode,
-                                 bool off_the_record,
-                                 NaClAppProcessType process_type,
-                                 const base::FilePath& profile_directory)
+NaClProcessHost::NaClProcessHost(
+    const GURL& manifest_url,
+    base::File nexe_file,
+    const NaClFileToken& nexe_token,
+    const std::vector<NaClResourceFileInfo>& resource_files_info,

[Mark Seaborn, 2015/02/12 03:57:33]

I walked through the IPC flow to refresh my memory about how this
works.  Currently this happens, in order:

 * renderer:
    * DownloadNexe() does a sync IPC to the browser
      (NaClHostMsg_OpenNaClResources) to fetch files from the manifest
      file, and receives FDs back (+ NaClFileTokens).
    * LaunchSelLdr() sends the startup message to the browser,
      including FDs (+ NaClFileTokens).

 * browser: Forwards the startup message to the NaCl loader process.
   This includes FDs.
    * Under SFI NaCl, it would ignore the FDs it got from the renderer
      and use the NaClFileTokens to retrieve known-good FDs from a
      stash (path_cache_ in nacl_browser.cc).
    * Under Non-SFI NaCl, it would ignore the NaClFileTokens and just
      use the FDs.

Why are we sending the FDs from the browser to the renderer and back
again?

Instead, how about this:

 * The renderer's startup message, sent to the browser, can contain a
   list of <manifest_key, url> pairs.
 * The browser can map the URLs to filenames, open the files, and send
   a list of <manifest_key, FD, file_pathname> tuples to the NaCl
   loader process in the startup message.

Would that work OK?  I think it would simplify this change a lot.

I'm sorry I didn't notice this before.  It's a case of not seeing the
wood for the trees.  It took me a while to understand what's happening
in this change.

[Yusuke Sato, 2015/02/13 23:01:16]

I didn't do this originally mainly because I wanted to reuse the security checks
(in renderer and browser) for open_resource as-is, but yes, the approach is
probably too complicated. I've implemented yours with security checks. Done.

BTW, opening a main nexe binary with a separate IPC before sending the
LaunchNaCl IPC is also unnecessary then (at least when the nexe's scheme is
chrome-extension://)?  If you think it's possible to remove the IPC for faster
and even simpler NaCl process creation, I'll file a Chrome bug.

+    ppapi::PpapiPermissions permissions,
+    int render_view_id,
+    uint32 permission_bits,
+    bool uses_nonsfi_mode,
+    bool off_the_record,
+    NaClAppProcessType process_type,
+    const base::FilePath& profile_directory)
     : manifest_url_(manifest_url),
       nexe_file_(nexe_file.Pass()),
       nexe_token_(nexe_token),
+      resource_files_info_(resource_files_info),
       permissions_(permissions),
 #if defined(OS_WIN)
       process_launched_by_broker_(false),
@@ -886,32 +889,43 @@ bool NaClProcessHost::StartNaClExecution() {
   }
 
   base::FilePath file_path;
-  // 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.
-  if (!uses_nonsfi_mode_ &&
-      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;
+  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.
+    for (size_t i = 0; i < resource_files_info_.size(); ++i) {
+      params.resource_files.push_back(
+          NaClStartParams::ResourceFileInfo(resource_files_info_[i].file,
+                                            base::FilePath(),

[Mark Seaborn, 2015/02/12 03:57:33]

See comment in nonsfi_listener.cc -- you can add a sanity check for this empty
filename there.

[Yusuke Sato, 2015/02/13 23:01:16]

Done.

+                                            resource_files_info_[i].key));
+    }
+  } 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;
+      }
     }
+    // TODO(yusukes): Handle |resource_files_info_| for SFI-NaCl.
   }
 
   params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
                                                    process_->GetData().handle);
+
+  params.CheckNumOfDescriptors();
   process_->Send(new NaClProcessMsg_Start(params));
   return true;
 }