Enforce handle ownership in base::Process

content/browser/child_process_launcher.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 "content/browser/child_process_launcher.h"
 
 #include <utility>  // For std::pair.
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 57 matching lines @@
         terminate_child_on_shutdown_(false)
 #else
         terminate_child_on_shutdown_(true)
 #endif
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
         , zygote_(false)
 #endif
         {
   }
 
-  void Launch(
-      SandboxedProcessLauncherDelegate* delegate,
-      base::CommandLine* cmd_line,
-      int child_process_id,
-      Client* client) {
+  void Launch(SandboxedProcessLauncherDelegate* delegate,
+              base::CommandLine* cmd_line,
+              int child_process_id,
+              Client* client) {
     client_ = client;
 
     CHECK(BrowserThread::GetCurrentThreadIdentifier(&client_thread_id_));
 
 #if defined(OS_ANDROID)
     // We need to close the client end of the IPC channel to reliably detect
     // child termination. We will close this fd after we create the child
     // process which is asynchronous on Android.
     ipcfd_ = delegate->GetIpcFd();
 #endif
     BrowserThread::PostTask(
         BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
-        base::Bind(
-            &Context::LaunchInternal,
-            make_scoped_refptr(this),
-            client_thread_id_,
-            child_process_id,
-            delegate,
-            cmd_line));
+        base::Bind(&Context::LaunchInternal,
+                   make_scoped_refptr(this),
+                   client_thread_id_,
+                   child_process_id,
+                   delegate,
+                   cmd_line));
   }
 
 #if defined(OS_ANDROID)
   static void OnChildProcessStarted(
       // |this_object| is NOT thread safe. Only use it to post a task back.
       scoped_refptr<Context> this_object,
       BrowserThread::ID client_thread_id,
       const base::TimeTicks begin_launch_time,
       base::ProcessHandle handle) {
     RecordHistograms(begin_launch_time);
     if (BrowserThread::CurrentlyOn(client_thread_id)) {
       // This is always invoked on the UI thread which is commonly the
       // |client_thread_id| so we can shortcut one PostTask.
-      this_object->Notify(handle);
+      this_object->Notify(base::ProcessObject(handle));

[scottmg, 2014/10/14 18:51:03]

I'm a little worried that it's going to be difficult to make progress on
ownership, since handle<->ProcessObject isn't restricted.

[rvargas (doing something else), 2014/10/14 19:56:54]

The problem is that restricting the behavior implies fixing potentially hundreds
of callers at the same time :(.

The idea for this code is that StartSandboxedProcess,
base::LaunchElevatedProcess, StartChildProcess, etc, will return a proper
base::Process, but we have to be able to change a few files at a time.

     } else {
       BrowserThread::PostTask(
           client_thread_id, FROM_HERE,
           base::Bind(
               &ChildProcessLauncher::Context::Notify,
               this_object,
-              handle));
+              base::Passed(base::ProcessObject(handle))));
     }
   }
 #endif
 
   void ResetClient() {
     // No need for locking as this function gets called on the same thread that
     // client_ would be used.
     CHECK(BrowserThread::CurrentlyOn(client_thread_id_));
     client_ = NULL;
   }
 
   void set_terminate_child_on_shutdown(bool terminate_on_shutdown) {
     terminate_child_on_shutdown_ = terminate_on_shutdown;
   }
 
