Support a new remote IPC for for GetTerminationStatus.

(Reland of https://chromiumcodereview.appspot.com/23020010/)

This does three things:

1. Clean-up a lot of the code in the Linux NaCl loader.
2. Create a new IPC to the NaCl loader "Zygote" for the
main Zygote to query the termination status of processes.
3. Clean-up some code in the Zygote and fix "process tracking".

zygote_linux.cc:
* Split GetTerminationStatus() out from HandleGetTerminationStatus().
* Handle the case where we need to perform a remote IPC for GetTerminationStatus()
* Use the new GetTerminationStatus() to support the remote case for HandleReapRequest().
* Replace real_pids_to_sandbox_pids mapping with process_info_map_.
* Update shortcut case in ForkWithRealPid() to fill this out.
* Update GetTerminationStatus() to remove existing entries.

zygote_fork_delegate_linux.h:
* Create a new GetTerminationStatus() interface.

nacl_helper_linux.cc:
* Split HandleZygoteRequest() out from main().
* Split ChildNaClLoaderInit() to handle the child side of a fork().
* Handle a new IPC in HandleGetTerminationStatusRequest().

nacl_fork_delegate_linux.cc:
* Implement the new GetTerminationStatus() interface.
* Use Pickle for IPCs, make IPCs easier to write with SendIPCRequestAndReadReply().

BUG=133453
R=mseaborn@chromium.org, piman@chromium.org
TBR=mseaborn@chromium.org

Review URL: https://codereview.chromium.org/22875026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@218610 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 119 insertions, 37 deletions

diff --git a/content/zygote/zygote_linux.cc b/content/zygote/zygote_linux.cc
index 6e42a9d..65ed435 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -155,37 +155,99 @@ bool Zygote::HandleRequestFromBrowser(int fd) {
   return false;
 }
 
+// TODO(jln): remove callers to this broken API. See crbug.com/274855.
 void Zygote::HandleReapRequest(int fd,
                                const Pickle& pickle,
                                PickleIterator iter) {
   base::ProcessId child;
-  base::ProcessId actual_child;
 
   if (!pickle.ReadInt(&iter, &child)) {
     LOG(WARNING) << "Error parsing reap request from browser";
     return;
   }
 
-  if (UsingSUIDSandbox()) {
-    actual_child = real_pids_to_sandbox_pids[child];
-    if (!actual_child)
-      return;
-    real_pids_to_sandbox_pids.erase(child);
+  if (process_info_map_.find(child) == process_info_map_.end()) {
+    // TODO(jln): test on more bots and add a DCHECK.
+    LOG(ERROR) << "Child not found!";
+    return;
+  }
+  const base::ProcessId actual_child = process_info_map_[child].internal_pid;
+  const bool started_from_helper =
+      process_info_map_[child].started_from_helper;
+  if (!started_from_helper) {
+    // TODO(jln): this old code is completely broken. See crbug.com/274855.
+    base::EnsureProcessTerminated(actual_child);
   } else {
-    actual_child = child;
+    // For processes from the helper, send a GetTerminationStatus request
+    // with known_dead set to true.
+    // This is not perfect, as the process may be killed instantly, but is
+    // better than ignoring the request.
+    base::TerminationStatus status;
+    int exit_code;
+    bool got_termination_status =
+        GetTerminationStatus(child, true /* known_dead */, &status, &exit_code);
+    DCHECK(got_termination_status);
   }
+  process_info_map_.erase(child);
+}
 
-  base::EnsureProcessTerminated(actual_child);
+bool Zygote::GetTerminationStatus(base::ProcessHandle real_pid,
+                                  bool known_dead,
+                                  base::TerminationStatus* status,
+                                  int* exit_code) {
+  // Do we know about this child?
+  if (process_info_map_.find(real_pid) == process_info_map_.end()) {
+    // TODO(jln): test on more bots and add a DCHECK.
+    LOG(ERROR) << "Zygote::GetTerminationStatus for unknown PID "
+               << real_pid;
+    return false;
+  }
+  // We know about |real_pid|.
+  const base::ProcessHandle child =
+      process_info_map_[real_pid].internal_pid;
+  const bool started_from_helper =
+      process_info_map_[real_pid].started_from_helper;
+  if (started_from_helper) {
+    // Let the helper handle the request.
+    DCHECK(helper_);
+    if (!helper_->GetTerminationStatus(child, known_dead, status, exit_code)) {
+      return false;
+    }
+  } else {
+    // Handle the request directly.
+    if (known_dead) {
+      // If we know that the process is already dead and the kernel is cleaning
+      // it up, we do want to wait until it becomes a zombie and not risk
+      // returning eroneously that it is still running. However, we do not
+      // want to risk a bug where we're told a process is dead when it's not.
+      // By sending SIGKILL, we make sure that WaitForTerminationStatus will
+      // return quickly even in this case.
+      if (kill(child, SIGKILL)) {
+        PLOG(ERROR) << "kill (" << child << ")";
+      }
+      *status = base::WaitForTerminationStatus(child, exit_code);
+    } else {
+      // We don't know if the process is dying, so get its status but don't
+      // wait.
+      *status = base::GetTerminationStatus(child, exit_code);
+    }
+  }
+  // Successfully got a status for |real_pid|.
+  if (*status != base::TERMINATION_STATUS_STILL_RUNNING) {
+    // Time to forget about this process.
+    process_info_map_.erase(real_pid);
+  }
+  return true;
 }
 
 void Zygote::HandleGetTerminationStatus(int fd,
                                         const Pickle& pickle,
                                         PickleIterator iter) {
   bool known_dead;
-  base::ProcessHandle child;
+  base::ProcessHandle child_requested;
 
   if (!pickle.ReadBool(&iter, &known_dead) ||
-      !pickle.ReadInt(&iter, &child)) {
+      !pickle.ReadInt(&iter, &child_requested)) {
     LOG(WARNING) << "Error parsing GetTerminationStatus request "
                  << "from browser";
     return;
@@ -193,26 +255,13 @@ void Zygote::HandleGetTerminationStatus(int fd,
 
   base::TerminationStatus status;
   int exit_code;
-  if (UsingSUIDSandbox())
-    child = real_pids_to_sandbox_pids[child];
-  if (child) {
-    if (known_dead) {
-      // If we know that the process is already dead and the kernel is cleaning
-      // it up, we do want to wait until it becomes a zombie and not risk
-      // returning eroneously that it is still running. However, we do not
-      // want to risk a bug where we're told a process is dead when it's not.
-      // By sending SIGKILL, we make sure that WaitForTerminationStatus will
-      // return quickly even in this case.
-      if (kill(child, SIGKILL)) {
-        PLOG(ERROR) << "kill (" << child << ")";
-      }
-      status = base::WaitForTerminationStatus(child, &exit_code);
-    } else {
-      status = base::GetTerminationStatus(child, &exit_code);
-    }
-  } else {
+
+  bool got_termination_status =
+      GetTerminationStatus(child_requested, known_dead, &status, &exit_code);
+  if (!got_termination_status) {
     // Assume that if we can't find the child in the sandbox, then
     // it terminated normally.
+    // TODO(jln): add a DCHECK.
     status = base::TERMINATION_STATUS_NORMAL_TERMINATION;
     exit_code = RESULT_CODE_NORMAL_EXIT;
   }
@@ -236,8 +285,15 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
                                                        uma_name,
                                                        uma_sample,
                                                        uma_boundary_value));
+  // TODO(jln): this shortcut is silly and does nothing but make the code
+  // harder to follow and to test. Get rid of it.
   if (!(use_helper || UsingSUIDSandbox())) {
-    return fork();
+    pid_t pid = fork();
+    if (pid > 0) {
+      process_info_map_[pid].internal_pid = pid;
+      process_info_map_[pid].started_from_helper = use_helper;
+    }
+    return pid;
   }
 
   int dummy_fd;
@@ -326,13 +382,20 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
         LOG(ERROR) << "METHOD_GET_CHILD_WITH_INODE failed";
         goto error;
       }
-
-      real_pids_to_sandbox_pids[real_pid] = pid;
     } else {
       // If no SUID sandbox is involved then no pid translation is
       // necessary.
       real_pid = pid;
     }
+
+    // Now set-up this process to be tracked by the Zygote.
+    if (process_info_map_.find(real_pid) != process_info_map_.end()) {
+      // TODO(jln): add DCHECK.
+      LOG(ERROR) << "Already tracking PID " << real_pid;
+    }
+    process_info_map_[real_pid].internal_pid = pid;
+    process_info_map_[real_pid].started_from_helper = use_helper;
+
     if (use_helper) {
       if (!helper_->AckChild(pipe_fds[1], channel_switch)) {
         LOG(ERROR) << "Failed to synchronise with zygote fork helper";
diff --git a/content/zygote/zygote_linux.h b/content/zygote/zygote_linux.h
index 3b175ac..327f917 100644
--- a/content/zygote/zygote_linux.h
+++ b/content/zygote/zygote_linux.h
@@ -8,7 +8,8 @@
 #include <string>
 #include <vector>
 
-#include "base/containers/hash_tables.h"
+#include "base/containers/small_map.h"
+#include "base/process/kill.h"
 #include "base/process/process.h"
 
 class Pickle;
@@ -33,6 +34,16 @@ class Zygote {
   static const int kMagicSandboxIPCDescriptor = 5;
 
  private:
+  struct ZygoteProcessInfo {
+    // Pid from inside the Zygote's PID namespace.
+    base::ProcessHandle internal_pid;
+    // Keeps track of whether or not a process was started from a fork
+    // delegate helper.
+    bool started_from_helper;
+  };
+  typedef base::SmallMap< std::map<base::ProcessHandle, ZygoteProcessInfo> >
+      ZygoteProcessMap;
+
   // Returns true if the SUID sandbox is active.
   bool UsingSUIDSandbox() const;
 
@@ -45,6 +56,14 @@ class Zygote {
 
   void HandleReapRequest(int fd, const Pickle& pickle, PickleIterator iter);
 
+  // Get the termination status of |real_pid|. |real_pid| is the PID as it
+  // appears outside of the sandbox.
+  // Return true if it managed to get the termination status and return the
+  // status in |status| and the exit code in |exit_code|.
+  bool GetTerminationStatus(base::ProcessHandle real_pid, bool known_dead,
+                            base::TerminationStatus* status,
+                            int* exit_code);
+
   void HandleGetTerminationStatus(int fd,
                                   const Pickle& pickle,
                                   PickleIterator iter);
@@ -84,11 +103,11 @@ class Zygote {
                               const Pickle& pickle,
                               PickleIterator iter);
 
-  // In the SUID sandbox, we try to use a new PID namespace. Thus the PIDs
-  // fork() returns are not the real PIDs, so we need to map the Real PIDS
-  // into the sandbox PID namespace.
-  typedef base::hash_map<base::ProcessHandle, base::ProcessHandle> ProcessMap;
-  ProcessMap real_pids_to_sandbox_pids;
+  // The Zygote needs to keep some information about each process. Most
+  // notably what the PID of the process is inside the PID namespace of
+  // the Zygote and whether or not a process was started by the
+  // ZygoteForkDelegate helper.
+  ZygoteProcessMap process_info_map_;
 
   const int sandbox_flags_;
   ZygoteForkDelegate* helper_;