From 20fe8fea93ad2165d3f44453c648179c6ed6bd53 Mon Sep 17 00:00:00 2001
From: Karen Arutyunov <karen@codesynthesis.com>
Date: Sat, 28 Jan 2017 01:06:36 +0300
Subject: Make fdopen_pipe(), fdopen(), fdnull() and fddup() to set FD_CLOEXEC
 flag

---
 butl/fdstream              | 20 ++++++++++++++++++++
 butl/fdstream.cxx          | 23 +++++++++++++++++++++--
 butl/process.cxx           |  2 +-
 tests/timestamp/driver.cxx |  2 +-
 4 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/butl/fdstream b/butl/fdstream
index 8d01b2f..8d6385f 100644
--- a/butl/fdstream
+++ b/butl/fdstream
@@ -486,6 +486,9 @@ namespace butl
   // process' umask, so effective permissions are permissions & ~umask. On
   // Windows permissions other than ru and wu are unlikelly to have effect.
   //
+  // Also note that on POSIX the FD_CLOEXEC flag is set for the file descriptor
+  // to prevent its leakage into child processes.
+  //
   LIBBUTL_EXPORT auto_fd
   fdopen (const char*,
           fdopen_mode,
@@ -510,6 +513,13 @@ namespace butl
   // Duplicate an open file descriptor. Throw ios::failure on the underlying
   // OS error.
   //
+  // Note that on POSIX the FD_CLOEXEC flag is set for the new descriptor if it
+  // is present for the source one. That's in contrast to POSIX dup() that
+  // doesn't copy file descriptor flags. Also note that duplicating descriptor
+  // and setting the flag is not an atomic operation.
+  //
+  // @@ Should we copy HANDLE_FLAG_INHERIT flag on Windows as well?
+  //
   LIBBUTL_EXPORT auto_fd
   fddup (int fd);
 
@@ -563,6 +573,9 @@ namespace butl
   // closed. One difference, however, would be if you were to both write to
   // and read from the descriptor.
   //
+  // Note that on POSIX the FD_CLOEXEC flag is set for the file descriptor to
+  // prevent its leakage into child processes.
+  //
   // @@ You may have noticed that this interface doesn't match fdopen() (it
   //    does not throw and return int instead of auto_fd). The reason for this
   //    is process and its need to translate everything to process_error).
@@ -591,6 +604,13 @@ namespace butl
   // In particular, the process class that uses fdpipe underneath makes the
   // appropriate end of pipe (the one being passed to the child) inheritable.
   //
+  // Note that on POSIX the FD_CLOEXEC flag is set for both ends, so they get
+  // automatically closed by the child process to prevent undesired behaviors
+  // (such as child deadlock on read from a pipe due to the write-end leakage
+  // into the child process). Opening pipe and setting the flag is not an
+  // atomic operation. Also note that you don't need to reset the flag for a
+  // pipe end being passed to the process class ctor.
+  //
   LIBBUTL_EXPORT fdpipe
   fdopen_pipe (fdopen_mode = fdopen_mode::none);
 }
diff --git a/butl/fdstream.cxx b/butl/fdstream.cxx
index 370a75e..e28eb0f 100644
--- a/butl/fdstream.cxx
+++ b/butl/fdstream.cxx
@@ -658,7 +658,7 @@ namespace butl
     of |= O_LARGEFILE;
 #endif
 
-    int fd (open (f, of, pf));
+    int fd (open (f, of | O_CLOEXEC, pf));
 
 #else
 
@@ -745,6 +745,18 @@ namespace butl
   {
 #ifndef _WIN32
     int nfd (dup (fd));
+
+    int f (fcntl (fd, F_GETFD));
+    if (f == -1)
+      throw_ios_failure (errno);
+
+    if ((f & FD_CLOEXEC) != 0)
+    {
+      f = fcntl (nfd, F_GETFD);
+      if (f == -1 || fcntl (nfd, F_SETFD, f | FD_CLOEXEC) == -1)
+        throw_ios_failure (errno);
+    }
+
 #else
     int nfd (_dup (fd));
 #endif
@@ -766,7 +778,7 @@ namespace butl
   int
   fdnull () noexcept
   {
-    return open ("/dev/null", O_RDWR);
+    return open ("/dev/null", O_RDWR | O_CLOEXEC);
   }
 
   fdstream_mode
@@ -829,6 +841,13 @@ namespace butl
     if (pipe (pd) == -1)
       throw_ios_failure (errno);
 
+    for (size_t i (0); i < 2; ++i)
+    {
+      int f (fcntl (pd[i], F_GETFD));
+      if (f == -1 || fcntl (pd[i], F_SETFD, f | FD_CLOEXEC) == -1)
+        throw_ios_failure (errno);
+    }
+
     return {auto_fd (pd[0]), auto_fd (pd[1])};
   }
 
diff --git a/butl/process.cxx b/butl/process.cxx
index 526966e..892e930 100644
--- a/butl/process.cxx
+++ b/butl/process.cxx
@@ -865,7 +865,7 @@ namespace butl
     // Resolve file descriptor to HANDLE and make sure it is inherited. Note
     // that the handle is closed either when CloseHandle() is called for it or
     // when _close() is called for the associated file descriptor. Make sure
-    // that either the original file descriptor or the resulted HANDLE is
+    // that either the original file descriptor or the resulting HANDLE is
     // closed but not both of them.
     //
     auto get_osfhandle = [&fail](int fd) -> HANDLE
diff --git a/tests/timestamp/driver.cxx b/tests/timestamp/driver.cxx
index 41cea44..9161662 100644
--- a/tests/timestamp/driver.cxx
+++ b/tests/timestamp/driver.cxx
@@ -17,7 +17,7 @@
 using namespace std;
 using namespace butl;
 
-// Parse the input using the format string. Print the resulted time with the
+// Parse the input using the format string. Print the resulting time with the
 // same format string, ensure the output matches the input.
 //
 static bool
-- 
cgit v1.1