+  void GetTerminationStatus() {
+    termination_status_ =
+        base::GetTerminationStatus(process_.Handle(), &exit_code_);
+  }
+
+  void SetProcessBackgrounded(bool background) {
+    base::ProcessObject to_pass = process_.Duplicate();
+    BrowserThread::PostTask(
+        BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
+        base::Bind(&Context::SetProcessBackgroundedInternal,
+                   base::Passed(&to_pass), background));
+  }
+
  private:
   friend class base::RefCountedThreadSafe<ChildProcessLauncher::Context>;
   friend class ChildProcessLauncher;
 
   ~Context() {
     Terminate();
   }
 
   static void RecordHistograms(const base::TimeTicks begin_launch_time) {
     base::TimeDelta launch_time = base::TimeTicks::Now() - begin_launch_time;
@@ skipping 168 matching lines @@
   if (handle)
     RecordHistograms(begin_launch_time);
   BrowserThread::PostTask(
       client_thread_id, FROM_HERE,
       base::Bind(
           &Context::Notify,
           this_object.get(),
 #if defined(OS_POSIX) && !defined(OS_MACOSX)
           use_zygote,
 #endif
-          handle));
+          base::Passed(base::ProcessObject(handle))));
 #endif  // !defined(OS_ANDROID)
   }
 
   void Notify(
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
       bool zygote,
 #endif
-      base::ProcessHandle handle) {
+      base::ProcessObject process) {
 #if defined(OS_ANDROID)
     // Finally close the ipcfd
     base::ScopedFD ipcfd_closer(ipcfd_);
 #endif
     starting_ = false;
-    process_.set_handle(handle);
-    if (!handle)
+    process_ = process.Pass();
+    if (!process_.IsValid())
       LOG(ERROR) << "Failed to launch child process";
 
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
     zygote_ = zygote;
 #endif
     if (client_) {
-      if (handle) {
+      if (process_.IsValid()) {
         client_->OnProcessLaunched();
       } else {
         client_->OnProcessLaunchFailed();
       }
     } else {
       Terminate();
     }
   }
 
   void Terminate() {
-    if (!process_.handle())
+    if (!process_.IsValid())
       return;
 
     if (!terminate_child_on_shutdown_)
       return;
 
     // On Posix, EnsureProcessTerminated can lead to 2 seconds of sleep!  So
     // don't this on the UI/IO threads.
     BrowserThread::PostTask(
         BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
         base::Bind(
             &Context::TerminateInternal,
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
             zygote_,
 #endif
-            process_.handle()));
-    process_.set_handle(base::kNullProcessHandle);
+            base::Passed(&process_)));
   }
 
-  static void SetProcessBackgrounded(base::ProcessHandle handle,
-                                     bool background) {
-    base::Process process(handle);
+  static void SetProcessBackgroundedInternal(base::ProcessObject process,
+                                             bool background) {
     process.SetProcessBackgrounded(background);
 #if defined(OS_ANDROID)
-    SetChildProcessInForeground(handle, !background);
+    SetChildProcessInForeground(process.Handle(), !background);
 #endif
   }
 
   static void TerminateInternal(
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
       bool zygote,
 #endif
-      base::ProcessHandle handle) {
+      base::ProcessObject process) {
 #if defined(OS_ANDROID)
-    VLOG(0) << "ChromeProcess: Stopping process with handle " << handle;
-    StopChildProcess(handle);
+    VLOG(1) << "ChromeProcess: Stopping process with handle "
+            << process.Handle();
+    StopChildProcess(process.Handle());
 #else
-    base::Process process(handle);
      // Client has gone away, so just kill the process.  Using exit code 0
     // means that UMA won't treat this as a crash.
     process.Terminate(RESULT_CODE_NORMAL_EXIT);
     // On POSIX, we must additionally reap the child.
 #if defined(OS_POSIX)
 #if !defined(OS_MACOSX)
     if (zygote) {
       // If the renderer was created via a zygote, we have to proxy the reaping
       // through the zygote process.
-      ZygoteHostImpl::GetInstance()->EnsureProcessTerminated(handle);
+      ZygoteHostImpl::GetInstance()->EnsureProcessTerminated(process.Handle());
     } else
 #endif  // !OS_MACOSX
     {
-      base::EnsureProcessTerminated(handle);
+      base::EnsureProcessTerminated(process.Handle());
     }
 #endif  // OS_POSIX
-    process.Close();
 #endif  // defined(OS_ANDROID)
   }
 
   Client* client_;
   BrowserThread::ID client_thread_id_;
-  base::Process process_;
+  base::ProcessObject process_;
   base::TerminationStatus termination_status_;
   int exit_code_;
   bool starting_;
   // Controls whether the child process should be terminated on browser
   // shutdown. Default behavior is to terminate the child.
   bool terminate_child_on_shutdown_;
 #if defined(OS_ANDROID)
   // The fd to close after creating the process.
   int ipcfd_;
 #elif defined(OS_POSIX) && !defined(OS_MACOSX)
@@ skipping 16 matching lines @@
 }
 
 ChildProcessLauncher::~ChildProcessLauncher() {
   context_->ResetClient();
 }
 
 bool ChildProcessLauncher::IsStarting() {
   return context_->starting_;
 }
 
