From 59030f07f4ff117ddd881d746b3631e434ca2ac2 Mon Sep 17 00:00:00 2001 From: Alexey Spiridonov Date: Fri, 4 Dec 2015 01:35:41 -0800 Subject: [PATCH] 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 --- folly/Subprocess.cpp | 7 +++++ folly/Subprocess.h | 56 +++++++++++++++++++++++++++++++++++ folly/test/SubprocessTest.cpp | 46 ++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+) diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp index b517c55d..2a1c3bce 100644 --- 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 cdd8e38d..531d9181 100644 --- 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 792a9cac..4e0aba6a 100644 --- 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{"/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{"/bin/echo"}, + Subprocess::Options().dangerousPostForkPreExecCallback(&write_cob) + ), + SubprocessSpawnError + ); + EXPECT_FALSE(fs::exists(write_cob.filename_)); +} + TEST(CommunicateSubprocessTest, SimpleRead) { Subprocess proc(std::vector{ "/bin/echo", "-n", "foo", "bar"}, Subprocess::pipeStdout()); -- 2.34.1