Refactor NaClProcessHost. Reduce chances to leak the resource.

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 185 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;
 
+// Unfortunately, we cannot use ScopedGeneric directly for IPC::ChannelHandle,
+// because there is neither operator== nor operator != definition for it.
+// Instead, define a simple wrapper for IPC::ChannelHandle with an assumption
+// that this only takes a transferred IPC::ChannelHandle or one to be
+// transferred via IPC.
+class NaClProcessHost::ScopedChannelHandle {
+  MOVE_ONLY_TYPE_FOR_CPP_03(ScopedChannelHandle, RValue);
+ public:
+  ScopedChannelHandle() {
+  }
+  explicit ScopedChannelHandle(const IPC::ChannelHandle& handle)
+      : handle_(handle) {
+    DCHECK(IsSupportedHandle(handle_));
+  }
+  ScopedChannelHandle(RValue other) : handle_(other.object->handle_) {
+    other.object->handle_ = IPC::ChannelHandle();
+    DCHECK(IsSupportedHandle(handle_));
+  }
+  ~ScopedChannelHandle() {
+    CloseIfNecessary();
+  }
+
+  const IPC::ChannelHandle& get() const { return handle_; }
+  IPC::ChannelHandle release() WARN_UNUSED_RESULT {
+    IPC::ChannelHandle result = handle_;
+    handle_ = IPC::ChannelHandle();
+    return result;
+  }
+
+  void reset(const IPC::ChannelHandle& handle = IPC::ChannelHandle()) {
+    DCHECK(IsSupportedHandle(handle));
+#if defined(OS_POSIX)
+    // Following the manner of base::ScopedGeneric, we do not support
+    // reset() with same handle for simplicity of the implementation.
+    CHECK(handle.socket.fd == -1 || handle.socket.fd != handle_.socket.fd);
+#endif
+    CloseIfNecessary();
+    handle_ = handle;
+  }
+
+ private:
+  // Returns true if the given handle is closable automatically by this
+  // class. This function is just a helper for the validation.

[Mark Seaborn, 2015/05/06 21:24:52]

Nit: "for validation"?  (no "the")

[hidehiko, 2015/05/07 01:29:06]

Done.

+  static bool IsSupportedHandle(const IPC::ChannelHandle& handle) {
+#if defined(OS_WIN)
+    // On Windows, it is not supported to marshal the |pipe.handle|.
+    // In our case, we wrap a transferred ChannelHandle (or one to be
+    // transferred) via IPC, so we can assume |handle.pipe.handle| is NULL.
+    return handle.pipe.handle == NULL;
+#else
+    return true;
+#endif
+  }
+
+  void CloseIfNecessary() {
+#if defined(OS_POSIX)
+    if (handle_.socket.auto_close) {
+      // Defer closing task to the ScopedFD.
+      base::ScopedFD(handle_.socket.fd);
+    }
+#endif
+  }
+
+  IPC::ChannelHandle handle_;
+};
+
 NaClProcessHost::NaClProcessHost(
     const GURL& manifest_url,
     base::File nexe_file,
     const NaClFileToken& nexe_token,
     const std::vector<NaClResourcePrefetchResult>& prefetched_resource_files,
     ppapi::PpapiPermissions permissions,
     int render_view_id,
     uint32 permission_bits,
     bool uses_nonsfi_mode,
     bool off_the_record,
@@ skipping 434 matching lines @@
 void NaClProcessHost::OnResourcesReady() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   if (!nacl_browser->IsReady()) {
     SendErrorToRenderer("could not acquire shared resources needed by NaCl");
     delete this;
   } else if (!StartNaClExecution()) {
     delete this;
   }
 }
 
