Change child's working directory

Summary: Changing the parent's WD is prone to race conditions of various sorts, and needlessly alters parent state. Python's subprocess also has this feature.

Test Plan: fbmake dbg _bin/folly/test/subprocess_test ; ../_bin/folly/test/subprocess_test

Reviewed By: agoder@fb.com

FB internal diff: D1269526

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index b5f3e1dba7ffe6e1afd09e3915e06c9e545761b1..c03a5b29200651fc1401dfd367b09eaf765f9334 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -413,6 +413,14 @@ int Subprocess::prepareChild(const Options& options,
     return r;  // pthread_sigmask() returns an errno value
   }
 
+  // Change the working directory, if one is given
+  if (!options.childDir_.empty()) {
+    r = ::chdir(options.childDir_.c_str());
+    if (r == -1) {
+      return errno;
+    }
+  }
+
   // Close parent's ends of all pipes
   for (auto& p : pipes_) {
     r = ::close(p.parentFd);

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index d3a9fb8ec5240b206845ca52b97ec5e378440249..f189bb49ac44c1bfe32b3459ade79004d935cf80 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -264,6 +264,11 @@ class Subprocess : private boost::noncopyable {
      */
     Options& usePath() { usePath_ = true; return *this; }
 
+    /**
+     * Change the child's working directory, after the vfork.
+     */
+    Options& chdir(const std::string& dir) { childDir_ = dir; return *this; }
+
 #if __linux__
     /**
      * Child will receive a signal when the parent exits.
@@ -284,6 +289,7 @@ class Subprocess : private boost::noncopyable {
     FdMap fdActions_;
     bool closeOtherFds_;
     bool usePath_;
+    std::string childDir_;  // "" keeps the parent's working directory
 #if __linux__
     int parentDeathSignal_{0};
 #endif

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 9be77f2e6a030f4b3b3aee6452d3aa35107a4c9e..53088bb5f8882a79f76189c2465e146820cbd0a6 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -88,6 +88,33 @@ TEST(SimpleSubprocessTest, ShellExitsWithError) {
   EXPECT_EQ(1, proc.wait().exitStatus());
 }
 
+TEST(SimpleSubprocessTest, ChangeChildDirectorySuccessfully) {
+  // The filesystem root normally lacks a 'true' binary
+  EXPECT_EQ(0, chdir("/"));
+  EXPECT_SPAWN_ERROR(ENOENT, "failed to execute ./true", "./true");
+  // The child can fix that by moving to /bin before exec().
+  Subprocess proc("./true", Subprocess::Options().chdir("/bin"));
+  EXPECT_EQ(0, proc.wait().exitStatus());
+}
+
+TEST(SimpleSubprocessTest, ChangeChildDirectoryWithError) {
+  try {
+    Subprocess proc(
+      std::vector<std::string>{"/bin/true"},
+      Subprocess::Options().chdir("/usually/this/is/not/a/valid/directory/")
+    );
+    ADD_FAILURE() << "expected to fail when changing the child's directory";
+  } catch (const SubprocessSpawnError& ex) {
+    EXPECT_EQ(ENOENT, ex.errnoValue());
+    const std::string expectedError =
+      "error preparing to execute /bin/true: No such file or directory";
+    if (StringPiece(ex.what()).find(expectedError) == StringPiece::npos) {
+      ADD_FAILURE() << "failed to find \"" << expectedError <<
+        "\" in exception: \"" << ex.what() << "\"";
+    }
+  }
+}
+
 namespace {
 boost::container::flat_set<int> getOpenFds() {
   auto pid = getpid();