Convert DropFileSystemAccess to use ForkWithFlags.

sandbox/linux/services/credentials.cc

Index: sandbox/linux/services/credentials.cc
diff --git a/sandbox/linux/services/credentials.cc b/sandbox/linux/services/credentials.cc
index 06e1a6454218eff80669fea9b2591ae492d181b1..7ac1c8df782acc8e0ae60453ed2c123ed72c8ea4 100644
--- a/sandbox/linux/services/credentials.cc
+++ b/sandbox/linux/services/credentials.cc
@@ -15,12 +15,13 @@
 
 #include "base/basictypes.h"
 #include "base/bind.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/posix/eintr_wrapper.h"
-#include "base/strings/string_number_conversions.h"
+#include "base/process/process.h"
 #include "base/template_util.h"
 #include "base/third_party/valgrind/valgrind.h"
-#include "base/threading/thread.h"
 #include "sandbox/linux/services/syscall_wrappers.h"
 
 namespace {
@@ -96,51 +97,38 @@ bool GetRESIds(uid_t* resuid, gid_t* resgid) {
   return true;
 }
 
-// chroot() and chdir() to /proc/<tid>/fdinfo.
-void ChrootToThreadFdInfo(base::PlatformThreadId tid, bool* result) {
-  DCHECK(result);
-  *result = false;
-
-  static_assert((base::is_same<base::PlatformThreadId, int>::value),
-                "platform thread id should be an int");
-  const std::string current_thread_fdinfo = "/proc/" +
-      base::IntToString(tid) + "/fdinfo/";
-
-  // Make extra sure that /proc/<tid>/fdinfo is unique to the thread.
-  CHECK(0 == unshare(CLONE_FILES));
-  int chroot_ret = chroot(current_thread_fdinfo.c_str());
-  if (chroot_ret) {
-    PLOG(ERROR) << "Could not chroot";
-    return;
-  }
-
-  // CWD is essentially an implicit file descriptor, so be careful to not leave
-  // it behind.
-  PCHECK(0 == chdir("/"));
-
-  *result = true;
-  return;
-}
-
 // chroot() to an empty dir that is "safe". To be safe, it must not contain
 // any subdirectory (chroot-ing there would allow a chroot escape) and it must
 // be impossible to create an empty directory there.
 // We achieve this by doing the following:
-// 1. We create a new thread, which will create a new /proc/<tid>/ directory
-// 2. We chroot to /proc/<tid>/fdinfo/
+// 1. We create a new process sharing file system information.
+// 2. In the child, we chroot to /proc/self/fdinfo/
 // This is already "safe", since fdinfo/ does not contain another directory and
 // one cannot create another directory there.
-// 3. The thread dies
+// 3. The process dies
 // After (3) happens, the directory is not available anymore in /proc.
 bool ChrootToSafeEmptyDir() {
-  base::Thread chrooter("sandbox_chrooter");
-  if (!chrooter.Start()) return false;
-  bool is_chrooted = false;
-  chrooter.message_loop()->PostTask(FROM_HERE,
-      base::Bind(&ChrootToThreadFdInfo, chrooter.thread_id(), &is_chrooted));
-  // Make sure our task has run before committing the return value.
-  chrooter.Stop();
-  return is_chrooted;
+  // We do not use a thread because when we are in a PID namespace, we cannot
+  // easily get a handle to the /proc/tid directory for the thread (since /proc
+  // may not be aware of the PID namespace). With a process, we can just use
+  // /proc/self.
+  pid_t pid = base::ForkWithFlags(SIGCHLD | CLONE_FS, nullptr, nullptr);

[jln (very slow on Chromium), 2015/01/15 19:29:44]

This will be quite slow (we need to duplicate all the page tables), if it
becomes a problem we should consider VFORK.

[rickyz (no longer on Chrome), 2015/01/15 22:19:05]

Acknowledged. If processes being expensive ends up being an issue, we could
still use a thread along with some hacks to get the PID of the thread outside of
the namespace - the ideal solution would have been to use /proc/thread-self, but
that was unfortunately only added in 3.17.

+  PCHECK(pid != -1);
+  if (pid == 0) {
+    // Make extra sure that /proc/self/fdinfo is unique to the process.
+    RAW_CHECK(unshare(CLONE_FILES) == 0);

[jln (very slow on Chromium), 2015/01/15 19:29:44]

I don't think that this is required given that it's not a thread (and you're not
sharing CLONE_FILES in ForkWithFlags()

[rickyz (no longer on Chrome), 2015/01/15 22:19:05]

Good point, removed.

+    RAW_CHECK(chroot("/proc/self/fdinfo") == 0);
+
+    // CWD is essentially an implicit file descriptor, so be careful to not
+    // leave it behind.
+    RAW_CHECK(chdir("/") == 0);
+    _exit(0);
+  }
+
+  int status;
+  PCHECK(HANDLE_EINTR(waitpid(pid, &status, 0)) == pid);
+
+  return status == 0;
 }
 
 // CHECK() that an attempt to move to a new user namespace raised an expected
@@ -243,7 +231,8 @@ bool Credentials::MoveToNewUserNS() {
 }
 
 bool Credentials::DropFileSystemAccess() {
-  return ChrootToSafeEmptyDir();
+  return ChrootToSafeEmptyDir() &&

[jln (very slow on Chromium), 2015/01/15 19:29:44]

If ChrootToSafeEmptyDir() succeeds, it's critical that
base::DirectoryExists(base::FilePath("/proc")); fails, or we have a pretty awful
bug.

How about a CHECK or DCHECK instead of simply returning false?

[rickyz (no longer on Chrome), 2015/01/15 22:19:05]

Done.

+         !base::DirectoryExists(base::FilePath("/proc"));
 }
 
 }  // namespace sandbox.