Use RecvMsgWithPid to find real PID for zygote children

content/zygote/zygote_linux.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/zygote/zygote_linux.h"
 
 #include <fcntl.h>
 #include <string.h>
 #include <sys/socket.h>
 #include <sys/types.h>
@@ skipping 32 matching lines @@
 }
 
 int LookUpFd(const base::GlobalDescriptors::Mapping& fd_mapping, uint32_t key) {
   for (size_t index = 0; index < fd_mapping.size(); ++index) {
     if (fd_mapping[index].first == key)
       return fd_mapping[index].second;
   }
   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,
                ZygoteForkDelegate* helper)
     : sandbox_flags_(sandbox_flags),
       helper_(helper),
       initial_uma_sample_(0),
       initial_uma_boundary_value_(0) {
   if (helper_) {
     helper_->InitialUMA(&initial_uma_name_,
@@ skipping 225 matching lines @@
   write_pickle.WriteInt(exit_code);
   ssize_t written =
       HANDLE_EINTR(write(fd, write_pickle.data(), write_pickle.size()));
   if (written != static_cast<ssize_t>(write_pickle.size()))
     PLOG(ERROR) << "write";
 }
 
 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) {
   const bool use_helper = (helper_ && helper_->CanHelp(process_type,
                                                        uma_name,
                                                        uma_sample,
                                                        uma_boundary_value));
-  int dummy_fd;
-  ino_t dummy_inode;
-  int pipe_fds[2] = { -1, -1 };
+
+  base::ScopedFD read_pipe, write_pipe;
   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;
-  }
-
   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";
     }
     if (real_pid <= 0) {
       LOG(FATAL) << "Invalid pid from parent zygote";
     }
 #if defined(OS_LINUX)
     // Sandboxed processes need to send the global, non-namespaced PID when
     // setting up an IPC channel to their parent.
     IPC::Channel::SetGlobalPid(real_pid);
     // Force the real PID so chrome event data have a PID that corresponds
     // to system trace event data.
     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;
-      }
+  // In the parent process.
+  read_pipe.reset();
+  pid_oracle.reset();
 
-      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;
-    }
+  // 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)));
 
-    // 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;
+  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,
                                         PickleIterator iter,
                                         ScopedVector<base::ScopedFD> fds,
                                         std::string* uma_name,
                                         int* uma_sample,
                                         int* uma_boundary_value) {
   std::vector<std::string> args;
   int argc = 0;
@@ skipping 13 matching lines @@
     std::string arg;
     if (!pickle.ReadString(&iter, &arg))
       return -1;
     args.push_back(arg);
     if (arg.compare(0, channel_id_prefix.length(), channel_id_prefix) == 0)
       channel_id = arg.substr(channel_id_prefix.length());
   }
 
   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.
 
     // Our socket from the browser.
     PCHECK(0 == IGNORE_EINTR(close(kZygoteSocketPairFd)));
 
     // Pass ownership of file descriptors from fds to GlobalDescriptors.
     for (ScopedVector<base::ScopedFD>::iterator i = fds.begin(); i != fds.end();
          ++i)
@@ skipping 55 matching lines @@
                                     PickleIterator iter) {
   if (HANDLE_EINTR(write(fd, &sandbox_flags_, sizeof(sandbox_flags_))) !=
                    sizeof(sandbox_flags_)) {
     PLOG(ERROR) << "write";
   }
 
   return false;
 }
 
 }  // namespace content