Revert "[folly::Subprocess] Set O_CLOEXEC by default when creating pipes to avoid...

Summary: This reverts commit c2f089cf080f2b3effa9efa5e4708b9674437d45.

Test Plan: Compile && folly::Subprocess unit tests

Reviewed By: tudorb@fb.com

FB internal diff: D1327952

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 54f614b59fc94efd22285f1b67d4ec0e293e9f54..b1dd0b3520ce0fc4993f1cc39a8f1031fb3acfe7 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -252,24 +252,24 @@ void Subprocess::spawn(
   });
 
   // Create a pipe to use to receive error information from the child,
-  // in case it fails before calling exec(), setting the close-on-exec flag
-  // on both sides of the pipe.
-  // This way the pipe will be closed automatically in the child if execve()
-  // succeeds.  If the exec fails the child can write error information to the
-  // pipe.
-  // Note that O_CLOEXEC must be set in a single call while we are creating
-  // the pipe instead of doing pipe()/fcntl separately, which might race if a
-  // another thread calls fork()/exec() concurrently and both sides of the pipe
-  // may be inherited by the corresponding child process without being closed.
+  // in case it fails before calling exec()
   int errFds[2];
-  int r = ::pipe2(errFds, O_CLOEXEC);
-  checkUnixError(r, "pipe2");
+  int r = ::pipe(errFds);
+  checkUnixError(r, "pipe");
   SCOPE_EXIT {
     CHECK_ERR(::close(errFds[0]));
     if (errFds[1] >= 0) {
       CHECK_ERR(::close(errFds[1]));
     }
   };
+  // Ask the child to close the read end of the error pipe.
+  options.fdActions_[errFds[0]] = CLOSE;
+  // Set the close-on-exec flag on the write side of the pipe.
+  // This way the pipe will be closed automatically in the child if execve()
+  // succeeds.  If the exec fails the child can write error information to the
+  // pipe.
+  r = fcntl(errFds[1], F_SETFD, FD_CLOEXEC);
+  checkUnixError(r, "set FD_CLOEXEC");
 
   // Perform the actual work of setting up pipes then forking and
   // executing the child.
@@ -316,14 +316,8 @@ void Subprocess::spawnInternal(
   for (auto& p : options.fdActions_) {
     if (p.second == PIPE_IN || p.second == PIPE_OUT) {
       int fds[2];
-      // Set O_CLOEXEC on both ends of the pipe atomically while creating
-      // the pipe. The child will clear O_CLOEXEC on its side of the pipe
-      // before calling exec() so that stays open afterwards.
-      // This way even if a concurrently constructed Subprocess inherits
-      // both ends of this pipe, they will be automatically closed
-      // after the corresponding exec().
-      r = ::pipe2(fds, O_CLOEXEC);
-      checkUnixError(r, "pipe2");
+      r = ::pipe(fds);
+      checkUnixError(r, "pipe");
       PipeInfo pinfo;
       pinfo.direction = p.second;
       int cfd;
@@ -432,13 +426,9 @@ int Subprocess::prepareChild(const Options& options,
     }
   }
 
+  // Close parent's ends of all pipes
   for (auto& p : pipes_) {
-    // Clear FD_CLOEXEC on the child side of the pipe so
-    // it stays open after exec() (so that the child could
-    // access it).
-    // See spawnInternal() for why FD_CLOEXEC must be set
-    // by default on pipes.
-    r = fcntl(p.childFd, F_SETFD, 0);
+    r = ::close(p.parentFd);
     if (r == -1) {
       return errno;
     }

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index b81a1bf1f788c8d508762c5ef2dfd8250ede6130..098b66b3aaf4b721f8caaa61e365a22a3ace2a58 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -19,7 +19,6 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <dirent.h>
-#include <thread>
 
 #include <boost/container/flat_set.hpp>
 #include <glog/logging.h>
@@ -33,7 +32,6 @@
 #include "folly/gen/File.h"
 #include "folly/gen/String.h"
 #include "folly/experimental/io/FsUtil.h"
-#include "folly/Memory.h"
 
 using namespace folly;
 
@@ -438,21 +436,3 @@ TEST(CommunicateSubprocessTest, Chatty) {
     EXPECT_EQ(0, proc.wait().exitStatus());
   });
 }
-
-TEST(ConcurrentSubprocessTest, construction) {
-  std::vector<std::unique_ptr<Subprocess>> ps(100);
-  auto action = [](std::unique_ptr<Subprocess>& p) {
-    p = make_unique<Subprocess>("read", Subprocess::pipeStdout());
-  };
-  std::vector<std::thread> threads;
-  for (auto& p : ps) {
-    threads.emplace_back(action, ref(p));
-  }
-  for (auto& t : threads) {
-    t.join();
-  }
-  for (auto& p : ps) {
-    p->terminate();
-    p->wait();
-  }
-}