Fix file descriptors leakage to child process on Windows

5 files changed, 373 insertions, 156 deletions

diff --git a/butl/buildfile b/butl/buildfile
index 090f9ed..0aa0aae 100644
--- a/butl/buildfile
+++ b/butl/buildfile
@@ -2,7 +2,7 @@
 # copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
 # license   : MIT; see accompanying LICENSE file
 
-lib{butl}:                               \
+lib{butl}:                                  \
    {hxx         cxx}{ base64              } \
    {hxx         cxx}{ char-scanner        } \
    {hxx            }{ const-ptr           } \
@@ -21,6 +21,7 @@ lib{butl}:                               \
    {hxx            }{ path-map            } \
    {hxx     txx    }{ prefix-map          } \
    {hxx ixx     cxx}{ process             } \
+   {hxx            }{ process-details     } \
    {hxx         cxx}{ sha256              } \
    {hxx            }{ small-vector        } \
    {hxx     txx    }{ string-table        } \
@@ -66,6 +67,9 @@ lib{butl}: cxx.export.poptions = "-I$src_root"
 liba{butl}: cxx.export.poptions += -DLIBBUTL_STATIC
 libs{butl}: cxx.export.poptions += -DLIBBUTL_SHARED
 
+if ($cxx.target.class != "windows")
+  cxx.libs += -lpthread
+
 # Install into the butl/ subdirectory of, say, /usr/include/.
 #
 install.include = $install.include/butl/
diff --git a/butl/fdstream b/butl/fdstream
index 8d6385f..6697dd9 100644
--- a/butl/fdstream
+++ b/butl/fdstream
@@ -487,7 +487,9 @@ namespace butl
   // 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.
+  // to prevent its leakage into child processes. On Windows, for the same
+  // purpose, the _O_NOINHERIT flag is set. Note that the process class, that
+  // passes such a descriptor to the child, makes it inheritable for a while.
   //
   LIBBUTL_EXPORT auto_fd
   fdopen (const char*,
@@ -516,9 +518,16 @@ namespace butl
   // 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.
+  // and setting the flag is not an atomic operation generally, but it is in
+  // regards to child process spawning (to prevent file descriptor leakage into
+  // a child process).
   //
-  // @@ Should we copy HANDLE_FLAG_INHERIT flag on Windows as well?
+  // Note that on Windows the _O_NOINHERIT flag is set for the new descriptor
+  // if it is present for the source one. That's in contrast to Windows _dup()
+  // that doesn't copy the flag. Also note that duplicating descriptor and
+  // setting the flag is not an atomic operation generally, but it is in
+  // regards to child process spawning (to prevent file descriptor leakage into
+  // a child process).
   //
   LIBBUTL_EXPORT auto_fd
   fddup (int fd);
@@ -608,8 +617,10 @@ namespace butl
   // 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.
+  // atomic operation generally, but it is in regards to child process spawning
+  // (to prevent file descriptor leakage into child processes spawned from
+  // other threads). 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 bdbfa08..41dd1f5 100644
--- a/butl/fdstream.cxx
+++ b/butl/fdstream.cxx
@@ -12,8 +12,10 @@
 #  include <sys/stat.h>  // S_I*
 #  include <sys/types.h> // off_t
 #else
+#  include <butl/win32-utility>
+
 #  include <io.h>       // _close(), _read(), _write(), _setmode(), _sopen(),
-                        // _lseek(), _dup(), _pipe()
+                        // _lseek(), _dup(), _pipe(), _get_osfhandle()
 #  include <share.h>    // _SH_DENYNO
 #  include <stdio.h>    // _fileno(), stdin, stdout, stderr
 #  include <fcntl.h>    // _O_*
@@ -32,8 +34,14 @@
 #include <type_traits>
 #include <system_error>
 
+#include <butl/process-details>
+
 using namespace std;
 
+#ifdef _WIN32
+using namespace butl::win32;
+#endif
+
 namespace butl
 {
   // throw_ios_failure
@@ -63,7 +71,7 @@ namespace butl
     throw ios_base::failure (m != nullptr ? m : e.message ().c_str ());
   }
 
-  inline void
+  static inline void
   throw_ios_failure (int ev, const char* m = nullptr)
   {
     error_code ec (ev, system_category ());
@@ -708,6 +716,10 @@ namespace butl
       pass_perm = false;
     }
 
+    // Make sure the file descriptor is not inheritable by default.
+    //
+    of |= _O_NOINHERIT;
+
     int fd (pass_perm
             ? _sopen (f, of, _SH_DENYNO, pf)
             : _sopen (f, of, _SH_DENYNO));
@@ -739,35 +751,48 @@ namespace butl
     return auto_fd (fd);
   }
 
