Use pipe2 in Subprocess; platform-specific config

Summary:
We've wanted to use pipe2 in Subprocess for a while, but that's not supported
on glibc 2.5.1. This is not a problem in OSS (autoconf can solve this),
but, internally, we don't run autoconf; add an internal platform include file
with such per-platform differences.

Test Plan: fbconfig -r folly && fbmake runtests_opt

Reviewed By: meyering@fb.com

Subscribers: jhj, lesha, kma, agallagher

FB internal diff: D1422128

diff --git a/folly/Bits.h b/folly/Bits.h
index 491fcf70007521707a9198e7be2a113ea94b48ce..ade126d0803b6920c7fdd8dd1c30fd0009308811 100644 (file)
--- a/folly/Bits.h
+++ b/folly/Bits.h
@@ -68,9 +68,7 @@
 #define FOLLY_INTRINSIC_CONSTEXPR const
 #endif
 
-#ifndef FOLLY_NO_CONFIG
-#include <folly/folly-config.h>
-#endif
+#include <folly/Portability.h>
 
 #include <folly/detail/BitsDetail.h>
 #include <folly/detail/BitIteratorDetail.h>

diff --git a/folly/Portability.h b/folly/Portability.h
index 51642a1c7649e9869c93604a3b3c030b0f1c71dc..9c65c650c14f2b68496028ffedbc13d185902e3d 100644 (file)
--- a/folly/Portability.h
+++ b/folly/Portability.h
@@ -21,6 +21,10 @@
 #include "folly-config.h"
 #endif
 
+#ifdef FOLLY_PLATFORM_CONFIG
+#include FOLLY_PLATFORM_CONFIG
+#endif
+
 #if FOLLY_HAVE_FEATURES_H
 #include <features.h>
 #endif

diff --git a/folly/Subprocess.cpp b/folly/Subprocess.cpp
index 08d93b4a8fd9ae3af58bafba27fef9aaaabd30da..ac8eab60d7a1423c8db0af85ef0b0998885c870b 100644 (file)
--- a/folly/Subprocess.cpp
+++ b/folly/Subprocess.cpp
@@ -14,6 +14,10 @@
  * limitations under the License.
  */
 
+#ifndef _GNU_SOURCE
+#define _GNU_SOURCE
+#endif
+
 #include <folly/Subprocess.h>
 
 #if __linux__
@@ -22,9 +26,6 @@
 #include <fcntl.h>
 #include <poll.h>
 
-#ifndef _GNU_SOURCE
-#define _GNU_SOURCE
-#endif
 #include <unistd.h>
 
 #include <array>
