Bug 1481978 - Change IPC CloseSuperfluousFds to prevent accidentally heap-allocating closures. r=glandium
authorJed Davis <jld@mozilla.com>
Wed, 15 Aug 2018 19:08:40 -0600
changeset 486941 542197624ba293765e206322c3e828242d2fd875
parent 486940 e19fc7991514a647468e3b80c798db58b06cf15b
child 486942 7c3423f706f79e09da412dd8be1ea4a58a5265aa
push id9719
push userffxbld-merge
push dateFri, 24 Aug 2018 17:49:46 +0000
treeherdermozilla-beta@719ec98fba77 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersglandium
bugs1481978
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1481978 - Change IPC CloseSuperfluousFds to prevent accidentally heap-allocating closures. r=glandium Closures are nice but -- as pointed out in bug 1481978 comment #2 -- it's a footgun to take a std::function argument in a context where heap allocation isn't safe. Fortunately, non-capturing closures convert to C function pointers, so a C-style interface with a void* context can still be relatively ergonomic.
ipc/chromium/src/base/process_util.h
ipc/chromium/src/base/process_util_linux.cc
ipc/chromium/src/base/process_util_posix.cc
security/sandbox/linux/launch/SandboxLaunch.cpp
--- a/ipc/chromium/src/base/process_util.h
+++ b/ipc/chromium/src/base/process_util.h
@@ -88,17 +88,17 @@ ProcessId GetProcId(ProcessHandle proces
 // TODO(agl): remove this function
 // WARNING: do not use. It's inherently race-prone in the face of
 // multi-threading.
 void SetAllFDsToCloseOnExec();
 // Close all file descriptors, except for std{in,out,err} and those
 // for which the given function returns true.  Only call this function
 // in a child process where you know that there aren't any other
 // threads.
-void CloseSuperfluousFds(std::function<bool(int)>&& should_preserve);
+void CloseSuperfluousFds(void* aCtx, bool (*aShouldPreserve)(void*, int));
 
 typedef std::vector<std::pair<int, int> > file_handle_mapping_vector;
 typedef std::map<std::string, std::string> environment_map;
 #endif
 
 struct LaunchOptions {
   // If true, wait for the process to terminate.  Otherwise, return
   // immediately.
--- a/ipc/chromium/src/base/process_util_linux.cc
+++ b/ipc/chromium/src/base/process_util_linux.cc
@@ -52,19 +52,19 @@ bool LaunchApp(const std::vector<std::st
       if (HANDLE_EINTR(dup2(fds.first, fds.second)) != fds.second) {
         // This shouldn't happen, but check for it.  And see below
         // about logging being unsafe here, so this is debug only.
         DLOG(ERROR) << "dup2 failed";
         _exit(127);
       }
     }
 
-    auto fdIsUsed = [&shuffle](int fd) { return shuffle.MapsTo(fd); };
-    // Constructing a std::function from a reference_wrapper won't allocate.
-    CloseSuperfluousFds(std::ref(fdIsUsed));
+    CloseSuperfluousFds(&shuffle, [](void* aCtx, int aFd) {
+      return static_cast<decltype(&shuffle)>(aCtx)->MapsTo(aFd);
+    });
 
     for (size_t i = 0; i < argv.size(); i++)
       argv_cstr[i] = const_cast<char*>(argv[i].c_str());
     argv_cstr[argv.size()] = NULL;
 
     execve(argv_cstr[0], argv_cstr.get(), envp.get());
     // if we get here, we're in serious trouble and should complain loudly
     // NOTE: This is async signal unsafe; it could deadlock instead.  (But
--- a/ipc/chromium/src/base/process_util_posix.cc
+++ b/ipc/chromium/src/base/process_util_posix.cc
@@ -116,17 +116,18 @@ class ScopedDIRClose {
     if (x) {
       closedir(x);
     }
   }
 };
 typedef mozilla::UniquePtr<DIR, ScopedDIRClose> ScopedDIR;
 
 
-void CloseSuperfluousFds(std::function<bool(int)>&& should_preserve) {
+void CloseSuperfluousFds(void* aCtx, bool (*aShouldPreserve)(void*, int))
+{
   // DANGER: no calls to malloc (or locks, etc.) are allowed from now on:
   // https://crbug.com/36678
   // Also, beware of STL iterators: https://crbug.com/331459
 #if defined(ANDROID)
   static const rlim_t kSystemDefaultMaxFds = 1024;
   static const char kFDDir[] = "/proc/self/fd";
 #elif defined(OS_LINUX) || defined(OS_SOLARIS)
   static const rlim_t kSystemDefaultMaxFds = 8192;
@@ -157,17 +158,17 @@ void CloseSuperfluousFds(std::function<b
 
   DirReaderPosix fd_dir(kFDDir);
 
   if (!fd_dir.IsValid()) {
     // Fallback case: Try every possible fd.
     for (rlim_t i = 0; i < max_fds; ++i) {
       const int fd = static_cast<int>(i);
       if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO ||
-          should_preserve(fd)) {
+          aShouldPreserve(aCtx, fd)) {
         continue;
       }
 
       // Since we're just trying to close anything we can find,
       // ignore any error return values of close().
       close(fd);
     }
     return;
@@ -183,17 +184,17 @@ void CloseSuperfluousFds(std::function<b
     char *endptr;
     errno = 0;
     const long int fd = strtol(fd_dir.name(), &endptr, 10);
     if (fd_dir.name()[0] == 0 || *endptr || fd < 0 || errno)
       continue;
     if (fd == dir_fd)
       continue;
     if (fd == STDIN_FILENO || fd == STDOUT_FILENO || fd == STDERR_FILENO ||
-        should_preserve(fd)) {
+        aShouldPreserve(aCtx, fd)) {
       continue;
     }
 
     // When running under Valgrind, Valgrind opens several FDs for its
     // own use and will complain if we try to close them.  All of
     // these FDs are >= |max_fds|, so we can check against that here
     // before closing.  See https://bugs.kde.org/show_bug.cgi?id=191758
     if (fd < static_cast<int>(max_fds)) {
--- a/security/sandbox/linux/launch/SandboxLaunch.cpp
+++ b/security/sandbox/linux/launch/SandboxLaunch.cpp
@@ -585,17 +585,19 @@ SandboxFork::StartChrootServer()
 
   LinuxCapabilities caps;
   caps.Effective(CAP_SYS_CHROOT) = true;
   if (!caps.SetCurrent()) {
     SANDBOX_LOG_ERROR("capset (chroot helper): %s", strerror(errno));
     MOZ_DIAGNOSTIC_ASSERT(false);
   }
 
-  base::CloseSuperfluousFds([this](int fd) { return fd == mChrootServer; });
+  base::CloseSuperfluousFds(this, [](void* aCtx, int aFd) {
+    return aFd == static_cast<decltype(this)>(aCtx)->mChrootServer;
+  });
 
   char msg;
   ssize_t msgLen = HANDLE_EINTR(read(mChrootServer, &msg, 1));
   if (msgLen == 0) {
     // Process exited before chrooting (or chose not to chroot?).
     _exit(0);
   }
   MOZ_RELEASE_ASSERT(msgLen == 1);