Add sandbox support for AsanCoverage.

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 59 matching lines @@
   }
 
   // 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 == HANDLE_EINTR(waitpid(pid, NULL, 0)));
 }
 
 }  // namespace
 
-Zygote::Zygote(int sandbox_flags, ScopedVector<ZygoteForkDelegate> helpers)
+Zygote::Zygote(int sandbox_flags, ScopedVector<ZygoteForkDelegate> helpers,
+               const std::vector<base::ProcessHandle>& extra_children,
+               const std::vector<int>& extra_fds)
     : sandbox_flags_(sandbox_flags),
       helpers_(helpers.Pass()),
-      initial_uma_index_(0) {
-}
+      initial_uma_index_(0),
+      extra_children_(extra_children),
+      extra_fds_(extra_fds) {}
 
 Zygote::~Zygote() {
 }
 
 bool Zygote::ProcessRequests() {
   // A SOCK_SEQPACKET socket is installed in fd 3. We get commands from the
   // browser on it.
   // A SOCK_DGRAM is installed in fd 5. This is the sandbox IPC channel.
   // See http://code.google.com/p/chromium/wiki/LinuxSandboxIPC
 
@@ skipping 44 matching lines @@
 bool Zygote::UsingSUIDSandbox() const {
   return sandbox_flags_ & kSandboxLinuxSUID;
 }
 
 bool Zygote::HandleRequestFromBrowser(int fd) {
   ScopedVector<base::ScopedFD> fds;
   char buf[kZygoteMaxMessageLength];
   const ssize_t len = UnixDomainSocket::RecvMsg(fd, buf, sizeof(buf), &fds);
 
   if (len == 0 || (len == -1 && errno == ECONNRESET)) {
     // EOF from the browser. We should die.

[earthdok, 2014/05/27 12:50:12]

Note to self: call __asan_cov_dump() to dump coverage from the zygote itself at
this point.

+    for (std::vector<int>::iterator it = extra_fds_.begin();

[jln (very slow on Chromium), 2014/05/27 23:15:00]

Would you mind adding a TODO(earthdok) to replace the "note to self"?

[earthdok, 2014/05/28 16:44:51]

I was planning to add the call as part of this CL, I just didn't have the time
to do it yesterday.

I realize now that __sanitizer_cov_dump() actually closes the file descriptor
before returning. Thus if we choose to call it here, we won't need extra_fds_ at
all. I'm not sure I'm happy with introducing this sort of implicit dependency
(i.e. if __sanitizer_cov_dump forgets to close the FD, we'll be stuck waiting on
the helper) but it's probably ok as long as we document it as part of the
interface. WDYT?

(I'd still need to fix the sanitizer side to close the FD even if coverage is
not enabled, so I cannot implement this just yet. Added a TODO)

[jln (very slow on Chromium), 2014/05/29 23:29:19]

Yeah, it would be nice to get rid of extra_fds_.

If you can document the behavior very clearly as part of the interface, and make
sure that it's unit tested (we could even unit test it in a Chrome unit test),
it seems fine.

+         it < extra_fds_.end(); it++)

[jln (very slow on Chromium), 2014/05/27 23:15:00]

Style: always use {} for multi-line if/for

[jln (very slow on Chromium), 2014/05/27 23:15:00]

Nit: ++it is more idiomatic than it++ as it never requires storing a temporary.

[earthdok, 2014/05/28 16:44:51]

Done.

[earthdok, 2014/05/28 16:44:51]

Done.

+      PCHECK(0 == IGNORE_EINTR(close(*it)));
+    for (std::vector<base::ProcessHandle>::iterator it =
+             extra_children_.begin();
+         it < extra_children_.end(); it++)

[jln (very slow on Chromium), 2014/05/27 23:15:00]

Style: {}

[jln (very slow on Chromium), 2014/05/27 23:15:00]

Nit: ++it

[earthdok, 2014/05/28 16:44:51]

Done.

[earthdok, 2014/05/28 16:44:51]

Done.

+      PCHECK(*it == HANDLE_EINTR(waitpid(*it, NULL, 0)));

[jln (very slow on Chromium), 2014/05/27 23:15:00]

I'm a little scared of the implicit requirement that all renderers are exiting
at this point. It should work, but it's something that could easily regress.

I think we'll really need to add a watchdog thread here for production code.
ASAN is the only user of this for now, but I fear something else will use it in
the future.

How about something such as:

#if !defined(ADDRESS_SANITIZER)
// TODO(jln): add watchdog thread before using this in non-ASAN builds.
CHECK(extra_children.empty());
#endif

[earthdok, 2014/05/28 16:44:51]

Like I said, I'm willing to implement the watchdog thread if you can suggest a
good home for the watchdog code (which will be shared between this class and
SuicideOnChannelErrorFilter).

I think it's best to do that in a separate CL, so I've added your suggested
CHECK for now.

[jln (very slow on Chromium), 2014/05/29 23:29:19]

I wonder if we could simply use base::Watchdog for this.
CreateWaitAndExitThread() creates a non-joinable thread, which base::Watchdog
wouldn't do, but I don't think this is a big deal. I probably only did that due
to an abundance of caution. We could simply leak the Watchdog object.

If there is a case for not using base::Watchdog,
content/common/wait_and_exit_thread_linux.cc could be a reasonable home in that
case, or maybe base/threading/, depending on how generic that reason seems.

It would be great if you could take care of it. Feel free to change TODO(jln) to
TODO(earthdok) in that case :)

     _exit(0);
     return false;
   }
 
   if (len == -1) {
     PLOG(ERROR) << "Error reading message from browser";
     return false;
   }
 
   Pickle pickle(buf, len);
@@ skipping 401 matching lines @@
                                     PickleIterator iter) {
   if (HANDLE_EINTR(write(fd, &sandbox_flags_, sizeof(sandbox_flags_))) !=
                    sizeof(sandbox_flags_)) {
     PLOG(ERROR) << "write";
   }
 
   return false;
 }
 
 }  // namespace content