Bug 1480401 - Avoid heap-allocated closures in async signal safe part of LaunchApp. r?froydnj draft
authorJed Davis <jld@mozilla.com>
Thu, 02 Aug 2018 14:18:01 -0600
changeset 826155 95e4b610a947924344ca2561d6e69c09f3bd9b61
parent 826154 0165db643e65a68bc83678f19ea3aa4dc608ea3e
push id118250
push userbmo:jld@mozilla.com
push dateFri, 03 Aug 2018 02:24:56 +0000
reviewersfroydnj
bugs1480401
milestone63.0a1
Bug 1480401 - Avoid heap-allocated closures in async signal safe part of LaunchApp. r?froydnj MozReview-Commit-ID: 4LYtBGbqtVh
ipc/chromium/src/base/process_util.h
ipc/chromium/src/base/process_util_linux.cc
ipc/glue/FileDescriptorShuffle.h
--- a/ipc/chromium/src/base/process_util.h
+++ b/ipc/chromium/src/base/process_util.h
@@ -123,16 +123,17 @@ struct LaunchOptions {
 
 #if defined(OS_LINUX) || defined(OS_SOLARIS)
   struct ForkDelegate {
     virtual ~ForkDelegate() { }
     virtual pid_t Fork() = 0;
   };
 
   // If non-null, the fork delegate will be called instead of fork().
+  // It is not required to call pthread_atfork hooks.
   mozilla::UniquePtr<ForkDelegate> fork_delegate = nullptr;
 #endif
 };
 
 #if defined(OS_WIN)
 // Runs the given application name with the given command line. Normally, the
 // first command line argument should be the path to the process, and don't
 // forget to quote it.
--- a/ipc/chromium/src/base/process_util_linux.cc
+++ b/ipc/chromium/src/base/process_util_linux.cc
@@ -32,31 +32,39 @@ bool LaunchApp(const std::vector<std::st
 
   EnvironmentArray envp = BuildEnvironmentArray(options.env_map);
   mozilla::ipc::FileDescriptorShuffle shuffle;
   if (!shuffle.Init(options.fds_to_remap)) {
     return false;
   }
 
   pid_t pid = options.fork_delegate ? options.fork_delegate->Fork() : fork();
+  // WARNING: if pid == 0, only async signal safe operations are permitted from
+  // here until exec or _exit.
+  //
+  // Specifically, heap allocation is not safe: the sandbox's fork substitute
+  // won't run the pthread_atfork handlers that fix up the malloc locks.
+
   if (pid < 0)
     return false;
 
   if (pid == 0) {
     // In the child:
     for (const auto& fds : shuffle.Dup2Sequence()) {
       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);
       }
     }
 
-    CloseSuperfluousFds(shuffle.MapsToFunc());
+    auto fdIsUsed = [&shuffle](int fd) { return shuffle.MapsTo(fd); };
+    // Constructing a std::function from a reference_wrapper won't allocate.
+    CloseSuperfluousFds(std::ref(fdIsUsed));
 
     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/glue/FileDescriptorShuffle.h
+++ b/ipc/glue/FileDescriptorShuffle.h
@@ -45,22 +45,16 @@ public:
   // Accessor for the dup2() sequence.  Do not use the returned value
   // or the fds contained in it after this object is destroyed.
   MappingRef Dup2Sequence() const { return mMapping; }
 
   // Tests whether the given fd is used as a destination in this mapping.
   // Can be used to close other fds after performing the dup2()s.
   bool MapsTo(int aFd) const;
 
-  // Wraps MapsTo in a function object, as a convenience for use with
-  // base::CloseSuperfluousFds.
-  std::function<bool(int)> MapsToFunc() const {
-    return [this](int fd) { return MapsTo(fd); };
-  }
-
 private:
   nsTArray<std::pair<int, int>> mMapping;
   nsTArray<int> mTempFds;
   int mMaxDst;
 
   FileDescriptorShuffle(const FileDescriptorShuffle&) = delete;
   void operator=(const FileDescriptorShuffle&) = delete;
 };