+#ifndef _WIN32
+
   auto_fd
   fddup (int fd)
   {
-#ifndef _WIN32
-    int nfd (dup (fd));
+    // dup() doesn't copy FD_CLOEXEC flag, so we need to do it ourselves. Note
+    // that the new descriptor can leak into child processes before we copy the
+    // flag. To prevent this we will acquire the process_spawn_mutex (see
+    // process-details header) prior to duplicating the descriptor. Also note
+    // there is dup3() (available on Linux and FreeBSD but not on Max OS) that
+    // takes flags, but it's usage tends to be hairy (need to preopen a dummy
+    // file descriptor to pass as a second argument).
+    //
+    auto dup = [fd] () -> auto_fd
+    {
+      auto_fd nfd (::dup (fd));
+      if (nfd.get () == -1)
+        throw_ios_failure (errno);
+
+      return nfd;
+    };
 
     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);
-    }
+    // If the source descriptor has no FD_CLOEXEC flag set then no flag copy is
+    // required (as the duplicate will have no flag by default).
+    //
+    if ((f & FD_CLOEXEC) == 0)
+      return dup ();
 
-#else
-    int nfd (_dup (fd));
-#endif
+    slock l (process_spawn_mutex);
+    auto_fd nfd (dup ());
 
-    if (nfd == -1)
+    f = fcntl (nfd.get (), F_GETFD);
+    if (f == -1 || fcntl (nfd.get (), F_SETFD, f | FD_CLOEXEC) == -1)
       throw_ios_failure (errno);
 
-    return auto_fd (nfd);
+    return nfd;
   }
 
