Non-SFI NaCl: Batch-open resource files

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>
 
@@ skipping 217 matching lines @@
 
 void CloseFile(base::File file) {
   // The base::File destructor will close the file for us.
 }
 
 }  // namespace
 
 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),
 #endif
       reply_msg_(NULL),
 #if defined(OS_WIN)
       debug_exception_handler_requested_(false),
 #endif
       uses_nonsfi_mode_(uses_nonsfi_mode),
       enable_debug_stub_(false),
@@ skipping 618 matching lines @@
 #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;
   }
 
   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;
 }
 
 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
@@ skipping 255 matching lines @@
         process.Pass(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl