Linux sandbox: Provide AssertSingleThreaded() helper

sandbox/linux/services/thread_helpers.cc

Index: sandbox/linux/services/thread_helpers.cc
diff --git a/sandbox/linux/services/thread_helpers.cc b/sandbox/linux/services/thread_helpers.cc
index dbadbd44e7011593174770856ad89a700c95bcb2..3d91558ff02c7601af4644e55b80ceeed4937f9f 100644
--- a/sandbox/linux/services/thread_helpers.cc
+++ b/sandbox/linux/services/thread_helpers.cc
@@ -14,6 +14,9 @@
 #include <string>
 
 #include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/files/scoped_file.h"
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/strings/string_number_conversions.h"
@@ -24,6 +27,9 @@ namespace sandbox {
 
 namespace {
 
+const char kAssertSingleThreadedError[] =
+    "Current process is not mono-threaded!";
+
 bool IsSingleThreadedImpl(int proc_self_task) {
   CHECK_LE(0, proc_self_task);
   struct stat task_stat;
@@ -38,22 +44,96 @@ bool IsSingleThreadedImpl(int proc_self_task) {
   return task_stat.st_nlink == 3;
 }
 
-}  // namespace
+bool IsThreadPresentInProcFS(int proc_self_task,
+                             const std::string& thread_id_dir_str) {
+  struct stat task_stat;
+  const int fstat_ret =
+      fstatat(proc_self_task, thread_id_dir_str.c_str(), &task_stat, 0);

[mdempsky, 2015/02/05 02:12:42]

minor: Since you don't care about the stat result, you could potentially use
faccessat() with F_OK.  But might be simpler to understand as is.

[jln (very slow on Chromium), 2015/02/09 22:24:50]

Good point! I'll keep as-is for now so that I don't have to go through another
cycle of updating seccomp policies.

+  if (fstat_ret < 0) {
+    PCHECK(ENOENT == errno);
+    return false;
+  }
+  return true;
+}
 
-bool ThreadHelpers::IsSingleThreaded(int proc_self_task) {
+// Run |cb| in a loop until it returns false. Every time |cb| runs, sleep
+// for an exponentially increasing amount of time. |cb| is expected to return
+// false very quickly and this will crash if it doesn't happen withing ~64ms on
+// Debug builds (2s on Release builds).
+// This is guaranteed to not sleep more than twice as much as the bare minimum
+// amount of time.
+void RunUntilFalse(const base::Callback<bool(void)>& cb) {

[mdempsky, 2015/02/05 02:12:42]

"RunWhile" or "RunWhileTrue" feels like a more intuitive name IMO.

[jln (very slow on Chromium), 2015/02/09 22:24:50]

Done.

+  unsigned int iterations = 0;
+  // Run |cb| with an exponential back-off, sleeping 2^iterations nanoseconds
+  // in nanosleep(2).
+  // Note: the clock may not allow for nanosecond granularity, in this case the
+  // first iterations would sleep a tiny bit more instead, which would not
+  // change the calculations significantly.
+  while (true) {

[mdempsky, 2015/02/05 02:12:41]

I would think this would be more readable as something like:

  #if ...
  const unsigned int kMaxIterations = ...
  #else
  ...
  #endif

  for (unsigned int i = 0; i < kMaxIterations; ++i) {
    if (!cb.Run()) {
      return;
    }

    nanosleep(...);
  }

  LOG(FATAL) << ...
  NOTREACHED();

[jln (very slow on Chromium), 2015/02/09 22:24:50]

Ooch yeah, it looks like this re-factor wasn't complete yet.

Thanks, that's much nicer!

+    if (!cb.Run()) {
+      return;
+    }
+
+    // Increase the waiting time exponentially.
+    struct timespec ts = {0, 1L << iterations /* nanoseconds */};
+    PCHECK(0 == HANDLE_EINTR(nanosleep(&ts, &ts)));
+    ++iterations;
+
+#if defined(NDEBUG)
+    // In Release mode, crash after 30 iterations, which means having spent
+    // roughly 2s in
+    // nanosleep(2) cumulatively.
+    const unsigned int kMaxIterations = 30U;
+#else
+    // In practice, this never goes through more than a couple iterations. In
+    // debug mode, crash after 64ms (+ eventually 25 times the granularity of
+    // the clock) in nanosleep(2). This ensures that this is not becoming too
+    // slow.
+    const unsigned int kMaxIterations = 25U;
+#endif
+    if (iterations >= kMaxIterations) {
+      LOG(FATAL) << kAssertSingleThreadedError << " (iterations: "
+                 << iterations << ")";
+    }
+  }
+  NOTREACHED();
+}
+
+// Return a ScopedFD to /proc/self/task/. If |proc_self_task| is -1, try to
+// open it directly, otherwise duplicate it.
+base::ScopedFD OpenProcSelfTask(int proc_self_task) {
   DCHECK_LE(-1, proc_self_task);
   if (-1 == proc_self_task) {
-    const int task_fd =
-        open("/proc/self/task/", O_RDONLY | O_DIRECTORY | O_CLOEXEC);
-    PCHECK(0 <= task_fd);
-    const bool result = IsSingleThreadedImpl(task_fd);
-    PCHECK(0 == IGNORE_EINTR(close(task_fd)));
-    return result;
-  } else {
-    return IsSingleThreadedImpl(proc_self_task);
+    return base::ScopedFD(HANDLE_EINTR(
+        open("/proc/self/task/", O_RDONLY | O_DIRECTORY | O_CLOEXEC)));
   }
+
+  return base::ScopedFD(HANDLE_EINTR(
+      openat(proc_self_task, "./", O_RDONLY | O_DIRECTORY | O_CLOEXEC)));

[mdempsky, 2015/02/05 02:12:41]

Two small questions:
  1. Why use openat() instead of fcntl() with F_DUPFD_CLOEXEC?
  2. Assuming openat() is needed, any reason to use "./" here instead of "."?

[jln (very slow on Chromium), 2015/02/09 22:24:50]

Ahh yeah, good point. I wanted to avoid dup3(), but F_DUPFD_CLOEXEC would work.

The other reasons were:
- The symmetry between open() and openat() is nice (I should probably even use
openat() with AT_FDCWD instead of open). Given seccomp etc, it's always nice to
reduce the syscall footprint.

- we have methods that take handles to /proc/self/task/ and others to
/proc/self/ and I want to unify everything (to /proc/self/), so the pattern
would already be correct.
I like the pattern of "if X must be a directory, enforce it with X/". I admit
that this could be pushing the paranoia a little, given O_DIRECTORY.

 }
 
+bool IsMultiThreaded(int proc_self_task) {
+  return !ThreadHelpers::IsSingleThreaded(proc_self_task);
+}
+
+}  // namespace
+
+// static
+bool ThreadHelpers::IsSingleThreaded(int proc_self_task) {

[mdempsky, 2015/02/05 02:12:42]

This feels like a kinda clunky interface.  What about just having two separate
functions: one that takes a file descriptor (which must be valid), and one that
opens internally?

[jln (very slow on Chromium), 2015/02/09 22:24:50]

Yeah, you're right. I won't change it at this point as it should be part of a
larger refactor if we decide to do it (there are proxy function for these in
content/).

I'm not 100% sold yet: there are a bunch of functions in content/ with a similar
interface at this point and they all can call each other. If we unbundle one, we
need to unbundle all of them.

But maybe it's the right thing to do.

+  DCHECK_LE(-1, proc_self_task);
+  base::ScopedFD task_fd(OpenProcSelfTask(proc_self_task));
+  CHECK(task_fd.is_valid());
+  return IsSingleThreadedImpl(task_fd.get());
+}
+
+// static
+void ThreadHelpers::AssertSingleThreaded(int proc_self_task) {
+  const base::Callback<bool(void)> cb =
+      base::Bind(&IsMultiThreaded, proc_self_task);
+  RunUntilFalse(cb);
+}
+
+// static
 bool ThreadHelpers::StopThreadAndWatchProcFS(int proc_self_task,
                                              base::Thread* thread) {
   DCHECK_LE(0, proc_self_task);
@@ -66,38 +146,17 @@ bool ThreadHelpers::StopThreadAndWatchProcFS(int proc_self_task,
   // not have been updated.
   thread->Stop();
 
-  unsigned int iterations = 0;
-  bool thread_present_in_procfs = true;
-  // Poll /proc with an exponential back-off, sleeping 2^iterations nanoseconds
-  // in nanosleep(2).
-  // Note: the clock may not allow for nanosecond granularity, in this case the
-  // first iterations would sleep a tiny bit more instead, which would not
-  // change the calculations significantly.
-  while (thread_present_in_procfs) {
-    struct stat task_stat;
-    const int fstat_ret =
-        fstatat(proc_self_task, thread_id_dir_str.c_str(), &task_stat, 0);
-    if (fstat_ret < 0) {
-      PCHECK(ENOENT == errno);
-      // The thread disappeared from /proc, we're done.
-      thread_present_in_procfs = false;
-      break;
-    }
-    // Increase the waiting time exponentially.
-    struct timespec ts = {0, 1L << iterations /* nanoseconds */};
-    PCHECK(0 == HANDLE_EINTR(nanosleep(&ts, &ts)));
-    ++iterations;
+  const base::Callback<bool(void)> cb =
+      base::Bind(&IsThreadPresentInProcFS, proc_self_task, thread_id_dir_str);
 
-    // Crash after 30 iterations, which means having spent roughly 2s in
-    // nanosleep(2) cumulatively.
-    CHECK_GT(30U, iterations);
-    // In practice, this never goes through more than a couple iterations. In
-    // debug mode, crash after 64ms (+ eventually 25 times the granularity of
-    // the clock) in nanosleep(2).
-    DCHECK_GT(25U, iterations);
-  }
+  RunUntilFalse(cb);
 
   return true;
 }
 
+// static
+const char* ThreadHelpers::GetAssertSingleThreadedErrorMessageForTests() {
+  return kAssertSingleThreadedError;
+}
+
 }  // namespace sandbox