-#ifndef _WIN32
-
   bool
   fdclose (int fd) noexcept
   {
@@ -836,10 +861,20 @@ namespace butl
   {
     assert (m == fdopen_mode::none || m == fdopen_mode::binary);
 
+    // Note that the pipe file descriptors can leak into child processes before
+    // we set FD_CLOEXEC flag for them. To prevent this we will acquire the
+    // process_spawn_mutex (see process-details header) prior to creating the
+    // pipe. Also note there is pipe2() (available on Linux and FreeBSD but not
+    // on Max OS) that takes flags.
+    //
+    slock l (process_spawn_mutex);
+
     int pd[2];
     if (pipe (pd) == -1)
       throw_ios_failure (errno);
 
+    fdpipe r {auto_fd (pd[0]), auto_fd (pd[1])};
+
     for (size_t i (0); i < 2; ++i)
     {
       int f (fcntl (pd[i], F_GETFD));
@@ -847,11 +882,62 @@ namespace butl
         throw_ios_failure (errno);
     }
 
-    return {auto_fd (pd[0]), auto_fd (pd[1])};
+    return r;
   }
 
 #else
 
+  auto_fd
+  fddup (int fd)
+  {
+    // _dup() doesn't copy _O_NOINHERIT flag, so we need to do it ourselves.
+    // Note that the new descriptor can leak into child processes before we
+    // copy the flag. To prevent this we will acquire the process_spawn_mutex
+    // (see process-details header) prior to duplicating the descriptor.
+    //
+    // We can not ammend file descriptors directly (nor obtain the flag value),
+    // so need to resolve them to Windows HANDLE first. Such handles are closed
+    // either when CloseHandle() is called for them or when _close() is called
+    // for the associated file descriptors. Make sure that either the original
+    // file descriptor or the resulting HANDLE is closed but not both of them.
+    //
+    auto handle = [] (int fd) -> HANDLE
+    {
+      HANDLE h (reinterpret_cast<HANDLE> (_get_osfhandle (fd)));
+      if (h == INVALID_HANDLE_VALUE)
+        throw_ios_failure (EIO, "unable to obtain file handle");
+
+      return h;
+    };
+
+    auto dup = [fd] () -> auto_fd
+    {
+      auto_fd nfd (_dup (fd));
+      if (nfd.get () == -1)
+        throw_ios_failure (errno);
+
+      return nfd;
+    };
+
+    DWORD f;
+    if (!GetHandleInformation (handle (fd), &f))
+      throw_ios_failure (EIO, last_error_msg ().c_str ());
+
+    // If the source handle is inheritable then no flag copy is required (as
+    // the duplicate handle will be inheritable by default).
+    //
+    if (f & HANDLE_FLAG_INHERIT)
+      return dup ();
+
+    slock l (process_spawn_mutex);
+
+    auto_fd nfd (dup ());
+    if (!SetHandleInformation (handle (nfd.get ()), HANDLE_FLAG_INHERIT, 0))
+      throw_ios_failure (EIO, last_error_msg ().c_str ());
+
+    return nfd;
+  }
+
   bool
   fdclose (int fd) noexcept
   {
@@ -864,7 +950,7 @@ namespace butl
     // No need to translate \r\n before sending it to void.
     //
     if (!temp)
-      return _sopen ("nul", _O_RDWR | _O_BINARY, _SH_DENYNO);
+      return _sopen ("nul", _O_RDWR | _O_BINARY | _O_NOINHERIT, _SH_DENYNO);
 
     try
     {
@@ -873,11 +959,12 @@ namespace butl
       //
       path p (path::temp_path ("null")); // Can throw.
       return _sopen (p.string ().c_str (),
-                     (_O_CREAT      |
-                      _O_RDWR       |
-                      _O_BINARY     |  // Don't translate.
-                      _O_TEMPORARY  |  // Remove on close.
-                      _O_SHORT_LIVED), // Don't flush to disk.
+                     (_O_CREAT       |
+                      _O_RDWR        |
+                      _O_BINARY      | // Don't translate.
+                      _O_TEMPORARY   | // Remove on close.
+                      _O_SHORT_LIVED | // Don't flush to disk.
+                      _O_NOINHERIT),   // Don't inherit by child process.
                      _SH_DENYNO,
                      _S_IREAD | _S_IWRITE);
     }
@@ -908,6 +995,8 @@ namespace butl
     if (m != fdstream_mode::binary && m != fdstream_mode::text)
       throw invalid_argument ("invalid translation mode");
 
+    // Note that _setmode() preserves the _O_NOINHERIT flag value.
+    //
     int r (_setmode (fd, m == fdstream_mode::binary ? _O_BINARY : _O_TEXT));
     if (r == -1)
       throw_ios_failure (errno);
diff --git a/butl/process-details b/butl/process-details
new file mode 100644
index 0000000..b07ceff
--- /dev/null
+++ b/butl/process-details
@@ -0,0 +1,49 @@
+// file      : butl/process-details -*- C++ -*-
+// copyright : Copyright (c) 2014-2017 Code Synthesis Ltd
+// license   : MIT; see accompanying LICENSE file
+
+#ifndef BUTL_PROCESS_DETAILS
+#define BUTL_PROCESS_DETAILS
+
+#include <mutex>
+
+#include <butl/ft/shared_mutex>
+#if defined(__cpp_lib_shared_mutex) || defined(__cpp_lib_shared_timed_mutex)
+#  include <shared_mutex>
+#endif
+
+namespace butl
+{
+#if   defined(__cpp_lib_shared_mutex)
+  using shared_mutex = std::shared_mutex;
+  using ulock        = std::unique_lock<shared_mutex>;
+  using slock        = std::shared_lock<shared_mutex>;
+#elif defined(__cpp_lib_shared_timed_mutex)
+  using shared_mutex = std::shared_timed_mutex;
+  using ulock        = std::unique_lock<shared_mutex>;
+  using slock        = std::shared_lock<shared_mutex>;
+#else
+  // Because we have this fallback, we need to be careful not to create
+  // multiple shared locks in the same thread.
+  //
+  struct shared_mutex: std::mutex
+  {
+    using mutex::mutex;
+
+    void lock_shared     () { lock ();     }
+    void try_lock_shared () { try_lock (); }
+    void unlock_shared   () { unlock ();   }
+  };
+
+  using ulock        = std::unique_lock<shared_mutex>;
+  using slock        = ulock;
+#endif
+
+  // Mutex that is acquired to make a sequence of operations atomic in regards
+  // to child process spawning. Must be aquired for exclusive access for child
+  // process startup, and for shared access otherwise. Defined in process.cxx.
+  //
+  extern shared_mutex process_spawn_mutex;
+}
+
+#endif // BUTL_PROCESS_DETAILS
diff --git a/butl/process.cxx b/butl/process.cxx
index 892e930..189d1ee 100644
--- a/butl/process.cxx
+++ b/butl/process.cxx
@@ -27,7 +27,7 @@
 
 #  include <cstdlib> // __argv[]
 
-#  include <butl/win32-utility>
+#  include <butl/small-vector>
 #endif
 
 #include <errno.h>
@@ -39,8 +39,9 @@
 #include <utility>  // move()
 #include <ostream>
 
-#include <butl/utility>  // casecmp()
-#include <butl/fdstream> // fdnull()
+#include <butl/utility>         // casecmp()
+#include <butl/fdstream>        // fdnull()
+#include <butl/process-details>
 
 using namespace std;
 
@@ -50,6 +51,8 @@ using namespace butl::win32;
 
 namespace butl
 {
+  shared_mutex process_spawn_mutex;
+
   // process
   //
   static process_path
@@ -256,7 +259,7 @@ namespace butl
     fdpipe in_ofd;
     fdpipe in_efd;
 
-    auto fail = [](bool child) {throw process_error (errno, child);};
+    auto fail = [] (bool child) {throw process_error (errno, child);};
 
     auto open_pipe = [] () -> fdpipe
     {
@@ -303,65 +306,68 @@ namespace butl
     else if (err == -2)
       in_efd.out = open_null ();
 
-    handle = fork ();
+    {
+      ulock l (process_spawn_mutex); // Will not be released in child.
+      handle = fork ();
 
-    if (handle == -1)
-      fail (false);
+      if (handle == -1)
+        fail (false);
 
-    if (handle == 0)
-    {
-      // Child.
-      //
-      // Duplicate the user-supplied (fd > -1) or the created pipe descriptor
-      // to the standard stream descriptor (read end for STDIN_FILENO, write
-      // end otherwise). Close the the pipe afterwards.
-      //
-      auto duplicate = [&fail](int sd, int fd, fdpipe& pd)
+      if (handle == 0)
       {
-        if (fd == -1 || fd == -2)
-          fd = (sd == STDIN_FILENO ? pd.in : pd.out).get ();
+        // Child.
+        //
+        // Duplicate the user-supplied (fd > -1) or the created pipe descriptor
+        // to the standard stream descriptor (read end for STDIN_FILENO, write
+        // end otherwise). Close the the pipe afterwards.
+        //
+        auto duplicate = [&fail] (int sd, int fd, fdpipe& pd)
+        {
+          if (fd == -1 || fd == -2)
+            fd = (sd == STDIN_FILENO ? pd.in : pd.out).get ();
 
-        assert (fd > -1);
-        if (dup2 (fd, sd) == -1)
-          fail (true);
+          assert (fd > -1);
+          if (dup2 (fd, sd) == -1)
+            fail (true);
 
-        pd.in.reset ();  // Silently close.
-        pd.out.reset (); // Silently close.
-      };
+          pd.in.reset ();  // Silently close.
+          pd.out.reset (); // Silently close.
+        };
 
-      if (in != STDIN_FILENO)
-        duplicate (STDIN_FILENO, in, out_fd);
+        if (in != STDIN_FILENO)
+          duplicate (STDIN_FILENO, in, out_fd);
 
-      // If stdout is redirected to stderr (out == 2) we need to duplicate it
-      // after duplicating stderr to pickup the proper fd. Otherwise keep the
-      // "natual" order of duplicate() calls, so if stderr is redirected to
-      // stdout it picks up the proper fd as well.
-      //
-      if (out == STDERR_FILENO)
-      {
-        if (err != STDERR_FILENO)
-          duplicate (STDERR_FILENO, err, in_efd);
+        // If stdout is redirected to stderr (out == 2) we need to duplicate it
+        // after duplicating stderr to pickup the proper fd. Otherwise keep the
+        // "natual" order of duplicate() calls, so if stderr is redirected to
+        // stdout it picks up the proper fd as well.
+        //
+        if (out == STDERR_FILENO)
+        {
+          if (err != STDERR_FILENO)
+            duplicate (STDERR_FILENO, err, in_efd);
 
-        if (out != STDOUT_FILENO)
-          duplicate (STDOUT_FILENO, out, in_ofd);
-      }
-      else
-      {
-        if (out != STDOUT_FILENO)
-          duplicate (STDOUT_FILENO, out, in_ofd);
+          if (out != STDOUT_FILENO)
+            duplicate (STDOUT_FILENO, out, in_ofd);
+        }
+        else
+        {
+          if (out != STDOUT_FILENO)
+            duplicate (STDOUT_FILENO, out, in_ofd);
 
-        if (err != STDERR_FILENO)
-          duplicate (STDERR_FILENO, err, in_efd);
-      }
+          if (err != STDERR_FILENO)
+            duplicate (STDERR_FILENO, err, in_efd);
+        }
 
-      // Change current working directory if requested.
-      //
-      if (cwd != nullptr && *cwd != '\0' && chdir (cwd) != 0)
-        fail (true);
+        // Change current working directory if requested.
+        //
+        if (cwd != nullptr && *cwd != '\0' && chdir (cwd) != 0)
+          fail (true);
 
-      if (execv (pp.effect_string (), const_cast<char**> (&args[0])) == -1)
-        fail (true);
-    }
+        if (execv (pp.effect_string (), const_cast<char**> (&args[0])) == -1)
+          fail (true);
+      }
+    } // Release the lock in parent.
 
     assert (handle != 0); // Shouldn't get here unless in the parent process.
 
@@ -798,6 +804,61 @@ namespace butl
     HANDLE handle_;
   };
 
+  // Make handles inheritable. The process_spawn_mutex must be pre-acquired for
+  // exclusive access. Revert handles inheritability state in destructor.
+  //
+  // There is a period of time when the process ctor makes file handles it
+  // passes to the child to be inheritable, that otherwise are not inheritable
+  // by default. During this time these handles can also be inherited by other
+  // (irrelevant) child processed spawned from other threads. That can lead to
+  // some unwanted consequences, such as inability to delete a file
+  // corresponding to such a handle until all childs, that the handle leaked
+  // into, terminate. To prevent this behavior the specific sequence of steps
+  // (that involves making handles inheritable, spawning process and reverting
+  // handles to non-inheritable state back) will be performed after aquiring
+  // the process_spawn_mutex (that is released afterwards).
+  //
+  class inheritability_guard
+  {
+  public:
+    // Require the proof that the mutex is pre-acquired for exclusive access.
+    //
+    inheritability_guard (const ulock&) {}
+
+    ~inheritability_guard ()
+    {
+      for (auto h: handles_)
+        inheritable (h, false); // Can't throw.
+    }
+
+    void
+    inheritable (HANDLE h)
+    {
+      inheritable (h, true);  // Can throw.
+      handles_.push_back (h);
+    }
+
+  private:
+    void
+    inheritable (HANDLE h, bool state)
+    {
+      if (!SetHandleInformation (
+            h, HANDLE_FLAG_INHERIT, state ? HANDLE_FLAG_INHERIT : 0))
+      {
+        if (state)
+          throw process_error (last_error_msg ());
+
+        // We should be able to successfully reset the HANDLE_FLAG_INHERIT flag
+        // that we successfully set, unless something is severely damaged.
+        //
+        assert (false);
+      }
+    }
+
+  private:
+    small_vector<HANDLE, 3> handles_;
+  };
+
   process::
   process (const char* cwd,
            const process_path& pp, const char* args[],
@@ -862,35 +923,6 @@ namespace butl
     else if (err == -2)
       in_efd.out = open_null ();
 
-    // 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 resulting HANDLE is
-    // closed but not both of them.
-    //
-    auto get_osfhandle = [&fail](int fd) -> HANDLE
-    {
-      HANDLE h (reinterpret_cast<HANDLE> (_get_osfhandle (fd)));
-      if (h == INVALID_HANDLE_VALUE)
-        fail ("unable to obtain file handle");
-
-      // Make the handle inheritable by the child unless it is already
-      // inheritable.
-      //
-      DWORD f;
-      if (!GetHandleInformation (h, &f))
-	fail ();
-
-      // Note that the flag check is essential as SetHandleInformation() fails
-      // for standard handles and their duplicates.
-      //
-      if ((f & HANDLE_FLAG_INHERIT) == 0 &&
-          !SetHandleInformation (h, HANDLE_FLAG_INHERIT, HANDLE_FLAG_INHERIT))
-        fail ();
-
-      return h;
-    };
-
     // Create the process.
     //
 
@@ -934,58 +966,90 @@ namespace butl
     si.cb = sizeof (STARTUPINFO);
     si.dwFlags |= STARTF_USESTDHANDLES;
 
-    si.hStdInput = in == -1 || in == -2
-      ? get_osfhandle (out_fd.in.get ())
-      : in == STDIN_FILENO
-        ? GetStdHandle (STD_INPUT_HANDLE)
-        : get_osfhandle (in);
-
-    si.hStdOutput = out == -1 || out == -2
-      ? get_osfhandle (in_ofd.out.get ())
-      : out == STDOUT_FILENO
-        ? GetStdHandle (STD_OUTPUT_HANDLE)
-        : get_osfhandle (out);
-
-    si.hStdError = err == -1 || err == -2
-      ? get_osfhandle (in_efd.out.get ())
-      : err == STDERR_FILENO
-        ? GetStdHandle (STD_ERROR_HANDLE)
-        : get_osfhandle (err);
-
-    // Perform standard stream redirection if requested.
-    //
-    if (err == STDOUT_FILENO)
-      si.hStdError = si.hStdOutput;
-    else if (out == STDERR_FILENO)
-      si.hStdOutput = si.hStdError;
-
-    if (err == STDIN_FILENO ||
-        out == STDIN_FILENO ||
-        in == STDOUT_FILENO ||
-        in == STDERR_FILENO)
-      fail ("invalid file descriptor");
-
-    if (!CreateProcess (
-          pp.effect_string (),
-          const_cast<char*> (cmd_line.c_str ()),
-          0,    // Process security attributes.
-          0,    // Primary thread security attributes.
-          true, // Inherit handles.
-          0,    // Creation flags.
-          0,    // Use our environment.
-          cwd != nullptr && *cwd != '\0' ? cwd : nullptr,
-          &si,
-          &pi))
-      fail ();
+    {
+      ulock l (process_spawn_mutex);
+      inheritability_guard ig (l);
+
+      // 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 resulting HANDLE
+      // is closed but not both of them.
+      //
+      auto get_osfhandle = [&fail, &ig] (int fd) -> HANDLE
+      {
+        HANDLE h (reinterpret_cast<HANDLE> (_get_osfhandle (fd)));
+        if (h == INVALID_HANDLE_VALUE)
+          fail ("unable to obtain file handle");
+
+        // Make the handle inheritable by the child unless it is already
+        // inheritable.
+        //
+        DWORD f;
+        if (!GetHandleInformation (h, &f))
+          fail ();
+
+        // Note that the flag check is essential as SetHandleInformation()
+        // fails for standard handles and their duplicates.
+        //
+        if ((f & HANDLE_FLAG_INHERIT) == 0)
+          ig.inheritable (h);
+
+        return h;
+      };
+
+      si.hStdInput = in == -1 || in == -2
+        ? get_osfhandle (out_fd.in.get ())
+        : (in == STDIN_FILENO
+           ? GetStdHandle (STD_INPUT_HANDLE)
+           : get_osfhandle (in));
+
+      si.hStdOutput = out == -1 || out == -2
+        ? get_osfhandle (in_ofd.out.get ())
+        : (out == STDOUT_FILENO
+           ? GetStdHandle (STD_OUTPUT_HANDLE)
+           : get_osfhandle (out));
+
+      si.hStdError = err == -1 || err == -2
+        ? get_osfhandle (in_efd.out.get ())
+        : (err == STDERR_FILENO
+           ? GetStdHandle (STD_ERROR_HANDLE)
+           : get_osfhandle (err));
+
+      // Perform standard stream redirection if requested.
+      //
+      if (err == STDOUT_FILENO)
+        si.hStdError = si.hStdOutput;
+      else if (out == STDERR_FILENO)
+        si.hStdOutput = si.hStdError;
+
+      if (err == STDIN_FILENO ||
+          out == STDIN_FILENO ||
+          in == STDOUT_FILENO ||
+          in == STDERR_FILENO)
+        fail ("invalid file descriptor");
+
+      if (!CreateProcess (
+            pp.effect_string (),
+            const_cast<char*> (cmd_line.c_str ()),
+            0,    // Process security attributes.
+            0,    // Primary thread security attributes.
+            true, // Inherit handles.
+            0,    // Creation flags.
+            0,    // Use our environment.
+            cwd != nullptr && *cwd != '\0' ? cwd : nullptr,
+            &si,
+            &pi))
+        fail ();
+    } // Revert handled back to non-inheritable and release the lock.
 
     auto_handle (pi.hThread).reset (); // Close.
-    auto_handle process (pi.hProcess);
 
     this->out_fd = move (out_fd.out);
     this->in_ofd = move (in_ofd.in);
     this->in_efd = move (in_efd.in);
 
-    this->handle = process.release ();
+    this->handle = pi.hProcess;
 
     // 0 has a special meaning denoting a terminated process handle.
     //