NaCl: Simpler validation for main nexe.

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 3b6ddcf7050d0caa0f59588b5e4854146e7b4984..a097b9aa27e0d0c14bfd59a0c5c249e3b83b0403 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -230,6 +230,10 @@ bool ShareHandleToSelLdr(
   return true;
 }
 
+void CloseFile(base::File file) {
+  // The base::File destructor will close the file for us.
+}
+
 }  // namespace
 
 unsigned NaClProcessHost::keepalive_throttle_interval_milliseconds_ =
@@ -632,10 +636,8 @@ bool NaClProcessHost::OnMessageReceived(const IPC::Message& msg) {
                           OnQueryKnownToValidate)
       IPC_MESSAGE_HANDLER(NaClProcessMsg_SetKnownToValidate,
                           OnSetKnownToValidate)
-      IPC_MESSAGE_HANDLER_DELAY_REPLY(NaClProcessMsg_ResolveFileToken,
-                                      OnResolveFileToken)
-      IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileTokenAsync,
-                          OnResolveFileTokenAsync)
+      IPC_MESSAGE_HANDLER(NaClProcessMsg_ResolveFileToken,
+                          OnResolveFileToken)
 
 #if defined(OS_WIN)
       IPC_MESSAGE_HANDLER_DELAY_REPLY(
@@ -822,11 +824,6 @@ bool NaClProcessHost::StartNaClExecution() {
     params.enable_debug_stub = enable_debug_stub_ &&
         NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
 
-    // TODO(teravest): Resolve the file tokens right now instead of making the
-    // loader send IPC to resolve them later.
-    params.nexe_token_lo = nexe_token_.lo;
-    params.nexe_token_hi = nexe_token_.hi;
-
     const ChildProcessData& data = process_->GetData();
     if (!ShareHandleToSelLdr(data.handle,
                              socket_for_sel_ldr_.TakePlatformFile(),
@@ -881,18 +878,59 @@ bool NaClProcessHost::StartNaClExecution() {
 #endif
   }
 
-  params.nexe_file = IPC::TakeFileHandleForProcess(nexe_file_.Pass(),
-                                                   process_->GetData().handle);
   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;
+  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;
+    }
+  }
+
+  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 nexe_file) {

[Mark Seaborn, 2014/10/16 18:01:01]

How about renaming this to "checked_nexe_file" or something similar?

Otherwise you have both "nexe_file" (safe) and "nexe_file_" (unsafe) in scope. 
It would be easy to make a mistake with that.

[teravest, 2014/10/16 21:40:40]

Done.

+  if (nexe_file.IsValid()) {
+    // Release the file received from the renderer. This has to be done on a

[Mark Seaborn, 2014/10/16 18:01:01]

Does this mean that StartNaClExecution() and StartNaClFileResolved() are running
in different threads?  Wouldn't that mean that multiple threads are accessing
nexe_file_?  That doesn't sound like it would be thread-safe.

[teravest, 2014/10/16 21:40:40]

StartNaClExecution() and StartNaClFileResolved() are called on the same thread. 

[Mark Seaborn, 2014/10/16 21:58:40]

Let me check I understand this:

StartNaClExecution() is called in the IO thread (in response to incoming IPCs).

StartNaClExecution() doesn't need to close a handle (so it doesn't need to
interact with the blocking-IO-thread).

Earlier we do:
PostTaskAndReplyWithResult(..., bind(OpenNaClReadExecImpl),
bind(StartNaClFileResolved))
-- This calls OpenNaClReadExecImpl on a blocking-file-IO thread, but it calls
StartNaClFileResolved on the IO thread.  (I hadn't realised about the latter
part before.)

+    // 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(
+        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(
@@ -996,8 +1034,7 @@ void NaClProcessHost::OnSetKnownToValidate(const std::string& signature) {
 }
 
 void NaClProcessHost::OnResolveFileToken(uint64 file_token_lo,
-                                         uint64 file_token_hi,
-                                         IPC::Message* reply_msg) {
+                                         uint64 file_token_hi) {
   // Was the file registered?
   //
   // Note that the file path cache is of bounded size, and old entries can get
@@ -1018,47 +1055,11 @@ void NaClProcessHost::OnResolveFileToken(uint64 file_token_lo,
   // nexe are currently not resolved.  Shared libraries will be resolved.  They
   // will be loaded sequentially, so they will only consume a single entry
   // while the load is in flight.
-  //
-  // TODO(ncbray): track behavior with UMA. If entries are getting evicted or

[Mark Seaborn, 2014/10/16 18:01:00]

Why remove this comment?  AFAIK this still applies.

[teravest, 2014/10/16 21:40:40]

Added this back.

-  // bogus keys are getting queried, this would be good to know.
-  CHECK(!uses_nonsfi_mode_);
-  base::FilePath file_path;
-  if (!NaClBrowser::GetInstance()->GetFilePath(
-        file_token_lo, file_token_hi, &file_path)) {
-    NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-        reply_msg,
-        IPC::InvalidPlatformFileForTransit(),
-        base::FilePath());
-    Send(reply_msg);
-    return;
-  }
-
-  // Open the file.
-  if (!base::PostTaskAndReplyWithResult(
-          content::BrowserThread::GetBlockingPool(),
-          FROM_HERE,
-          base::Bind(OpenNaClReadExecImpl, file_path, true /* is_executable */),
-          base::Bind(&NaClProcessHost::FileResolved,
-                     weak_factory_.GetWeakPtr(),
-                     file_path,
-                     reply_msg))) {
-     NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-         reply_msg,
-         IPC::InvalidPlatformFileForTransit(),
-         base::FilePath());
-     Send(reply_msg);
-  }
-}
-
-void NaClProcessHost::OnResolveFileTokenAsync(uint64 file_token_lo,
-                                              uint64 file_token_hi) {
-  // See the comment at OnResolveFileToken() for details of the file path cache
-  // behavior.
   CHECK(!uses_nonsfi_mode_);
   base::FilePath file_path;
   if (!NaClBrowser::GetInstance()->GetFilePath(
         file_token_lo, file_token_hi, &file_path)) {
-    Send(new NaClProcessMsg_ResolveFileTokenAsyncReply(
+    Send(new NaClProcessMsg_ResolveFileTokenReply(
              file_token_lo,
              file_token_hi,
              IPC::PlatformFileForTransit(),
@@ -1071,12 +1072,12 @@ void NaClProcessHost::OnResolveFileTokenAsync(uint64 file_token_lo,
           content::BrowserThread::GetBlockingPool(),
           FROM_HERE,
           base::Bind(OpenNaClReadExecImpl, file_path, true /* is_executable */),
-          base::Bind(&NaClProcessHost::FileResolvedAsync,
+          base::Bind(&NaClProcessHost::FileResolved,
                      weak_factory_.GetWeakPtr(),
                      file_token_lo,
                      file_token_hi,
                      file_path))) {
-    Send(new NaClProcessMsg_ResolveFileTokenAsyncReply(
+    Send(new NaClProcessMsg_ResolveFileTokenReply(
             file_token_lo,
             file_token_hi,
             IPC::PlatformFileForTransit(),
@@ -1085,27 +1086,6 @@ void NaClProcessHost::OnResolveFileTokenAsync(uint64 file_token_lo,
 }
 
 void NaClProcessHost::FileResolved(
-    const base::FilePath& file_path,
-    IPC::Message* reply_msg,
-    base::File file) {
-  if (file.IsValid()) {
-    IPC::PlatformFileForTransit handle = IPC::TakeFileHandleForProcess(
-        file.Pass(),
-        process_->GetData().handle);
-    NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-        reply_msg,
-        handle,
-        file_path);
-  } else {
-    NaClProcessMsg_ResolveFileToken::WriteReplyParams(
-        reply_msg,
-        IPC::InvalidPlatformFileForTransit(),
-        base::FilePath());
-  }
-  Send(reply_msg);
-}
-
-void NaClProcessHost::FileResolvedAsync(
     uint64_t file_token_lo,
     uint64_t file_token_hi,
     const base::FilePath& file_path,
@@ -1120,7 +1100,7 @@ void NaClProcessHost::FileResolvedAsync(
   } else {
     out_handle = IPC::InvalidPlatformFileForTransit();
   }
-  Send(new NaClProcessMsg_ResolveFileTokenAsyncReply(
+  Send(new NaClProcessMsg_ResolveFileTokenReply(
            file_token_lo,
            file_token_hi,
            out_handle,