diff options
| -rw-r--r-- | butl/buildfile | 6 | ||||
| -rw-r--r-- | butl/fdstream | 21 | ||||
| -rw-r--r-- | butl/fdstream.cxx | 137 | ||||
| -rw-r--r-- | butl/process-details | 49 | ||||
| -rw-r--r-- | butl/process.cxx | 316 |
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. // |