Linux Sandbox: Stop GPU watchdog in accountable way.

content/common/sandbox_linux/sandbox_linux.cc

Index: content/common/sandbox_linux/sandbox_linux.cc
diff --git a/content/common/sandbox_linux/sandbox_linux.cc b/content/common/sandbox_linux/sandbox_linux.cc
index 6b14a1c6e05454ff605f5744fd6e2896f286f5c7..5eaffebcefd6467dc8c06925b6582eab0d8c2bfb 100644
--- a/content/common/sandbox_linux/sandbox_linux.cc
+++ b/content/common/sandbox_linux/sandbox_linux.cc
@@ -8,6 +8,7 @@
 #include <sys/stat.h>
 #include <sys/time.h>
 #include <sys/types.h>
+#include <unistd.h>
 
 #include <limits>
 
@@ -15,6 +16,7 @@
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/singleton.h"
 #include "base/posix/eintr_wrapper.h"
 #include "base/strings/string_number_conversions.h"
@@ -25,10 +27,23 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/sandbox_linux.h"
 #include "sandbox/linux/services/credentials.h"
+#include "sandbox/linux/services/thread_helpers.h"
 #include "sandbox/linux/suid/client/setuid_sandbox_client.h"
 
 namespace {
 
+struct FDCloser {
+  inline void operator()(int* fd) const {
+    DCHECK(fd);
+    PCHECK(0 == IGNORE_EINTR(close(*fd)));
+    *fd = -1;
+  }
+};
+
+// Don't use base::ScopedFD since it doesn't CHECK that the file descriptor was
+// closed.
+typedef scoped_ptr<int, FDCloser> SafeScopedFD;
+
 void LogSandboxStarted(const std::string& sandbox_name) {
   const CommandLine& command_line = *CommandLine::ForCurrentProcess();
   const std::string process_type =
@@ -64,6 +79,21 @@ bool IsRunningTSAN() {
 #endif
 }
 
+// Try to open /proc/self/task/ with the help of |proc_fd|. |proc_fd| can be
+// -1. Will return -1 on error and set errno like open(2).
+int OpenProcTaskFd(int proc_fd) {
+  int proc_self_task = -1;
+  if (proc_fd >= 0) {
+    // If a handle to /proc is available, use it. This allows to bypass file
+    // system restrictions.
+    proc_self_task = openat(proc_fd, "self/task/", O_RDONLY | O_DIRECTORY);
+  } else {
+    // Otherwise, make an attempt to access the file system directly.
+    proc_self_task = open("/proc/self/task/", O_RDONLY | O_DIRECTORY);
+  }
+  return proc_self_task;
+}
+
 }  // namespace
 
 namespace content {
@@ -125,6 +155,11 @@ bool LinuxSandbox::InitializeSandbox() {
   return linux_sandbox->InitializeSandboxImpl();
 }
 
+void LinuxSandbox::StopThread(base::Thread* thread) {
+  LinuxSandbox* linux_sandbox = LinuxSandbox::GetInstance();
+  linux_sandbox->StopThreadImpl(thread);
+}
+
 int LinuxSandbox::GetStatus() {
   CHECK(pre_initialized_);
   if (kSandboxLinuxInvalid == sandbox_status_flags_) {
@@ -153,35 +188,28 @@ int LinuxSandbox::GetStatus() {
 // PID namespaces and existing sandboxes, so "self" must really be used instead
 // of using the pid.
 bool LinuxSandbox::IsSingleThreaded() const {
-  struct stat task_stat;
-  int fstat_ret;
-  if (proc_fd_ >= 0) {
-    // If a handle to /proc is available, use it. This allows to bypass file
-    // system restrictions.
-    fstat_ret = fstatat(proc_fd_, "self/task/", &task_stat, 0);
-  } else {
-    // Otherwise, make an attempt to access the file system directly.
-    fstat_ret = fstatat(AT_FDCWD, "/proc/self/task/", &task_stat, 0);
-  }
-  // In Debug mode, it's mandatory to be able to count threads to catch bugs.
+  bool is_single_threaded = false;
+  int proc_self_task = OpenProcTaskFd(proc_fd_);
+  SafeScopedFD task_closer(&proc_self_task);
+
+// In Debug mode, it's mandatory to be able to count threads to catch bugs.
 #if !defined(NDEBUG)
   // Using DCHECK here would be incorrect. DCHECK can be enabled in non

[Jorge Lucangeli Obes, 2014/02/07 22:30:34]

Are you trying to always keep the check?

[jln (very slow on Chromium), 2014/02/07 22:53:07]

The check needs to match exactly the cases where we do always have proc_fd_
available, which is only !NDEBUG (for security reasons). We may update this soon
for ASAN.

[Jorge Lucangeli Obes, 2014/02/08 00:07:11]

I ask because
https://code.google.com/p/chromium/codesearch#chromium/src/base/logging.h&l=468
suggests !NDEBUG is enough to guarantee DCHECK will be there.

Anyhow, I found the CHECK confusing because the comment suggests you are not
using DCHECK because DCHECK could be enabled in situations where you don't want
it enabled, but CHECK is gonna be enabled in all situations anyways.

I.e. IIUC the only reason to use CHECK over DCHECK is DCHECK being disabled, not
DCHECK being enabled -- the set "builds where DCHECK is enabled" is strictly
included in the set "builds where CHECK is enabled".

[jln (very slow on Chromium), 2014/02/08 00:13:41]

The comment pertains to using if "!defined(NDEBUG)" + CHECK rather than a
DCHECK.

This define has to match line 136 exactly. I want to check in exactly all cases
where line 136 is built.

[Jorge Lucangeli Obes, 2014/02/08 00:24:22]

Understood. Then I would suggest that as a comment.

"Using CHECK here since we want to check all the cases where !defined(NDEBUG)
gets built."

[jln (very slow on Chromium), 2014/02/08 00:36:35]

Done.

   // official release mode.

[Jorge Lucangeli Obes, 2014/02/07 22:30:34]

nit: non-official.

[jln (very slow on Chromium), 2014/02/07 22:53:07]

Done.

-  CHECK_EQ(0, fstat_ret) << "Could not count threads, the sandbox was not "
-                         << "pre-initialized properly.";
+  CHECK_LE(0, proc_self_task) << "Could not count threads, the sandbox was not "
+                              << "pre-initialized properly.";
 #endif  // !defined(NDEBUG)
-  if (fstat_ret) {
+
+  if (proc_self_task < 0) {
     // Pretend to be monothreaded if it can't be determined (for instance the
     // setuid sandbox is already engaged but no proc_fd_ is available).
-    return true;
+    is_single_threaded = true;
+  } else {
+    is_single_threaded =
+        sandbox::ThreadHelpers::IsSingleThreaded(proc_self_task);
   }
 
-  // At least "..", "." and the current thread should be present.
-  CHECK_LE(3UL, task_stat.st_nlink);
-  // Counting threads via /proc/self/task could be racy. For the purpose of
-  // determining if the current proces is monothreaded it works: if at any
-  // time it becomes monothreaded, it'll stay so.
-  return task_stat.st_nlink == 3;
+  return is_single_threaded;
 }
 
 bool LinuxSandbox::seccomp_bpf_started() const {
@@ -257,6 +285,10 @@ bool LinuxSandbox::InitializeSandboxImpl() {
   return seccomp_bpf_started;
 }
 
+void LinuxSandbox::StopThreadImpl(base::Thread* thread) {
+  DCHECK(thread);
+  StopThreadAndEnsureNotCounted(thread);
+}
 
 bool LinuxSandbox::seccomp_bpf_supported() const {
   CHECK(pre_initialized_);
@@ -303,7 +335,7 @@ bool LinuxSandbox::LimitAddressSpace(const std::string& process_type) {
 #endif  // !defined(ADDRESS_SANITIZER)
 }
 
-bool LinuxSandbox::HasOpenDirectories() {
+bool LinuxSandbox::HasOpenDirectories() const {
   return sandbox::Credentials().HasOpenDirectory(proc_fd_);
 }
 
@@ -330,4 +362,12 @@ void LinuxSandbox::CheckForBrokenPromises(const std::string& process_type) {
   }
 }
 
+void LinuxSandbox::StopThreadAndEnsureNotCounted(base::Thread* thread) const {
+  DCHECK(thread);
+  int proc_self_task = OpenProcTaskFd(proc_fd_);
+  SafeScopedFD task_closer(&proc_self_task);
+  CHECK(
+      sandbox::ThreadHelpers::StopThreadAndWatchProcFS(proc_self_task, thread));
+}
+
 }  // namespace content