1 files changed, 94 insertions, 31 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";