From 870912b89844e07283ff6d7e9602952f05ad3840 Mon Sep 17 00:00:00 2001
From: Rocky Liu <rockyliu@fb.com>
Date: Tue, 13 May 2014 11:43:59 -0700
Subject: [PATCH] Set O_CLOEXEC by default when creating pipes to avoid race
 conditions resulting from concurrent Subprocess creations

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
---
 folly/Subprocess.cpp          | 40 ++++++++++++++++++++++-------------
 folly/test/SubprocessTest.cpp | 20 ++++++++++++++++++
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 9c2b32d0..3f25f7ff 100644
--- 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 098b66b3..b81a1bf1 100644
--- 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();
+  }
+}
-- 
2.34.1