From 9888d0f511568aaf66fbb64a39dd99933b243cf2 Mon Sep 17 00:00:00 2001 From: Haowen Liu <35328328+lunacd@users.noreply.github.com> Date: Sun, 27 Apr 2025 16:43:42 +0100 Subject: [PATCH 1/7] subprocess: Fix string_arg when used with rref When passing in a rvalue reference, compiler considers it ambiguous between std::string and std::string&&. Making one of them take a lvalue reference makes compilers correctly pick the right one depending on whether the passed in value binds to a rvalue or lvalue reference. Github-Pull: arun11299/cpp-subprocess#110 Rebased-From: 2d8a8eebb03e509840e2c3c755d1abf32d930f33 --- src/util/subprocess.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 3449fa3b1ba..e579b2774eb 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -527,7 +527,7 @@ struct string_arg { string_arg(const char* arg): arg_value(arg) {} string_arg(std::string&& arg): arg_value(std::move(arg)) {} - string_arg(std::string arg): arg_value(std::move(arg)) {} + string_arg(const std::string& arg): arg_value(arg) {} std::string arg_value; }; From 9e8992bf8b7c4541fd8a50587aed6fced5eb8d42 Mon Sep 17 00:00:00 2001 From: Haowen Liu <35328328+lunacd@users.noreply.github.com> Date: Sun, 27 Apr 2025 16:47:04 +0100 Subject: [PATCH 2/7] subprocess: Get Windows return code in wait() Currently, wait() returns 0 on windows regardless of the actual return code of processes. Github-Pull: arun11299/cpp-subprocess#109 Rebased-From: 04b015a8e52ead4d8bb5f0eb486419c77e418a17 --- src/util/subprocess.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index e579b2774eb..81c798835b3 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -1043,7 +1043,12 @@ inline int Popen::wait() noexcept(false) #ifdef __USING_WINDOWS__ int ret = WaitForSingleObject(process_handle_, INFINITE); - return 0; + DWORD dretcode_; + + if (FALSE == GetExitCodeProcess(process_handle_, &dretcode_)) + throw OSError("Failed during call to GetExitCodeProcess", 0); + + return (int)dretcode_; #else int ret, status; std::tie(ret, status) = util::wait_for_child_exit(child_pid_); From 7ecb4e430a37989fbfadbf9e0cbd08851309a4e6 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 27 Apr 2025 16:50:15 +0100 Subject: [PATCH 3/7] subprocess: Fix cross-compiling with mingw toolchain Github-Pull: arun11299/cpp-subprocess#99 Rebased-From: 34033d03deacfdba892a708b7d8092b4d9e5e889 --- src/util/subprocess.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 81c798835b3..22f8b52903a 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -65,7 +65,7 @@ Documentation for C++ subprocessing library. extern "C" { #ifdef __USING_WINDOWS__ - #include + #include #include #include From df7a52241f7dd7e995e5971abf67c293fc667144 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 27 Apr 2025 17:23:20 +0100 Subject: [PATCH 4/7] subprocess: Avoid leaking POSIX name aliases beyond `subprocess.h` The commit a32c0f3df4b6bcd1d2e93f19e8f380bb890cd507 introduced code to silence MSVC's "warning C4996: The POSIX name for this item is deprecated." However, it exhibits several issues: 1. The aliases may leak into code outside the `subprocess.hpp` header. 2. They are unnecessarily applied when using the MinGW-w64 toolchain. 3. The fix is incomplete: downstream projects still see C4996 warnings. 4. The implementation lacks documentation. This change addresses all of the above shortcomings. Github-Pull: arun11299/cpp-subprocess#112 Rebased-From: 778543b2f2ca7f5d1c4f0241b635bbb265d750dd Co-authored-by: Luke Dashjr --- src/util/subprocess.h | 78 +++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 33 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 22f8b52903a..4de9851bab3 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -68,10 +68,6 @@ extern "C" { #include #include #include - - #define close _close - #define open _open - #define fileno _fileno #else #include #include @@ -81,6 +77,22 @@ extern "C" { #include } +// The Microsoft C++ compiler issues deprecation warnings +// for the standard POSIX function names. +// Its preferred implementations have a leading underscore. +// See: https://learn.microsoft.com/en-us/cpp/c-runtime-library/compatibility. +#if (defined _MSC_VER) + #define subprocess_close _close + #define subprocess_fileno _fileno + #define subprocess_open _open + #define subprocess_write _write +#else + #define subprocess_close close + #define subprocess_fileno fileno + #define subprocess_open open + #define subprocess_write write +#endif + /*! * Getting started with reading this source code. * The source is mainly divided into four parts: @@ -264,7 +276,7 @@ namespace util FILE *fp = _fdopen(os_fhandle, mode); if (fp == 0) { - _close(os_fhandle); + subprocess_close(os_fhandle); throw OSError("_fdopen", 0); } @@ -383,7 +395,7 @@ namespace util { size_t nwritten = 0; while (nwritten < length) { - int written = write(fd, buf + nwritten, length - nwritten); + int written = subprocess_write(fd, buf + nwritten, length - nwritten); if (written == -1) return -1; nwritten += written; } @@ -411,7 +423,7 @@ namespace util #ifdef __USING_WINDOWS__ return (int)fread(buf, 1, read_upto, fp); #else - int fd = fileno(fp); + int fd = subprocess_fileno(fp); int rbytes = 0; int eintr_cnter = 0; @@ -573,10 +585,10 @@ struct input explicit input(int fd): rd_ch_(fd) {} // FILE pointer. - explicit input (FILE* fp):input(fileno(fp)) { assert(fp); } + explicit input (FILE* fp):input(subprocess_fileno(fp)) { assert(fp); } explicit input(const char* filename) { - int fd = open(filename, O_RDONLY); + int fd = subprocess_open(filename, O_RDONLY); if (fd == -1) throw OSError("File not found: ", errno); rd_ch_ = fd; } @@ -606,10 +618,10 @@ struct output { explicit output(int fd): wr_ch_(fd) {} - explicit output (FILE* fp):output(fileno(fp)) { assert(fp); } + explicit output (FILE* fp):output(subprocess_fileno(fp)) { assert(fp); } explicit output(const char* filename) { - int fd = open(filename, O_APPEND | O_CREAT | O_RDWR, 0640); + int fd = subprocess_open(filename, O_APPEND | O_CREAT | O_RDWR, 0640); if (fd == -1) throw OSError("File not found: ", errno); wr_ch_ = fd; } @@ -637,10 +649,10 @@ struct error { explicit error(int fd): wr_ch_(fd) {} - explicit error(FILE* fp):error(fileno(fp)) { assert(fp); } + explicit error(FILE* fp):error(subprocess_fileno(fp)) { assert(fp); } explicit error(const char* filename) { - int fd = open(filename, O_APPEND | O_CREAT | O_RDWR, 0640); + int fd = subprocess_open(filename, O_APPEND | O_CREAT | O_RDWR, 0640); if (fd == -1) throw OSError("File not found: ", errno); wr_ch_ = fd; } @@ -803,28 +815,28 @@ public: void cleanup_fds() { if (write_to_child_ != -1 && read_from_parent_ != -1) { - close(write_to_child_); + subprocess_close(write_to_child_); } if (write_to_parent_ != -1 && read_from_child_ != -1) { - close(read_from_child_); + subprocess_close(read_from_child_); } if (err_write_ != -1 && err_read_ != -1) { - close(err_read_); + subprocess_close(err_read_); } } void close_parent_fds() { - if (write_to_child_ != -1) close(write_to_child_); - if (read_from_child_ != -1) close(read_from_child_); - if (err_read_ != -1) close(err_read_); + if (write_to_child_ != -1) subprocess_close(write_to_child_); + if (read_from_child_ != -1) subprocess_close(read_from_child_); + if (err_read_ != -1) subprocess_close(err_read_); } void close_child_fds() { - if (write_to_parent_ != -1) close(write_to_parent_); - if (read_from_parent_ != -1) close(read_from_parent_); - if (err_write_ != -1) close(err_write_); + if (write_to_parent_ != -1) subprocess_close(write_to_parent_); + if (read_from_parent_ != -1) subprocess_close(read_from_parent_); + if (err_write_ != -1) subprocess_close(err_write_); } FILE* input() { return input_.get(); } @@ -1161,8 +1173,8 @@ inline void Popen::execute_process() noexcept(false) child_pid_ = fork(); if (child_pid_ < 0) { - close(err_rd_pipe); - close(err_wr_pipe); + subprocess_close(err_rd_pipe); + subprocess_close(err_wr_pipe); throw OSError("fork failed", errno); } @@ -1172,14 +1184,14 @@ inline void Popen::execute_process() noexcept(false) stream_.close_parent_fds(); //Close the read end of the error pipe - close(err_rd_pipe); + subprocess_close(err_rd_pipe); detail::Child chld(this, err_wr_pipe); chld.execute_child(); } else { - close (err_wr_pipe);// close child side of pipe, else get stuck in read below + subprocess_close(err_wr_pipe);// close child side of pipe, else get stuck in read below stream_.close_child_fds(); @@ -1190,7 +1202,7 @@ inline void Popen::execute_process() noexcept(false) fdopen(err_rd_pipe, "r"), err_buf, SP_MAX_ERR_BUF_SIZ); - close(err_rd_pipe); + subprocess_close(err_rd_pipe); if (read_bytes || strlen(err_buf)) { // Call waitpid to reap the child process @@ -1276,13 +1288,13 @@ namespace detail { // Close the duped descriptors if (stream.read_from_parent_ != -1 && stream.read_from_parent_ > 2) - close(stream.read_from_parent_); + subprocess_close(stream.read_from_parent_); if (stream.write_to_parent_ != -1 && stream.write_to_parent_ > 2) - close(stream.write_to_parent_); + subprocess_close(stream.write_to_parent_); if (stream.err_write_ != -1 && stream.err_write_ > 2) - close(stream.err_write_); + subprocess_close(stream.err_write_); // Replace the current image with the executable sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data()); @@ -1309,15 +1321,15 @@ namespace detail { #ifdef __USING_WINDOWS__ util::configure_pipe(&this->g_hChildStd_IN_Rd, &this->g_hChildStd_IN_Wr, &this->g_hChildStd_IN_Wr); this->input(util::file_from_handle(this->g_hChildStd_IN_Wr, "w")); - this->write_to_child_ = _fileno(this->input()); + this->write_to_child_ = subprocess_fileno(this->input()); util::configure_pipe(&this->g_hChildStd_OUT_Rd, &this->g_hChildStd_OUT_Wr, &this->g_hChildStd_OUT_Rd); this->output(util::file_from_handle(this->g_hChildStd_OUT_Rd, "r")); - this->read_from_child_ = _fileno(this->output()); + this->read_from_child_ = subprocess_fileno(this->output()); util::configure_pipe(&this->g_hChildStd_ERR_Rd, &this->g_hChildStd_ERR_Wr, &this->g_hChildStd_ERR_Rd); this->error(util::file_from_handle(this->g_hChildStd_ERR_Rd, "r")); - this->err_read_ = _fileno(this->error()); + this->err_read_ = subprocess_fileno(this->error()); #else if (write_to_child_ != -1) input(fdopen(write_to_child_, "wb")); From bf8230d52520c2b0cf843d28d06565d2b7893c57 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Sun, 27 Apr 2025 17:23:27 +0100 Subject: [PATCH 5/7] subprocess: Do not escape double quotes for command line arguments on Windows * refactor: Guard `util::quote_argument()` with `#ifdef __USING_WINDOWS__` The `util::quote_argument()` function is specific to Windows and is used in code already guarded by `#ifdef __USING_WINDOWS__`. * Do not escape double quotes for command line arguments on Windows This change fixes the handling of double quotes and aligns the behavior with Python's `Popen` class. For example: ``` >py -3 >>> import subprocess >>> p = subprocess.Popen("cmd.exe /c dir \"C:\\Program Files\"", stdout=subprocess.PIPE, text=True) >>> print(f"Captured stdout:\n{stdout}") ``` Currently, the same command line processed by the `quote_argument()` function looks like `cmd.exe /c dir "\"C:\Program" "Files\""`, which is broken. With this change, it looks correct: `cmd.exe /c dir "C:\Program Files"`. Github-Pull: arun11299/cpp-subprocess#113 Rebased-From: ed313971c04ac10dc006104aba07d016ffc6542a --- src/util/subprocess.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 4de9851bab3..67d0a59ed10 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -171,6 +171,7 @@ public: //-------------------------------------------------------------------- namespace util { +#ifdef __USING_WINDOWS__ inline void quote_argument(const std::wstring &argument, std::wstring &command_line, bool force) { @@ -181,7 +182,7 @@ namespace util // if (force == false && argument.empty() == false && - argument.find_first_of(L" \t\n\v\"") == argument.npos) { + argument.find_first_of(L" \t\n\v") == argument.npos) { command_line.append(argument); } else { @@ -231,7 +232,6 @@ namespace util } } -#ifdef __USING_WINDOWS__ inline std::string get_last_error(DWORD errorMessageID) { if (errorMessageID == 0) From e25ec7300c18d6e4f05987614e5cfd8dbd93a300 Mon Sep 17 00:00:00 2001 From: Haowen Liu <35328328+lunacd@users.noreply.github.com> Date: Sun, 27 Apr 2025 17:23:38 +0100 Subject: [PATCH 6/7] subprocess: Proper implementation of wait() on Windows This commit makes sure: 1. WaitForSingleObject returns with expected code before proceeding. 2. Process handle is properly closed. Github-Pull: arun11299/cpp-subprocess#116 Rebased-From: 625a8775791e62736f20f3fa3e6cc4f1b24aa89a --- src/util/subprocess.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 67d0a59ed10..29f6e8ffa7e 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -1055,11 +1055,18 @@ inline int Popen::wait() noexcept(false) #ifdef __USING_WINDOWS__ int ret = WaitForSingleObject(process_handle_, INFINITE); + // WaitForSingleObject with INFINITE should only return when process has signaled + if (ret != WAIT_OBJECT_0) { + throw OSError("Unexpected return code from WaitForSingleObject", 0); + } + DWORD dretcode_; if (FALSE == GetExitCodeProcess(process_handle_, &dretcode_)) throw OSError("Failed during call to GetExitCodeProcess", 0); + CloseHandle(process_handle_); + return (int)dretcode_; #else int ret, status; From 7e80030a952a06101d5755032ebb1ff15823815e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Andr=C3=B3il?= Date: Mon, 28 Apr 2025 17:14:19 +0100 Subject: [PATCH 7/7] subprocess: check and handle fcntl(F_GETFD) failure Add missing error check for fcntl(fd, F_GETFD, 0) in set_clo_on_exec. Raise OSError on failure to align with existing FD_SETFD behavior. This improves robustness in subprocess setup and error visibility. Github-Pull: arun11299/cpp-subprocess#117 Rebased-From: 9974ff69cdd5fc1a2722cb63f006df9308628b35 --- src/util/subprocess.h | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 29f6e8ffa7e..244a99957c7 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -346,10 +346,14 @@ namespace util void set_clo_on_exec(int fd, bool set = true) { int flags = fcntl(fd, F_GETFD, 0); + if (flags == -1) { + throw OSError("fcntl F_GETFD failed", errno); + } if (set) flags |= FD_CLOEXEC; else flags &= ~FD_CLOEXEC; - //TODO: should check for errors - fcntl(fd, F_SETFD, flags); + if (fcntl(fd, F_SETFD, flags) == -1) { + throw OSError("fcntl F_SETFD failed", errno); + } }