-base::ProcessHandle ChildProcessLauncher::GetHandle() {
+const base::ProcessObject& ChildProcessLauncher::GetProcess() const {
   DCHECK(!context_->starting_);
-  return context_->process_.handle();
+  return context_->process_;
 }
 
 base::TerminationStatus ChildProcessLauncher::GetChildTerminationStatus(
     bool known_dead,
     int* exit_code) {
-  base::ProcessHandle handle = context_->process_.handle();
-  if (handle == base::kNullProcessHandle) {
+  if (!context_->process_.IsValid()) {
     // Process is already gone, so return the cached termination status.
     if (exit_code)
       *exit_code = context_->exit_code_;
     return context_->termination_status_;
   }
 #if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_ANDROID)
   if (context_->zygote_) {
     context_->termination_status_ = ZygoteHostImpl::GetInstance()->
-        GetTerminationStatus(handle, known_dead, &context_->exit_code_);
+        GetTerminationStatus(context_->process_.Handle(), known_dead,
+                             &context_->exit_code_);
   } else if (known_dead) {
     context_->termination_status_ =
-        base::GetKnownDeadTerminationStatus(handle, &context_->exit_code_);
+        base::GetKnownDeadTerminationStatus(context_->process_.Handle(),
+                                            &context_->exit_code_);
   } else {
 #elif defined(OS_MACOSX)
   if (known_dead) {
     context_->termination_status_ =
-        base::GetKnownDeadTerminationStatus(handle, &context_->exit_code_);
+        base::GetKnownDeadTerminationStatus(context_->process_.Handle(),
+                                            &context_->exit_code_);
   } else {
 #elif defined(OS_ANDROID)
-  if (IsChildProcessOomProtected(handle)) {
-      context_->termination_status_ = base::TERMINATION_STATUS_OOM_PROTECTED;
+  if (IsChildProcessOomProtected(context_->process_.Handle())) {
+    context_->termination_status_ = base::TERMINATION_STATUS_OOM_PROTECTED;
   } else {
 #else
   {
 #endif
-    context_->termination_status_ =
-        base::GetTerminationStatus(handle, &context_->exit_code_);
+    context_->GetTerminationStatus();
   }
 
   if (exit_code)
     *exit_code = context_->exit_code_;
 
   // POSIX: If the process crashed, then the kernel closed the socket
   // for it and so the child has already died by the time we get
   // here. Since GetTerminationStatus called waitpid with WNOHANG,
   // it'll reap the process.  However, if GetTerminationStatus didn't
   // reap the child (because it was still running), we'll need to
   // Terminate via ProcessWatcher. So we can't close the handle here.
   if (context_->termination_status_ != base::TERMINATION_STATUS_STILL_RUNNING)
     context_->process_.Close();
 
   return context_->termination_status_;
 }
 
 void ChildProcessLauncher::SetProcessBackgrounded(bool background) {
-  BrowserThread::PostTask(
-     BrowserThread::PROCESS_LAUNCHER, FROM_HERE,
-     base::Bind(
-         &ChildProcessLauncher::Context::SetProcessBackgrounded,
-         GetHandle(), background));
+  context_->process_.SetProcessBackgrounded(background);
 }
 
 void ChildProcessLauncher::SetTerminateChildOnShutdown(
     bool terminate_on_shutdown) {
   if (context_.get())
     context_->set_terminate_child_on_shutdown(terminate_on_shutdown);
 }
 
 }  // namespace content