Set O_CLOEXEC by default when creating pipes to avoid race conditions resulting from...

Summary:
[folly::Subprocess] Set O_CLOEXEC by default when creating pipes to avoid race conditions resulting from concurrent Subprocess creations

If multiple threads are creating Subprocess objects concurrently, the
write side file descriptor of the pipe created in the parent process
might be inherited into other child processes unintentionally and never
closed, causing the parent process to hang while reading from the read
side of its pipe, thinking the other side must have been closed.

The fix to the problem is to create the pipes and set O_CLOEXEC in
a single pipe2 call. Then the child could clear the O_CLOEXEC flag
selectively before calling exec().

Test Plan:
Existing unit tests of Subprocess
Added a new unit test which will hang in Subprocess constructor without
this fix.

Reviewed By: tudorb@fb.com

FB internal diff: D1267396

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 9c2b32d0e82d597684111bcb46d13a80e99c0414..3f25f7ffb68d78cc2525b291be95c227142e93fa 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -248,24 +248,24 @@ void Subprocess::spawn(
   });
 
   // Create a pipe to use to receive error information from the child,
-  // in case it fails before calling exec()
+  // 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.
   int errFds[2];
-  int r = ::pipe(errFds);
-  checkUnixError(r, "pipe");
+  int r = ::pipe2(errFds, O_CLOEXEC);
+  checkUnixError(r, "pipe2");
   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.
@@ -312,8 +312,14 @@ void Subprocess::spawnInternal(
   for (auto& p : options.fdActions_) {
     if (p.second == PIPE_IN || p.second == PIPE_OUT) {
       int fds[2];
-      r = ::pipe(fds);
-      checkUnixError(r, "pipe");
+      // 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");
       PipeInfo pinfo;
       pinfo.direction = p.second;
       int cfd;
@@ -422,9 +428,13 @@ int Subprocess::prepareChild(const Options& options,
     }
   }
 
-  // Close parent's ends of all pipes
   for (auto& p : pipes_) {
-    r = ::close(p.parentFd);
+    // 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);
     if (r == -1) {
       return errno;
     }

diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp
index 098b66b3aaf4b721f8caaa61e365a22a3ace2a58..b81a1bf1f788c8d508762c5ef2dfd8250ede6130 100644 (file)
--- a/folly/test/SubprocessTest.cpp
+++ b/folly/test/SubprocessTest.cpp
@@ -19,6 +19,7 @@
 #include <unistd.h>
 #include <sys/types.h>
 #include <dirent.h>
+#include <thread>
 
 #include <boost/container/flat_set.hpp>
 #include <glog/logging.h>
@@ -32,6 +33,7 @@
 #include "folly/gen/File.h"
 #include "folly/gen/String.h"
 #include "folly/experimental/io/FsUtil.h"
+#include "folly/Memory.h"
 
 using namespace folly;
 
@@ -436,3 +438,21 @@ 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();
+  }
+}