Add sandbox support for AsanCoverage.

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 b5186734de8ef82449bec27c0c9a12e90a0d24ec..e74763289599aa5272388765c8d8d82113af7dc6 100644
--- a/content/zygote/zygote_linux.cc
+++ b/content/zygote/zygote_linux.cc
@@ -77,11 +77,14 @@ void KillAndReap(pid_t pid, ZygoteForkDelegate* helper) {
 
 }  // 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() {
 }
@@ -147,6 +150,13 @@ bool Zygote::HandleRequestFromBrowser(int fd) {
 
   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;
   }