From aa5ce03b40003ee8f7cfff4d2f1285b405f5906a Mon Sep 17 00:00:00 2001 From: Karen Arutyunov Date: Sat, 18 Mar 2017 00:55:59 +0300 Subject: Fix file descriptors leakage to child process on Windows --- butl/process.cxx | 316 +++++++++++++++++++++++++++++++++---------------------- 1 file changed, 190 insertions(+), 126 deletions(-) (limited to 'butl/process.cxx') 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 // __argv[] -# include +# include #endif #include @@ -39,8 +39,9 @@ #include // move() #include -#include // casecmp() -#include // fdnull() +#include // casecmp() +#include // fdnull() +#include 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 (&args[0])) == -1) - fail (true); - } + if (execv (pp.effect_string (), const_cast (&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 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 (_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 (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 (_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 (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. // -- cgit v1.1