Add an "after fork, before exec" callback

Summary:
In rare cases, it is required to run code **in the child process**, before it calls `exec()` because you need to change the state of the process **before** the child binary gets to run. This diff adds a callback to permit this. The callback is deliberately harder to use than an `std::function`, because you **REALLY HAVE TO KNOW WHAT YOU ARE DOING** to use this. And you have to check your work three times. Even glog `LOG()` must not be called from inside this callback. Your random library of choice is also probably unsafe.

This diff is primarily applicable to job managers and shells. For example, these tasks benefit from this callback: adding a child to a Linux `cgroup`, twiddling its various POSIX process attributes.

Implementing a correct callback for the post-vfork environment is fraught with peril. Read http://ewontfix.com/7/, and the docstrings in this diff.

Reviewed By: yfeldblum

Differential Revision: D2688323

fb-gh-sync-id: aae49e2b3957ca845895acca26e9cb44df1afc07

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index b517c55df33c54377ebb886b4bbd052b964ef69b..2a1c3bce686d4280615aea4f8bdefdcd7b21f62d 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -493,6 +493,13 @@ int Subprocess::prepareChild(const Options& options,
     }
   }
 
+  // The user callback comes last, so that the child is otherwise all set up.
+  if (options.dangerousPostForkPreExecCallback_) {
+    if (int error = (*options.dangerousPostForkPreExecCallback_)()) {
+      return error;
+    }
+  }
+
   return 0;
 }
 

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index cdd8e38d25aeacadd117747e10e60ed710f7d99e..531d9181dd25f3cb38ad33bc7c236cd3c4b2f5d9 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -247,6 +247,23 @@ class Subprocess {
   static const int PIPE_IN = -3;
   static const int PIPE_OUT = -4;
 
+  /**
+   * See Subprocess::Options::dangerousPostForkPreExecCallback() for usage.
+   * Every derived class should include the following warning:
+   *
+   * DANGER: This class runs after fork in a child processes. Be fast, the
+   * parent thread is waiting, but remember that other parent threads are
+   * running and may mutate your state.  Avoid mutating any data belonging to
+   * the parent.  Avoid interacting with non-POD data that originated in the
+   * parent.  Avoid any libraries that may internally reference non-POD data.
+   * Especially beware parent mutexes -- for example, glog's LOG() uses one.
+   */
+  struct DangerousPostForkPreExecCallback {
+    virtual ~DangerousPostForkPreExecCallback() {}
+    // This must return 0 on success, or an `errno` error code.
+    virtual int operator()() = 0;
+  };
+
   /**
    * Class representing various options: file descriptor behavior, and
    * whether to use $PATH for searching for the executable,
@@ -341,6 +358,43 @@ class Subprocess {
       return *this;
     }
 
+    /**
+     * *** READ THIS WHOLE DOCBLOCK BEFORE USING ***
+     *
+     * Run this callback in the child after the fork, just before the
+     * exec(), and after the child's state has been completely set up:
+     *  - signal handlers have been reset to default handling and unblocked
+     *  - the working directory was set
+     *  - closed any file descriptors specified via Options()
+     *  - set child process flags (see code)
+     *
+     * This is EXTREMELY DANGEROUS. For example, this innocuous-looking code
+     * can cause a fraction of your Subprocess launches to hang forever:
+     *
+     *   LOG(INFO) << "Hello from the child";
+     *
+     * The reason is that glog has an internal mutex. If your fork() happens
+     * when the parent has the mutex locked, the child will wait forever.
+     *
+     * == GUIDELINES ==
+     *
+     * - Be quick -- the parent thread is blocked until you exit.
+     * - Remember that other parent threads are running, and may mutate your
+     *   state.
+     * - Avoid mutating any data belonging to the parent.
+     * - Avoid interacting with non-POD data that came from the parent.
+     * - Avoid any libraries that may internally reference non-POD state.
+     * - Especially beware parent mutexes, e.g. LOG() uses a global mutex.
+     * - Avoid invoking the parent's destructors (you can accidentally
+     *   delete files, terminate network connections, etc).
+     * - Read http://ewontfix.com/7/
+     */
+    Options& dangerousPostForkPreExecCallback(
+        DangerousPostForkPreExecCallback* cob) {
+      dangerousPostForkPreExecCallback_ = cob;
+      return *this;
+    }
+
     /**
      * Helpful way to combine Options.
      */
@@ -356,6 +410,8 @@ class Subprocess {
     int parentDeathSignal_{0};
 #endif
     bool processGroupLeader_{false};
+    DangerousPostForkPreExecCallback*
+      dangerousPostForkPreExecCallback_{nullptr};
   };
 
   static Options pipeStdin() { return Options().stdin(PIPE); }

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 792a9cac432b53d1fdc23b569c6075db756ca238..4e0aba6ae4aaa873287c2d935441d41cc6307162 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -232,6 +232,52 @@ TEST(PopenSubprocessTest, PopenRead) {
   proc.waitChecked();
 }
 
+// DANGER: This class runs after fork in a child processes. Be fast, the
+// parent thread is waiting, but remember that other parent threads are
+// running and may mutate your state.  Avoid mutating any data belonging to
+// the parent.  Avoid interacting with non-POD data that originated in the
+// parent.  Avoid any libraries that may internally reference non-POD data.
+// Especially beware parent mutexes -- for example, glog's LOG() uses one.
+struct WriteFileAfterFork
+    : public Subprocess::DangerousPostForkPreExecCallback {
+  explicit WriteFileAfterFork(std::string filename)
+    : filename_(std::move(filename)) {}
+  virtual ~WriteFileAfterFork() {}
+  int operator()() override {
+    return writeFile(std::string("ok"), filename_.c_str()) ? 0 : errno;
+  }
+  const std::string filename_;
+};
+
+TEST(AfterForkCallbackSubprocessTest, TestAfterForkCallbackSuccess) {
+  test::ChangeToTempDir td;
+  // Trigger a file write from the child.
+  WriteFileAfterFork write_cob("good_file");
+  Subprocess proc(
+    std::vector<std::string>{"/bin/echo"},
+    Subprocess::Options().dangerousPostForkPreExecCallback(&write_cob)
+  );
+  // The file gets written immediately.
+  std::string s;
+  EXPECT_TRUE(readFile(write_cob.filename_.c_str(), s));
+  EXPECT_EQ("ok", s);
+  proc.waitChecked();
+}
+
+TEST(AfterForkCallbackSubprocessTest, TestAfterForkCallbackError) {
+  test::ChangeToTempDir td;
+  // The child will try to write to a file, whose directory does not exist.
+  WriteFileAfterFork write_cob("bad/file");
+  EXPECT_THROW(
+    Subprocess proc(
+      std::vector<std::string>{"/bin/echo"},
+      Subprocess::Options().dangerousPostForkPreExecCallback(&write_cob)
+    ),
+    SubprocessSpawnError
+  );
+  EXPECT_FALSE(fs::exists(write_cob.filename_));
+}
+
 TEST(CommunicateSubprocessTest, SimpleRead) {
   Subprocess proc(std::vector<std::string>{ "/bin/echo", "-n", "foo", "bar"},
                   Subprocess::pipeStdout());