Linux sandbox: fix part of the *SingleThreaded() APIs

Various APIs take either a valid file descriptor or -1 as a parameter.
This change uses the type system to make this distinction instead.

This does not address all instances but makes some progress.

BUG=457530

Review URL: https://codereview.chromium.org/914053002

Cr-Commit-Position: refs/heads/master@{#315936}

10 files changed, 57 insertions, 41 deletions

diff --git a/sandbox/linux/BUILD.gn b/sandbox/linux/BUILD.gn
index eae3d6e..ed4e697 100644
--- a/sandbox/linux/BUILD.gn
+++ b/sandbox/linux/BUILD.gn
@@ -66,6 +66,7 @@ source_set("sandbox_linux_test_utils") {
 # The main sandboxing test target.
 test("sandbox_linux_unittests") {
   sources = [
+    "services/proc_util_unittest.cc",
     "services/scoped_process_unittest.cc",
     "services/syscall_wrappers_unittest.cc",
     "services/thread_helpers_unittests.cc",
@@ -121,7 +122,6 @@ test("sandbox_linux_unittests") {
       "services/credentials_unittest.cc",
       "services/namespace_sandbox_unittest.cc",
       "services/namespace_utils_unittest.cc",
-      "services/proc_util_unittest.cc",
     ]
   }
 }
@@ -231,14 +231,16 @@ component("sandbox_services") {
   sources = [
     "services/init_process_reaper.cc",
     "services/init_process_reaper.h",
+    "services/proc_util.cc",
+    "services/proc_util.h",
     "services/scoped_process.cc",
     "services/scoped_process.h",
     "services/syscall_wrappers.cc",
     "services/syscall_wrappers.h",
     "services/thread_helpers.cc",
     "services/thread_helpers.h",
-    "services/yama.h",
     "services/yama.cc",
+    "services/yama.h",
     "syscall_broker/broker_channel.cc",
     "syscall_broker/broker_channel.h",
     "syscall_broker/broker_client.cc",
@@ -264,8 +266,6 @@ component("sandbox_services") {
       "services/namespace_sandbox.h",
       "services/namespace_utils.cc",
       "services/namespace_utils.h",
-      "services/proc_util.cc",
-      "services/proc_util.h",
     ]
 
     # For capabilities.cc.
diff --git a/sandbox/linux/sandbox_linux.gypi b/sandbox/linux/sandbox_linux.gypi
index 2a01820..a1aa9f0 100644
--- a/sandbox/linux/sandbox_linux.gypi
+++ b/sandbox/linux/sandbox_linux.gypi
@@ -221,6 +221,8 @@
       'sources': [
         'services/init_process_reaper.cc',
         'services/init_process_reaper.h',
+        'services/proc_util.cc',
+        'services/proc_util.h',
         'services/scoped_process.cc',
         'services/scoped_process.h',
         'services/syscall_wrappers.cc',
@@ -258,8 +260,6 @@
             'services/namespace_sandbox.h',
             'services/namespace_utils.cc',
             'services/namespace_utils.h',
-            'services/proc_util.cc',
-            'services/proc_util.h',
           ],
           'dependencies': [
             # for capabilities.cc.
diff --git a/sandbox/linux/sandbox_linux_test_sources.gypi b/sandbox/linux/sandbox_linux_test_sources.gypi
index 6077fb1..2e57ff3 100644
--- a/sandbox/linux/sandbox_linux_test_sources.gypi
+++ b/sandbox/linux/sandbox_linux_test_sources.gypi
@@ -17,6 +17,7 @@
     '../..',
   ],
   'sources': [
+    'services/proc_util_unittest.cc',
     'services/scoped_process_unittest.cc',
     'services/syscall_wrappers_unittest.cc',
     'services/thread_helpers_unittests.cc',
@@ -59,7 +60,6 @@
         'services/credentials_unittest.cc',
         'services/namespace_sandbox_unittest.cc',
         'services/namespace_utils_unittest.cc',
-        'services/proc_util_unittest.cc',
       ],
     }],
   ],
diff --git a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
index 588cd2e..6f385b8 100644
--- a/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
+++ b/sandbox/linux/seccomp-bpf/sandbox_bpf.cc
@@ -34,6 +34,7 @@
 #include "sandbox/linux/seccomp-bpf/syscall.h"
 #include "sandbox/linux/seccomp-bpf/trap.h"
 #include "sandbox/linux/seccomp-bpf/verifier.h"
+#include "sandbox/linux/services/proc_util.h"
 #include "sandbox/linux/services/syscall_wrappers.h"
 #include "sandbox/linux/services/thread_helpers.h"
 #include "sandbox/linux/system_headers/linux_syscalls.h"
@@ -116,6 +117,10 @@ bool SandboxBPF::StartSandbox(SeccompLevel seccomp_level) {
     return false;
   }
 
+  if (!proc_task_fd_.is_valid()) {
+    SetProcTaskFd(ProcUtil::OpenProcSelfTask());
+  }
+
   const bool supports_tsync = KernelSupportsSeccompTsync();
 
   if (seccomp_level == SeccompLevel::SINGLE_THREADED) {
diff --git a/sandbox/linux/services/proc_util.cc b/sandbox/linux/services/proc_util.cc
index fd0e348..13d8842 100644
--- a/sandbox/linux/services/proc_util.cc
+++ b/sandbox/linux/services/proc_util.cc
@@ -13,6 +13,7 @@
 
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
+#include "base/posix/eintr_wrapper.h"
 #include "base/strings/string_number_conversions.h"
 
 namespace sandbox {
@@ -31,7 +32,8 @@ typedef scoped_ptr<DIR, DIRCloser> ScopedDIR;
 
 int ProcUtil::CountOpenFds(int proc_fd) {
   DCHECK_LE(0, proc_fd);
-  int proc_self_fd = openat(proc_fd, "self/fd", O_DIRECTORY | O_RDONLY);
+  int proc_self_fd = HANDLE_EINTR(
+      openat(proc_fd, "self/fd", O_DIRECTORY | O_RDONLY | O_CLOEXEC));
   PCHECK(0 <= proc_self_fd);
 
   // Ownership of proc_self_fd is transferred here, it must not be closed
@@ -109,4 +111,13 @@ bool ProcUtil::HasOpenDirectory(int proc_fd) {
   return false;
 }
 
+//static
+base::ScopedFD ProcUtil::OpenProcSelfTask() {
+  base::ScopedFD proc_self_task(
+   HANDLE_EINTR(
+      open("/proc/self/task/", O_RDONLY | O_DIRECTORY | O_CLOEXEC)));
+  PCHECK(proc_self_task.is_valid());
+  return proc_self_task.Pass();
+}
+
 }  // namespace sandbox
diff --git a/sandbox/linux/services/proc_util.h b/sandbox/linux/services/proc_util.h
index 9d9bb2e..74388f8 100644
--- a/sandbox/linux/services/proc_util.h
+++ b/sandbox/linux/services/proc_util.h
@@ -5,6 +5,7 @@
 #ifndef SANDBOX_LINUX_SERVICES_PROC_UTIL_H_
 #define SANDBOX_LINUX_SERVICES_PROC_UTIL_H_
 
+#include "base/files/scoped_file.h"
 #include "base/macros.h"
 #include "sandbox/sandbox_export.h"
 
@@ -31,6 +32,9 @@ class SANDBOX_EXPORT ProcUtil {
   // false.
   static bool HasOpenDirectory(int proc_fd);
 
+  // Open /proc/self/task/ or crash if it cannot.
+  static base::ScopedFD OpenProcSelfTask();
+
  private:
   DISALLOW_IMPLICIT_CONSTRUCTORS(ProcUtil);
 };
diff --git a/sandbox/linux/services/scoped_process.cc b/sandbox/linux/services/scoped_process.cc
index bdeaee9..4433201 100644
--- a/sandbox/linux/services/scoped_process.cc
+++ b/sandbox/linux/services/scoped_process.cc
@@ -39,7 +39,7 @@ ScopedProcess::ScopedProcess(const base::Closure& child_callback)
   PCHECK(0 == pipe(pipe_fds_));
 #if !defined(THREAD_SANITIZER)
   // Make sure that we can safely fork().
-  CHECK(ThreadHelpers::IsSingleThreaded(-1));
+  CHECK(ThreadHelpers::IsSingleThreaded());
 #endif
   child_process_id_ = fork();
   PCHECK(0 <= child_process_id_);
diff --git a/sandbox/linux/services/thread_helpers.cc b/sandbox/linux/services/thread_helpers.cc
index 3916aa7..51feb98 100644
--- a/sandbox/linux/services/thread_helpers.cc
+++ b/sandbox/linux/services/thread_helpers.cc
@@ -22,6 +22,7 @@
 #include "base/strings/string_number_conversions.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/thread.h"
+#include "sandbox/linux/services/proc_util.h"
 
 namespace sandbox {
 
@@ -97,19 +98,6 @@ void RunWhileTrue(const base::Callback<bool(void)>& cb) {
   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) {
-    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)));
-}
-
 bool IsMultiThreaded(int proc_self_task) {
   return !ThreadHelpers::IsSingleThreaded(proc_self_task);
 }
@@ -118,19 +106,29 @@ bool IsMultiThreaded(int proc_self_task) {
 
 // static
 bool ThreadHelpers::IsSingleThreaded(int proc_self_task) {
-  DCHECK_LE(-1, proc_self_task);
-  base::ScopedFD task_fd(OpenProcSelfTask(proc_self_task));
-  CHECK(task_fd.is_valid());
-  return IsSingleThreadedImpl(task_fd.get());
+  DCHECK_LE(0, proc_self_task);
+  return IsSingleThreadedImpl(proc_self_task);
+}
+
+// static
+bool ThreadHelpers::IsSingleThreaded() {
+  base::ScopedFD task_fd(ProcUtil::OpenProcSelfTask());
+  return IsSingleThreaded(task_fd.get());
 }
 
 // static
 void ThreadHelpers::AssertSingleThreaded(int proc_self_task) {
+  DCHECK_LE(0, proc_self_task);
   const base::Callback<bool(void)> cb =
       base::Bind(&IsMultiThreaded, proc_self_task);
   RunWhileTrue(cb);
 }
 
+void ThreadHelpers::AssertSingleThreaded() {
+  base::ScopedFD task_fd(ProcUtil::OpenProcSelfTask());
+  AssertSingleThreaded(task_fd.get());
+}
+
 // static
 bool ThreadHelpers::StopThreadAndWatchProcFS(int proc_self_task,
                                              base::Thread* thread) {
diff --git a/sandbox/linux/services/thread_helpers.h b/sandbox/linux/services/thread_helpers.h
index 88360af..efc2458 100644
--- a/sandbox/linux/services/thread_helpers.h
+++ b/sandbox/linux/services/thread_helpers.h
@@ -15,19 +15,17 @@ namespace sandbox {
 class SANDBOX_EXPORT ThreadHelpers {
  public:
   // Check whether the current process is single threaded. |proc_self_tasks|
-  // can be a file descriptor to /proc/self/task/ and remains owned by the
-  // caller or -1.
-  // If |proc_self_tasks| is -1, this method will open /proc/self/task/ and
-  // crash if it cannot.
+  // must be a file descriptor to /proc/self/task/ and remains owned by the
+  // caller.
   static bool IsSingleThreaded(int proc_self_task);
+  static bool IsSingleThreaded();
 
   // Crash if the current process is not single threaded. This will wait
   // on /proc to be updated. In the case where this doesn't crash, this will
   // return promptly. In the case where this does crash, this will first wait
   // for a few ms in Debug mode, a few seconds in Release mode.
-  // If |proc_self_tasks| is -1, this method will open /proc/self/task/ and
-  // crash if it cannot.
   static void AssertSingleThreaded(int proc_self_task);
+  static void AssertSingleThreaded();
 
   // Stop |thread| and ensure that it does not have an entry in
   // /proc/self/task/ from the point of view of the current thread. This is
diff --git a/sandbox/linux/services/thread_helpers_unittests.cc b/sandbox/linux/services/thread_helpers_unittests.cc
index 86a7a5b..7f99aaf 100644
--- a/sandbox/linux/services/thread_helpers_unittests.cc
+++ b/sandbox/linux/services/thread_helpers_unittests.cc
@@ -57,12 +57,12 @@ class ScopedProcSelfTask {
 TEST(ThreadHelpers, IsSingleThreadedBasic) {
   ScopedProcSelfTask task;
   ASSERT_TRUE(ThreadHelpers::IsSingleThreaded(task.fd()));
-  ASSERT_TRUE(ThreadHelpers::IsSingleThreaded(-1));
+  ASSERT_TRUE(ThreadHelpers::IsSingleThreaded());
 
   base::Thread thread("sandbox_tests");
   ASSERT_TRUE(thread.Start());
   ASSERT_FALSE(ThreadHelpers::IsSingleThreaded(task.fd()));
-  ASSERT_FALSE(ThreadHelpers::IsSingleThreaded(-1));
+  ASSERT_FALSE(ThreadHelpers::IsSingleThreaded());
   // Explicitly stop the thread here to not pollute the next test.
   ASSERT_TRUE(ThreadHelpers::StopThreadAndWatchProcFS(task.fd(), &thread));
 }
@@ -70,10 +70,10 @@ TEST(ThreadHelpers, IsSingleThreadedBasic) {
 SANDBOX_TEST(ThreadHelpers, AssertSingleThreaded) {
   ScopedProcSelfTask task;
   SANDBOX_ASSERT(ThreadHelpers::IsSingleThreaded(task.fd()));
-  SANDBOX_ASSERT(ThreadHelpers::IsSingleThreaded(-1));
+  SANDBOX_ASSERT(ThreadHelpers::IsSingleThreaded());
 
   ThreadHelpers::AssertSingleThreaded(task.fd());
-  ThreadHelpers::AssertSingleThreaded(-1);
+  ThreadHelpers::AssertSingleThreaded();
 }
 
 TEST(ThreadHelpers, IsSingleThreadedIterated) {
@@ -108,7 +108,7 @@ TEST(ThreadHelpers, IsSingleThreadedStartAndStop) {
 }
 
 SANDBOX_TEST(ThreadHelpers, AssertSingleThreadedAfterThreadStopped) {
-  SANDBOX_ASSERT(ThreadHelpers::IsSingleThreaded(-1));
+  SANDBOX_ASSERT(ThreadHelpers::IsSingleThreaded());
 
   base::Thread thread1("sandbox_tests");
   base::Thread thread2("sandbox_tests");
@@ -116,14 +116,14 @@ SANDBOX_TEST(ThreadHelpers, AssertSingleThreadedAfterThreadStopped) {
   for (int i = 0; i < GetRaceTestIterations(); ++i) {
     SANDBOX_ASSERT(thread1.Start());
     SANDBOX_ASSERT(thread2.Start());
-    SANDBOX_ASSERT(!ThreadHelpers::IsSingleThreaded(-1));
+    SANDBOX_ASSERT(!ThreadHelpers::IsSingleThreaded());
 
     thread1.Stop();
     thread2.Stop();
     // This will wait on /proc/ to reflect the state of threads in the
     // process.
-    ThreadHelpers::AssertSingleThreaded(-1);
-    SANDBOX_ASSERT(ThreadHelpers::IsSingleThreaded(-1));
+    ThreadHelpers::AssertSingleThreaded();
+    SANDBOX_ASSERT(ThreadHelpers::IsSingleThreaded());
   }
 }
 
@@ -137,7 +137,7 @@ SANDBOX_DEATH_TEST(
         ThreadHelpers::GetAssertSingleThreadedErrorMessageForTests())) {
   base::Thread thread1("sandbox_tests");
   SANDBOX_ASSERT(thread1.Start());
-  ThreadHelpers::AssertSingleThreaded(-1);
+  ThreadHelpers::AssertSingleThreaded();
 }
 #endif  // !defined(NDEBUG)