Pepper: Remove LOAD_MODULE SRPC call in SFI mode.

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 232 matching lines @@
       socket_for_sel_ldr(NACL_INVALID_HANDLE) { }
 };
 
 // -----------------------------------------------------------------------------
 
 unsigned NaClProcessHost::keepalive_throttle_interval_milliseconds_ =
     ppapi::kKeepaliveThrottleIntervalDefaultMilliseconds;
 
 NaClProcessHost::NaClProcessHost(const GURL& manifest_url,
                                  base::File nexe_file,
+                                 NaClFileToken nexe_token,
                                  ppapi::PpapiPermissions permissions,
                                  int render_view_id,
                                  uint32 permission_bits,
                                  bool uses_irt,
                                  bool uses_nonsfi_mode,
                                  bool enable_dyncode_syscalls,
                                  bool enable_exception_handling,
                                  bool enable_crash_throttling,
                                  bool off_the_record,
                                  const base::FilePath& profile_directory)
     : manifest_url_(manifest_url),
       nexe_file_(nexe_file.Pass()),
+      nexe_token_(nexe_token),
       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
       internal_(new NaClInternal()),
       weak_factory_(this),
@@ skipping 535 matching lines @@
 }
 #endif
 
 bool NaClProcessHost::StartNaClExecution() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   NaClStartParams params;
 
   // Enable PPAPI proxy channel creation only for renderer processes.
   params.enable_ipc_proxy = enable_ppapi_proxy();
+  // nexe_file_ still keeps the ownership at this moment, because |params|
+  // may just be destroyed before sending IPC is properly processed.
+  // Note that although we set auto_close=true for FileDescriptor's
+  // constructor, it is not automatically handled in its destructor as RAII.
+#if defined(OS_POSIX)
+  params.nexe_file =
+      base::FileDescriptor(nexe_file_.TakePlatformFile(), false);

[Mark Seaborn, 2014/07/08 18:46:00]

This leaks the FD.  You should either do:

// takes ownership of FD
// -- would leak on an early return
base::FileDescriptor(nexe_file_.TakePlatformFile(), true)

or

// borrowed reference to FD
// -- wouldn't leak on an early return
base::FileDescriptor(nexe_file_.GetPlatformFile(), false)

[teravest, 2014/07/08 19:03:45]

Yes, you're right. I wanted GetPlatformFile() here.

+#elif defined(OS_WIN)
+  params.nexe_file = IPC::GetFileHandleForProcess(nexe_file_.TakePlatformFile(),

[Mark Seaborn, 2014/07/08 18:46:00]

This has a similar leak.

[teravest, 2014/07/08 19:03:45]

Done.

+                                                  process_->GetData().handle,
+                                                  false);
+#else
+#error Unsupported target platform.
+#endif
+
   if (uses_nonsfi_mode_) {
     // Currently, non-SFI mode is supported only on Linux.
 #if defined(OS_LINUX)
-    // nexe_file_ still keeps the ownership at this moment, because |params|
-    // may just be destroyed before sending IPC is properly processed.
-    // Note that although we set auto_close=true for FileDescriptor's
-    // constructor, it is not automatically handled in its destructor as RAII.
-    params.nexe_file =
-        base::FileDescriptor(nexe_file_.GetPlatformFile(), true);
     // In non-SFI mode, we do not use SRPC. Make sure that the socketpair is
     // not created.
     DCHECK_EQ(internal_->socket_for_sel_ldr, NACL_INVALID_HANDLE);
 #endif
   } else {
     params.validation_cache_enabled = nacl_browser->ValidationCacheIsEnabled();
     params.validation_cache_key = nacl_browser->GetValidationCacheKey();
     params.version = NaClBrowser::GetDelegate()->GetVersionString();
     params.enable_exception_handling = enable_exception_handling_;
     params.enable_debug_stub = enable_debug_stub_ &&
         NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
     params.uses_irt = uses_irt_;
     params.enable_dyncode_syscalls = enable_dyncode_syscalls_;
 
+    // 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,
                              internal_->socket_for_sel_ldr, true,
                              &params.handles)) {
       return false;
     }
 
     if (params.uses_irt) {
       const base::File& irt_file = nacl_browser->IrtFile();
       CHECK(irt_file.IsValid());
@@ skipping 31 matching lines @@
     if (params.enable_debug_stub) {
       net::SocketDescriptor server_bound_socket = GetDebugStubSocketHandle();
       if (server_bound_socket != net::kInvalidSocket) {
         params.debug_stub_server_bound_socket =
             FileDescriptor(server_bound_socket, true);
       }
     }
 #endif
   }
 
-  // Here we are about to send the IPC, so release file descriptors to delegate
-  // the ownership to the message.
-  if (uses_nonsfi_mode_) {
-    nexe_file_.TakePlatformFile();
-  } else {
+  // Here we are about to send the IPC, so close the file descriptor in
+  // nexe_file_.
+  nexe_file_.Close();

[Mark Seaborn, 2014/07/08 18:46:00]

This has no effect, because the code did TakePlatformFile() on nexe_file_
earlier, so nexe_file_ contains no descriptor/handle.

[teravest, 2014/07/08 19:03:45]

Done.

+  if (!uses_nonsfi_mode_) {
     internal_->socket_for_sel_ldr = NACL_INVALID_HANDLE;
   }
 
   process_->Send(new NaClProcessMsg_Start(params));
   return true;
 }
 
 // This method is called when NaClProcessHostMsg_PpapiChannelCreated is
 // received.
 void NaClProcessHost::OnPpapiChannelsCreated(
@@ skipping 237 matching lines @@
         process_handle.Take(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl