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..4267c893c64904c07f00003c947d81602ea64821 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -50,6 +50,15 @@ int LookUpFd(const base::GlobalDescriptors::Mapping& fd_mapping, uint32_t key) {
   return -1;
 }
 
+bool CreatePipe(base::ScopedFD* read_pipe, base::ScopedFD* write_pipe) {
+  int raw_pipe[2];
+  if (pipe(raw_pipe) != 0)

[jln (very slow on Chromium), 2014/05/02 18:25:00]

Feel free to PCHECK and return void, as you prefer.

[mdempsky, 2014/05/02 23:36:01]

Done.

+    return false;
+  read_pipe->reset(raw_pipe[0]);
+  write_pipe->reset(raw_pipe[1]);
+  return true;
+}
+
 }  // namespace
 
 Zygote::Zygote(int sandbox_flags,
@@ -295,6 +304,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 +312,42 @@ 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 {
+    if (!CreatePipe(&read_pipe, &write_pipe)) {
+      LOG(ERROR) << "Failed to create pipe";
+      return -1;
+    }
     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(UnixDomainSocket::SendMsg(
+        pid_oracle.get(), "x", 1, std::vector<int>()));

[jln (very slow on Chromium), 2014/05/02 18:25:00]

Do you want to make it a defined constant and check it on the other end?

The FD soup can get confusing and it could be helpful to immediately know if one
uses the wrong fd.

[mdempsky, 2014/05/02 23:36:01]

Done.

+
+    // 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(),

[jln (very slow on Chromium), 2014/05/02 18:25:00]

I would much rather not add more of these without using Pickles.

Since existing code already does that, feel free to land as-is (and eventually
migrate to Pickles in a further CL).

[mdempsky, 2014/05/02 20:23:09]

I'll look into this, and defer if I think it'll make the current CL too ugly.

[mdempsky, 2014/05/02 23:36:01]

I'm going to keep the pipe using just plain read/write for now.  I'll fix that
in a followup.

+                          reinterpret_cast<char*>(&real_pid),
                           sizeof(real_pid))) {
       LOG(FATAL) << "Failed to synchronise with parent zygote process";
     }
@@ -361,78 +363,51 @@ 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;
+  CHECK(base::ReadFromFD(kZygoteSocketPairFd,
+                         reinterpret_cast<char*>(&real_pid),

[jln (very slow on Chromium), 2014/05/02 18:25:00]

Pickle?

[mdempsky, 2014/05/02 23:36:01]

Done.

+                         sizeof(real_pid)));
+
+  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) {
+  error:

[jln (very slow on Chromium), 2014/05/02 18:25:00]

Let's kill this label at all cost!

[mdempsky, 2014/05/02 20:23:09]

Yeah, I'm not a fan of it and hope to kill it completely still.  I at least got
it down to just two cases and it only has the waitpid() call, so it's at least a
big improvement over the status quo I think.

[mdempsky, 2014/05/02 23:36:01]

Done.

+    if (waitpid(pid, NULL, WNOHANG) == -1)

[jln (very slow on Chromium), 2014/05/02 18:25:00]

This is racy. The kernel can perfectly return immediately even if the child is
dead, so we'd keep a zombie around.

Let's at least document this and return an (other) error if waitpid returns 0.

[mdempsky, 2014/05/02 23:36:01]

I changed this to using kill(pid, SIGKILL) in KillAndReap(), so I think there
shouldn't be any races now.  It should also help catch if the child process
closes the PID oracle socket, but then doesn't exit on its own.

+      LOG(ERROR) << "Failed to wait for child process";
+    return -1;
+  }
+
+  // If we're not using a helper, send the PID back to the child process.

[jln (very slow on Chromium), 2014/05/02 18:25:00]

Isn't it better to send something in all cases? I would much rather be sure that
the child process is still "trusted" and this point and know that it'll wait for
us

[mdempsky, 2014/05/02 20:23:09]

I'm leaning towards not sending the PID, but I don't feel super strongly either
way.  My rationale for doing it this way was:

  1. We want to discourage dependencies on children knowing their real PID, and
helper (i.e., NaCl) children already don't care.

  2. Non-SFI NaCl children actually specifically don't want to know their real
PID, so we need to add some helper-specific logic here either way; previously it
was sending a useless 0 PID, but now we might as well not send anything.

  3. Previously sending the PID was also an important synchronization so the
child would know when we've discovered its PID and it can close the dummy
socket.  This is no longer a concern now that we don't have a dummy socket.

  4. Arguably the children are still trusted even now, because we haven't
responded to the browser yet, so it hasn't had a chance to send the child any
untrusted content.

+  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)));

[jln (very slow on Chromium), 2014/05/02 18:25:00]

Pickle?

[jln (very slow on Chromium), 2014/05/02 18:25:00]

Aren't we at risk of SIGPIPE if the child disappears here?

Do you want to use sendmsg with MSG_NOSIGNAL instead?

[mdempsky, 2014/05/02 20:23:09]

Yeah, I was wondering about that.  I was somewhat relying on us ignoring
SIGPIPE.
Yeah, I'll change the pipe to a socketpair and use sendmsg/MSG_NOSIGNAL instead.

[mdempsky, 2014/05/02 23:36:01]

Decided to keep as a pipe and use read/write for now.  I noticed a few other
places where we're write()'ing to pipes/sockets and risk SIGPIPE'ing, so I'm
going to tackle that separately.

+    if (written != sizeof(real_pid)) {
       LOG(ERROR) << "Failed to synchronise with child process";
       goto error;
     }
-    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,
@@ -466,22 +441,30 @@ base::ProcessId Zygote::ReadArgsAndFork(const Pickle& pickle,
 
   if (!pickle.ReadInt(&iter, &numfds))
     return -1;
-  if (numfds != static_cast<int>(fds.size()))
+  if (numfds != static_cast<int>(fds.size()) - 1)

[jln (very slow on Chromium), 2014/05/02 18:25:00]

This seems very confusing.
Is it really problematic to account for the oracle socket on the other end?

[mdempsky, 2014/05/02 23:36:01]

Nope, just didn't think to do that instead.  Agreed, the logic is a bit cleaner
this way.

     return -1;
 
+  // First FD is the PID oracle socket.
+  base::ScopedFD pid_oracle(fds[0]->Pass());
+
+  // Remaining FDs are for the global descriptor mapping.
   for (int i = 0; i < numfds; ++i) {
     base::GlobalDescriptors::Key key;
     if (!pickle.ReadUInt32(&iter, &key))
       return -1;
-    mapping.push_back(std::make_pair(key, fds[i]->get()));
+    mapping.push_back(std::make_pair(key, fds[1 + i]->get()));
   }
 
   mapping.push_back(std::make_pair(
       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.