@@ -254,7 +255,11 @@ void Subprocess::spawn(
   // Create a pipe to use to receive error information from the child,
   // in case it fails before calling exec()
   int errFds[2];
+#if FOLLY_HAVE_PIPE2
+  int r = ::pipe2(errFds, O_CLOEXEC);
+#else
   int r = ::pipe(errFds);
+#endif
   checkUnixError(r, "pipe");
   SCOPE_EXIT {
     CHECK_ERR(::close(errFds[0]));
@@ -264,12 +269,17 @@ void Subprocess::spawn(
   };
   // Ask the child to close the read end of the error pipe.
   options.fdActions_[errFds[0]] = CLOSE;
+
+#if !FOLLY_HAVE_PIPE2
+  r = fcntl(errFds[0], F_SETFD, FD_CLOEXEC);
+  checkUnixError(r, "set FD_CLOEXEC");
   // 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");
+#endif
 
   // Perform the actual work of setting up pipes then forking and
   // executing the child.
@@ -316,8 +326,20 @@ void Subprocess::spawnInternal(
   for (auto& p : options.fdActions_) {
     if (p.second == PIPE_IN || p.second == PIPE_OUT) {
       int fds[2];
+      // We're setting both ends of the pipe as close-on-exec. The child
+      // doesn't need to reset the flag on its end, as we always dup2() the fd,
+      // and dup2() fds don't share the close-on-exec flag.
+#if FOLLY_HAVE_PIPE2
+      r = ::pipe2(fds, O_CLOEXEC);
+      checkUnixError(r, "pipe2");
+#else
       r = ::pipe(fds);
       checkUnixError(r, "pipe");
+      r = fcntl(fds[0], F_SETFD, FD_CLOEXEC);
+      checkUnixError(r, "set FD_CLOEXEC");
+      r = fcntl(fds[1], F_SETFD, FD_CLOEXEC);
+      checkUnixError(r, "set FD_CLOEXEC");
+#endif
       PipeInfo pinfo;
       pinfo.direction = p.second;
       int cfd;
@@ -380,9 +402,12 @@ void Subprocess::spawnInternal(
     CHECK_EQ(r, 0) << "pthread_sigmask: " << errnoStr(r);  // shouldn't fail
   };
 
+  // Call c_str() here, as it's not necessarily safe after fork.
+  const char* childDir =
+    options.childDir_.empty() ? nullptr : options.childDir_.c_str();
   pid_t pid = vfork();
   if (pid == 0) {
-    int errnoValue = prepareChild(options, &oldSignals);
+    int errnoValue = prepareChild(options, &oldSignals, childDir);
     if (errnoValue != 0) {
       childError(errFd, kChildFailure, errnoValue);
     }
@@ -406,43 +431,44 @@ void Subprocess::spawnInternal(
 }
 
 int Subprocess::prepareChild(const Options& options,
-                             const sigset_t* sigmask) const {
+                             const sigset_t* sigmask,
+                             const char* childDir) const {
   // While all signals are blocked, we must reset their
   // dispositions to default.
   for (int sig = 1; sig < NSIG; ++sig) {
     ::signal(sig, SIG_DFL);
   }
-  // Unblock signals; restore signal mask.
-  int r = pthread_sigmask(SIG_SETMASK, sigmask, nullptr);
-  if (r != 0) {
-    return r;  // pthread_sigmask() returns an errno value
+
+  {
+    // Unblock signals; restore signal mask.
+    int r = pthread_sigmask(SIG_SETMASK, sigmask, nullptr);
+    if (r != 0) {
+      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) {
+  if (childDir) {
+    if (::chdir(childDir) == -1) {
       return errno;
     }
   }
 
   // Close parent's ends of all pipes
   for (auto& p : pipes_) {
-    r = ::close(p.parentFd);
-    if (r == -1) {
+    if (::close(p.parentFd) == -1) {
       return errno;
     }
   }
 
   // Close all fds that we're supposed to close.
-  // Note that we're ignoring errors here, in case some of these
-  // fds were set to close on exec.
   for (auto& p : options.fdActions_) {
     if (p.second == CLOSE) {
-      ::close(p.first);
-    } else {
-      r = ::dup2(p.second, p.first);
-      if (r == -1) {
+      if (::close(p.first) == -1) {
+        return errno;
+      }
+    } else if (p.second != p.first) {
+      if (::dup2(p.second, p.first) == -1) {
         return errno;
       }
     }
@@ -462,8 +488,7 @@ int Subprocess::prepareChild(const Options& options,
 #if __linux__
   // Opt to receive signal on parent death, if requested
   if (options.parentDeathSignal_ != 0) {
-    r = prctl(PR_SET_PDEATHSIG, options.parentDeathSignal_, 0, 0, 0);
-    if (r == -1) {
+    if (prctl(PR_SET_PDEATHSIG, options.parentDeathSignal_, 0, 0, 0) == -1) {
       return errno;
     }
   }

diff --git a/folly/Subprocess.h b/folly/Subprocess.h
index e1fed9f7beb6a1ef4c4a28309f7b6a9354047840..5acf6235aa0bfc585eb291f0d5f91999a2cde0d6 100644 (file)
--- a/folly/Subprocess.h
+++ b/folly/Subprocess.h
@@ -500,7 +500,9 @@ class Subprocess : private boost::noncopyable {
   // Actions to run in child.
   // Note that this runs after vfork(), so tread lightly.
   // Returns 0 on success, or an errno value on failure.
-  int prepareChild(const Options& options, const sigset_t* sigmask) const;
+  int prepareChild(const Options& options,
+                   const sigset_t* sigmask,
+                   const char* childDir) const;
   int runChild(const char* executable, char** argv, char** env,
                const Options& options) const;
 

diff --git a/folly/configure.ac b/folly/configure.ac
index d5465a2727fb7db4f4ad3da7f618548b0d2b0f1c..774cb5b46afdbfe18bf7e8cef93cac1d741013f0 100644 (file)
--- a/folly/configure.ac
+++ b/folly/configure.ac
@@ -271,7 +271,8 @@ AC_CHECK_FUNCS([getdelim \
                 rallocm \
                 malloc_size \
                 malloc_usable_size \
-                memrchr])
+                memrchr \
+                pipe2])
 
 if test "$ac_cv_func_pthread_yield" = "no"; then
    AC_CHECK_HEADERS([sched.h])

diff --git a/folly/detail/Clock.h b/folly/detail/Clock.h
index 34a61b65a37cf8528e935937bfbd783f4fa900b1..fc4ffd051ea5d3c9ad57fcc4b4249592b14ce61b 100644 (file)
--- a/folly/detail/Clock.h
+++ b/folly/detail/Clock.h
@@ -20,9 +20,7 @@
 #include <ctime>
 #include <cstdint>
 
-#ifndef FOLLY_NO_CONFIG
-#include <folly/folly-config.h>
-#endif
+#include <folly/Portability.h>
 
 #if FOLLY_HAVE_CLOCK_GETTIME
 #error This should only be used as a workaround for platforms \

diff --git a/folly/detail/Malloc.h b/folly/detail/Malloc.h
index 6b045b44d2d31f59220d541dc1954f22c9c0b754..6a6298a7bd1b93336a26bceb280cab091fe77081 100644 (file)
--- a/folly/detail/Malloc.h
+++ b/folly/detail/Malloc.h
@@ -19,9 +19,7 @@
 
 #include <stdlib.h>
 
-#ifndef FOLLY_NO_CONFIG
-#include <folly/folly-config.h>
-#endif
+#include <folly/Portability.h>
 
 extern "C" {