-bool NaClProcessHost::ReplyToRenderer(
-    const IPC::ChannelHandle& ppapi_channel_handle,
-    const IPC::ChannelHandle& trusted_channel_handle,
-    const IPC::ChannelHandle& manifest_service_channel_handle) {
+void NaClProcessHost::ReplyToRenderer(
+    ScopedChannelHandle ppapi_channel_handle,
+    ScopedChannelHandle trusted_channel_handle,
+    ScopedChannelHandle manifest_service_channel_handle) {
 #if defined(OS_WIN)
   // If we are on 64-bit Windows, the NaCl process's sandbox is
   // managed by a different process from the renderer's sandbox.  We
   // need to inform the renderer's sandbox about the NaCl process so
   // that the renderer can send handles to the NaCl process using
   // BrokerDuplicateHandle().
   if (RunningOnWOW64()) {
     if (!content::BrokerAddTargetPeer(process_->GetData().handle)) {
       SendErrorToRenderer("BrokerAddTargetPeer() failed");
-      return false;
+      return;
     }
   }
 #endif
 
-  FileDescriptor imc_handle_for_renderer;
-#if defined(OS_WIN)
-  // Copy the handle into the renderer process.
-  HANDLE handle_in_renderer;
-  if (!DuplicateHandle(base::GetCurrentProcessHandle(),
-                       socket_for_renderer_.TakePlatformFile(),
-                       nacl_host_message_filter_->PeerHandle(),
-                       &handle_in_renderer,
-                       0,  // Unused given DUPLICATE_SAME_ACCESS.
-                       FALSE,
-                       DUPLICATE_CLOSE_SOURCE | DUPLICATE_SAME_ACCESS)) {
-    SendErrorToRenderer("DuplicateHandle() failed");
-    return false;
-  }
-  imc_handle_for_renderer = reinterpret_cast<FileDescriptor>(
-      handle_in_renderer);
-#else
-  // No need to dup the imc_handle - we don't pass it anywhere else so
-  // it cannot be closed.
-  FileDescriptor imc_handle;
-  imc_handle.fd = socket_for_renderer_.TakePlatformFile();
-  imc_handle.auto_close = true;
-  imc_handle_for_renderer = imc_handle;
-#endif
+  // Hereafter, always we send an IPC message with handles which are not

[Mark Seaborn, 2015/05/06 21:24:52]

Nit: "we always"

[hidehiko, 2015/05/07 01:29:06]

Done.

+  // closable in this process.

[Mark Seaborn, 2015/05/06 21:24:53]

Nit: Clarify that the unclosability applies only to Windows: e.g. "...which, on
Windows, are not closable in this process"

[hidehiko, 2015/05/07 01:29:06]

Done.

+  IPC::PlatformFileForTransit imc_handle_for_renderer =
+      IPC::TakeFileHandleForProcess(socket_for_renderer_.Pass(),
+                                    nacl_host_message_filter_->PeerHandle());
 
-  const ChildProcessData& data = process_->GetData();
+  std::string error_message;
   base::SharedMemoryHandle crash_info_shmem_renderer_handle;
   if (!crash_info_shmem_.ShareToProcess(nacl_host_message_filter_->PeerHandle(),
                                         &crash_info_shmem_renderer_handle)) {
-    SendErrorToRenderer("ShareToProcess() failed");
-    return false;
+    // On error, we do not send "IPC::ChannelHandle"s to the renderer process.

[Mark Seaborn, 2015/05/06 21:24:52]

Can you also add:

"Some other FDs/handles still get sent to the renderer, but will be closed
there."

since that isn't obvious without looking at ppb_nacl_private_impl.cc.

[hidehiko, 2015/05/07 01:29:06]

Done.

+    ppapi_channel_handle.reset();
+    trusted_channel_handle.reset();
+    manifest_service_channel_handle.reset();
+    error_message = "ShareToProcess() failed";
   }
 
+  const ChildProcessData& data = process_->GetData();

[Mark Seaborn, 2015/05/06 21:24:52]

Nit: if you're moving this, you could just use "process_->GetData().id" below.

[hidehiko, 2015/05/07 01:29:06]

Pls let me keep it as is. |data| is also used for base::GetProcId(data.handle),
too.

   SendMessageToRenderer(
       NaClLaunchResult(imc_handle_for_renderer,
-                       ppapi_channel_handle,
-                       trusted_channel_handle,
-                       manifest_service_channel_handle,
+                       ppapi_channel_handle.release(),
+                       trusted_channel_handle.release(),
+                       manifest_service_channel_handle.release(),
                        base::GetProcId(data.handle),
                        data.id,
                        crash_info_shmem_renderer_handle),
-      std::string() /* error_message */);
+      error_message);
 
   // Now that the crash information shmem handles have been shared with the
   // plugin and the renderer, the browser can close its handle.
   crash_info_shmem_.Close();
-  return true;
 }
 
 void NaClProcessHost::SendErrorToRenderer(const std::string& error_message) {
   LOG(ERROR) << "NaCl process launch failed: " << error_message;
   SendMessageToRenderer(NaClLaunchResult(), error_message);
 }
 
 void NaClProcessHost::SendMessageToRenderer(
     const NaClLaunchResult& result,
     const std::string& error_message) {
   DCHECK(nacl_host_message_filter_.get());
   DCHECK(reply_msg_);
-  if (nacl_host_message_filter_.get() != NULL && reply_msg_ != NULL) {
-    NaClHostMsg_LaunchNaCl::WriteReplyParams(
-        reply_msg_, result, error_message);
-    nacl_host_message_filter_->Send(reply_msg_);
-    nacl_host_message_filter_ = NULL;
-    reply_msg_ = NULL;
+  if (nacl_host_message_filter_.get() == NULL || reply_msg_ == NULL) {
+    // As DCHECKed above, this case should not happen in general.
+    // Though, in the case, unfortunately there is no proper way to release

[Mark Seaborn, 2015/05/06 21:24:52]

Nit: "in the case" -> "in this case"

[hidehiko, 2015/05/07 01:29:06]

Done.

+    // resources which are already created in |result|. We just give up to

[Mark Seaborn, 2015/05/06 21:24:52]

Nit: "give up on releasing them"

[hidehiko, 2015/05/07 01:29:06]

Done.

+    // release them, and leak them.
+    return;
   }
+
+  NaClHostMsg_LaunchNaCl::WriteReplyParams(reply_msg_, result, error_message);
+  nacl_host_message_filter_->Send(reply_msg_);
+  nacl_host_message_filter_ = NULL;
+  reply_msg_ = NULL;
 }
 
 void NaClProcessHost::SetDebugStubPort(int port) {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
   nacl_browser->SetProcessGdbDebugStubPort(process_->GetData().id, port);
 }
 
 #if defined(OS_POSIX)
 // TCP port we chose for NaCl debug stub. It can be any other number.
 static const uint16_t kInitialDebugStubPort = 4014;
@@ skipping 178 matching lines @@
   } 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(
-    const IPC::ChannelHandle& browser_channel_handle,
-    const IPC::ChannelHandle& ppapi_renderer_channel_handle,
-    const IPC::ChannelHandle& trusted_renderer_channel_handle,
-    const IPC::ChannelHandle& manifest_service_channel_handle) {
+    const IPC::ChannelHandle& raw_browser_channel_handle,
+    const IPC::ChannelHandle& raw_ppapi_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_trusted_renderer_channel_handle,
+    const IPC::ChannelHandle& raw_manifest_service_channel_handle) {
+  ScopedChannelHandle browser_channel_handle(raw_browser_channel_handle);
+  ScopedChannelHandle ppapi_renderer_channel_handle(
+      raw_ppapi_renderer_channel_handle);
+  ScopedChannelHandle trusted_renderer_channel_handle(
+      raw_trusted_renderer_channel_handle);
+  ScopedChannelHandle manifest_service_channel_handle(
+      raw_manifest_service_channel_handle);
+
   if (!enable_ppapi_proxy()) {
-    ReplyToRenderer(IPC::ChannelHandle(),
-                    trusted_renderer_channel_handle,
-                    manifest_service_channel_handle);
+    ReplyToRenderer(ScopedChannelHandle(),
+                    trusted_renderer_channel_handle.Pass(),
+                    manifest_service_channel_handle.Pass());
     return;
   }
 
-  if (!ipc_proxy_channel_.get()) {
-    DCHECK_EQ(PROCESS_TYPE_NACL_LOADER, process_->GetData().process_type);
-
-    ipc_proxy_channel_ =
-        IPC::ChannelProxy::Create(browser_channel_handle,
-                                  IPC::Channel::MODE_CLIENT,
-                                  NULL,
-                                  base::MessageLoopProxy::current().get());
-    // Create the browser ppapi host and enable PPAPI message dispatching to the
-    // browser process.
-    ppapi_host_.reset(content::BrowserPpapiHost::CreateExternalPluginProcess(
-        ipc_proxy_channel_.get(),  // sender
-        permissions_,
-        process_->GetData().handle,
-        ipc_proxy_channel_.get(),
-        nacl_host_message_filter_->render_process_id(),
-        render_view_id_,
-        profile_directory_));
-    ppapi_host_->SetOnKeepaliveCallback(
-        NaClBrowser::GetDelegate()->GetOnKeepaliveCallback());
-
-    ppapi::PpapiNaClPluginArgs args;
-    args.off_the_record = nacl_host_message_filter_->off_the_record();
-    args.permissions = permissions_;
-    args.keepalive_throttle_interval_milliseconds =
-        keepalive_throttle_interval_milliseconds_;
-    base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
-    DCHECK(cmdline);
-    std::string flag_whitelist[] = {
-      switches::kV,
-      switches::kVModule,
-    };
-    for (size_t i = 0; i < arraysize(flag_whitelist); ++i) {
-      std::string value = cmdline->GetSwitchValueASCII(flag_whitelist[i]);
-      if (!value.empty()) {
-        args.switch_names.push_back(flag_whitelist[i]);
-        args.switch_values.push_back(value);
-      }
-    }
-
-    ppapi_host_->GetPpapiHost()->AddHostFactoryFilter(
-        scoped_ptr<ppapi::host::HostFactory>(
-            NaClBrowser::GetDelegate()->CreatePpapiHostFactory(
-                ppapi_host_.get())));
-
-    // Send a message to initialize the IPC dispatchers in the NaCl plugin.
-    ipc_proxy_channel_->Send(new PpapiMsg_InitializeNaClDispatcher(args));
-
-    // Let the renderer know that the IPC channels are established.
-    ReplyToRenderer(ppapi_renderer_channel_handle,
-                    trusted_renderer_channel_handle,
-                    manifest_service_channel_handle);
-  } else {
+  if (ipc_proxy_channel_.get()) {
     // Attempt to open more than 1 browser channel is not supported.
     // Shut down the NaCl process.
     process_->GetHost()->ForceShutdown();
+    return;
   }
+
+  DCHECK_EQ(PROCESS_TYPE_NACL_LOADER, process_->GetData().process_type);
+
+  ipc_proxy_channel_ =
+      IPC::ChannelProxy::Create(browser_channel_handle.release(),
+                                IPC::Channel::MODE_CLIENT,
+                                NULL,
+                                base::MessageLoopProxy::current().get());
+  // Create the browser ppapi host and enable PPAPI message dispatching to the
+  // browser process.
+  ppapi_host_.reset(content::BrowserPpapiHost::CreateExternalPluginProcess(
+      ipc_proxy_channel_.get(),  // sender
+      permissions_,
+      process_->GetData().handle,
+      ipc_proxy_channel_.get(),
+      nacl_host_message_filter_->render_process_id(),
+      render_view_id_,
+      profile_directory_));
+  ppapi_host_->SetOnKeepaliveCallback(
+      NaClBrowser::GetDelegate()->GetOnKeepaliveCallback());
+
+  ppapi::PpapiNaClPluginArgs args;
+  args.off_the_record = nacl_host_message_filter_->off_the_record();
+  args.permissions = permissions_;
+  args.keepalive_throttle_interval_milliseconds =
+      keepalive_throttle_interval_milliseconds_;
+  base::CommandLine* cmdline = base::CommandLine::ForCurrentProcess();
+  DCHECK(cmdline);
+  std::string flag_whitelist[] = {
+    switches::kV,
+    switches::kVModule,
+  };
+  for (size_t i = 0; i < arraysize(flag_whitelist); ++i) {
+    std::string value = cmdline->GetSwitchValueASCII(flag_whitelist[i]);
+    if (!value.empty()) {
+      args.switch_names.push_back(flag_whitelist[i]);
+      args.switch_values.push_back(value);
+    }
+  }
+
+  ppapi_host_->GetPpapiHost()->AddHostFactoryFilter(
+      scoped_ptr<ppapi::host::HostFactory>(
+          NaClBrowser::GetDelegate()->CreatePpapiHostFactory(
+              ppapi_host_.get())));
+
+  // Send a message to initialize the IPC dispatchers in the NaCl plugin.
+  ipc_proxy_channel_->Send(new PpapiMsg_InitializeNaClDispatcher(args));
+
+  // Let the renderer know that the IPC channels are established.
+  ReplyToRenderer(ppapi_renderer_channel_handle.Pass(),
+                  trusted_renderer_channel_handle.Pass(),
+                  manifest_service_channel_handle.Pass());
 }
 
 bool NaClProcessHost::StartWithLaunchedProcess() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   if (nacl_browser->IsReady()) {
     return StartNaClExecution();
   } else if (nacl_browser->IsOk()) {
     nacl_browser->WaitForResources(
         base::Bind(&NaClProcessHost::OnResourcesReady,
@@ skipping 160 matching lines @@
         process.Pass(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl