Use RecvMsgWithPid to find real PID for zygote children

content/zygote/zygote_linux.cc

Index: content/zygote/zygote_linux.cc
diff --git a/content/zygote/zygote_linux.cc b/content/zygote/zygote_linux.cc
index 67b071c2fd08fc4a2361e85747005619c42a2227..7cdeaf5cead4c9319eeea7501cb5104968222666 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -50,6 +50,29 @@ int LookUpFd(const base::GlobalDescriptors::Mapping& fd_mapping, uint32_t key) {
   return -1;
 }
 
+void CreatePipe(base::ScopedFD* read_pipe, base::ScopedFD* write_pipe) {
+  int raw_pipe[2];
+  PCHECK(0 == pipe(raw_pipe));
+  read_pipe->reset(raw_pipe[0]);
+  write_pipe->reset(raw_pipe[1]);
+}
+
+void KillAndReap(pid_t pid, bool use_helper) {
+  if (use_helper) {
+    // Helper children may be forked in another PID namespace, so |pid| might
+    // be meaningless to us; or we just might not be able to directly send it
+    // signals.  So we can't kill it.
+    // Additionally, we're not its parent, so we can't reap it anyway.
+    LOG(WARNING) << "Unable to kill or reap helper children";
+    return;
+  }
+
+  // Kill the child process in case it's not already dead, so we can safely
+  // perform a blocking wait.
+  PCHECK(0 == kill(pid, SIGKILL));
+  PCHECK(pid == waitpid(pid, NULL, 0));

[jln (very slow on Chromium), 2014/05/05 19:01:42]

HANDLE_EINTR()

[mdempsky, 2014/05/05 21:38:18]

Removed KillAndReap().

+}
+
 }  // namespace
 
 Zygote::Zygote(int sandbox_flags,
@@ -161,6 +184,12 @@ bool Zygote::HandleRequestFromBrowser(int fd) {
       case kZygoteCommandGetSandboxStatus:
         HandleGetSandboxStatus(fd, pickle, iter);
         return false;
+      case kZygoteCommandForkRealPID:
+        // This shouldn't happen in practice, but some failure paths in
+        // HandleForkRequest (e.g., if ReadArgsAndFork fails during depickling)
+        // could leave this command pending on the socket.
+        LOG(WARNING) << "Unexpected real PID message from browser";

[jln (very slow on Chromium), 2014/05/05 19:01:42]

Let's add NOTREACHED() then?

[mdempsky, 2014/05/05 21:38:18]

Done.

+        return false;
       default:
         NOTREACHED();
         break;
@@ -295,6 +324,7 @@ void Zygote::HandleGetTerminationStatus(int fd,
 int Zygote::ForkWithRealPid(const std::string& process_type,
                             const base::GlobalDescriptors::Mapping& fd_mapping,
                             const std::string& channel_id,
+                            base::ScopedFD pid_oracle,
                             std::string* uma_name,
                             int* uma_sample,
                             int* uma_boundary_value) {
@@ -302,50 +332,38 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
                                                        uma_name,
                                                        uma_sample,
                                                        uma_boundary_value));
-  int dummy_fd;
-  ino_t dummy_inode;
-  int pipe_fds[2] = { -1, -1 };
-  base::ProcessId pid = 0;
-
-  dummy_fd = socket(PF_UNIX, SOCK_DGRAM, 0);
-  if (dummy_fd < 0) {
-    LOG(ERROR) << "Failed to create dummy FD";
-    goto error;
-  }
-  if (!base::FileDescriptorGetInode(&dummy_inode, dummy_fd)) {
-    LOG(ERROR) << "Failed to get inode for dummy FD";
-    goto error;
-  }
-  if (pipe(pipe_fds) != 0) {
-    LOG(ERROR) << "Failed to create pipe";
-    goto error;
-  }
 
+  base::ScopedFD read_pipe, write_pipe;
+  base::ProcessId pid = 0;
   if (use_helper) {
-    std::vector<int> fds;
     int ipc_channel_fd = LookUpFd(fd_mapping, kPrimaryIPCChannel);
     if (ipc_channel_fd < 0) {
       DLOG(ERROR) << "Failed to find kPrimaryIPCChannel in FD mapping";
-      goto error;
+      return -1;
     }
+    std::vector<int> fds;
     fds.push_back(ipc_channel_fd);  // kBrowserFDIndex
-    fds.push_back(dummy_fd);  // kDummyFDIndex
-    fds.push_back(pipe_fds[0]);  // kParentFDIndex
+    fds.push_back(pid_oracle.get());  // kPIDOracleFDIndex
     pid = helper_->Fork(process_type, fds, channel_id);
+
+    // Helpers should never return in the child process.
+    CHECK_NE(pid, 0);
   } else {
+    CreatePipe(&read_pipe, &write_pipe);
     pid = fork();
   }
-  if (pid < 0) {
-    goto error;
-  } else if (pid == 0) {
+
+  if (pid == 0) {
     // In the child process.
-    close(pipe_fds[1]);
+    write_pipe.reset();
+
+    // Ping the PID oracle socket so the browser can find our PID.
+    CHECK(SendZygoteChildPing(pid_oracle.get()));
+
+    // Now read back our real PID from the zygote.
     base::ProcessId real_pid;
-    // Wait until the parent process has discovered our PID.  We
-    // should not fork any child processes (which the seccomp
-    // sandbox does) until then, because that can interfere with the
-    // parent's discovery of our PID.
-    if (!base::ReadFromFD(pipe_fds[0], reinterpret_cast<char*>(&real_pid),
+    if (!base::ReadFromFD(read_pipe.get(),
+                          reinterpret_cast<char*>(&real_pid),
                           sizeof(real_pid))) {
       LOG(FATAL) << "Failed to synchronise with parent zygote process";
     }
@@ -361,78 +379,64 @@ int Zygote::ForkWithRealPid(const std::string& process_type,
     base::debug::TraceLog::GetInstance()->SetProcessID(
         static_cast<int>(real_pid));
 #endif
-    close(pipe_fds[0]);
-    close(dummy_fd);
     return 0;
-  } else {
-    // In the parent process.
-    close(dummy_fd);
-    dummy_fd = -1;
-    close(pipe_fds[0]);
-    pipe_fds[0] = -1;
-    base::ProcessId real_pid;
-    if (UsingSUIDSandbox()) {
-      uint8_t reply_buf[512];
-      Pickle request;
-      request.WriteInt(LinuxSandbox::METHOD_GET_CHILD_WITH_INODE);
-      request.WriteUInt64(dummy_inode);
-
-      const ssize_t r = UnixDomainSocket::SendRecvMsg(
-          GetSandboxFD(), reply_buf, sizeof(reply_buf), NULL,
-          request);
-      if (r == -1) {
-        LOG(ERROR) << "Failed to get child process's real PID";
-        goto error;
-      }
-
-      Pickle reply(reinterpret_cast<char*>(reply_buf), r);
-      PickleIterator iter(reply);
-      if (!reply.ReadInt(&iter, &real_pid))
-        goto error;
-      if (real_pid <= 0) {
-        // METHOD_GET_CHILD_WITH_INODE failed. Did the child die already?
-        LOG(ERROR) << "METHOD_GET_CHILD_WITH_INODE failed";
-        goto error;
-      }
-    } 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()) {
-      LOG(ERROR) << "Already tracking PID " << real_pid;
-      NOTREACHED();
-    }
-    process_info_map_[real_pid].internal_pid = pid;
-    process_info_map_[real_pid].started_from_helper = use_helper;
+  // In the parent process.
+  read_pipe.reset();
+  pid_oracle.reset();
+
+  // Always receive a real PID from the zygote host, though it might
+  // be invalid (see below).
+  base::ProcessId real_pid;
+  {
+    ScopedVector<base::ScopedFD> recv_fds;
+    char buf[kZygoteMaxMessageLength];
+    const ssize_t len = UnixDomainSocket::RecvMsg(
+        kZygoteSocketPairFd, buf, sizeof(buf), &recv_fds);
+    CHECK_GT(len, 0);
+    CHECK(recv_fds.empty());
+
+    Pickle pickle(buf, len);
+    PickleIterator iter(pickle);
+
+    int kind;
+    CHECK(pickle.ReadInt(&iter, &kind));
+    CHECK(kind == kZygoteCommandForkRealPID);
+    CHECK(pickle.ReadInt(&iter, &real_pid));
+  }
+
+  // Fork failed.
+  if (pid < 0)
+    return -1;
 
-    // If we're using a helper, we still need to let the child process know
-    // we've discovered its real PID, but we don't actually reveal the PID.
-    const base::ProcessId pid_for_child = use_helper ? 0 : real_pid;
+  // If we successfully forked a child, but it crashed without sending
+  // a message to the browser, the browser won't have found its PID.
+  if (real_pid < 0) {
+    KillAndReap(pid, use_helper);

[jln (very slow on Chromium), 2014/05/05 19:01:42]

Since the children are trusted at this point, what do you think about LOG(FATAL)
instead here?

This code is very complex and I think we should use assertion as much as
possible when they make sense to prevent things from regressing.

[mdempsky, 2014/05/05 21:38:18]

Works for me, done.

+    return -1;
+  }
+
+  // If we're not using a helper, send the PID back to the child process.
+  if (!use_helper) {
     ssize_t written =
-        HANDLE_EINTR(write(pipe_fds[1], &pid_for_child, sizeof(pid_for_child)));
-    if (written != sizeof(pid_for_child)) {
+        HANDLE_EINTR(write(write_pipe.get(), &real_pid, sizeof(real_pid)));
+    if (written != sizeof(real_pid)) {
       LOG(ERROR) << "Failed to synchronise with child process";
-      goto error;
+      KillAndReap(pid, use_helper);

[jln (very slow on Chromium), 2014/05/05 19:01:42]

Same remark, how about just LOG(FATAL)? KillAndReap is already too complex since
it won't work with fork delegates.

[mdempsky, 2014/05/05 21:38:18]

Done.

+      return -1;
     }
-    close(pipe_fds[1]);
-    return real_pid;
   }
 
- error:
-  if (pid > 0) {
-    if (waitpid(pid, NULL, WNOHANG) == -1)
-      LOG(ERROR) << "Failed to wait for process";
+  // Now set-up this process to be tracked by the Zygote.
+  if (process_info_map_.find(real_pid) != process_info_map_.end()) {
+    LOG(ERROR) << "Already tracking PID " << real_pid;
+    NOTREACHED();
   }
-  if (dummy_fd >= 0)
-    close(dummy_fd);
-  if (pipe_fds[0] >= 0)
-    close(pipe_fds[0]);
-  if (pipe_fds[1] >= 0)
-    close(pipe_fds[1]);
-  return -1;
+  process_info_map_[real_pid].internal_pid = pid;
+  process_info_map_[real_pid].started_from_helper = use_helper;
+
+  return real_pid;
 }
 
 base::ProcessId Zygote::ReadArgsAndFork(const Pickle& pickle,
@@ -469,7 +473,13 @@ base::ProcessId Zygote::ReadArgsAndFork(const Pickle& pickle,
   if (numfds != static_cast<int>(fds.size()))
     return -1;
 
-  for (int i = 0; i < numfds; ++i) {
+  // First FD is the PID oracle socket.
+  if (fds.size() < 1)
+    return -1;
+  base::ScopedFD pid_oracle(fds[0]->Pass());
+
+  // Remaining FDs are for the global descriptor mapping.
+  for (int i = 1; i < numfds; ++i) {
     base::GlobalDescriptors::Key key;
     if (!pickle.ReadUInt32(&iter, &key))
       return -1;
@@ -480,8 +490,12 @@ base::ProcessId Zygote::ReadArgsAndFork(const Pickle& pickle,
       static_cast<uint32_t>(kSandboxIPCChannel), GetSandboxFD()));
 
   // Returns twice, once per process.
-  base::ProcessId child_pid = ForkWithRealPid(process_type, mapping, channel_id,
-                                              uma_name, uma_sample,
+  base::ProcessId child_pid = ForkWithRealPid(process_type,
+                                              mapping,
+                                              channel_id,
+                                              pid_oracle.Pass(),
+                                              uma_name,
+                                              uma_sample,
                                               uma_boundary_value);
   if (!child_pid) {
     // This is the child process.