From 2fd3f2fec67a3bb62378c286fbf9667e6fb3cc3b Mon Sep 17 00:00:00 2001 From: Haoran Peng Date: Thu, 1 May 2025 22:04:59 +0100 Subject: [PATCH 1/9] subprocess: Fix memory leaks I encountered this issue while running my code with Valgrind today. Below is part of the Valgrind error message: ``` ==1578139== 472 bytes in 1 blocks are still reachable in loss record 1 of 1 ==1578139== at 0x4848899: malloc (...) ==1578139== by 0x4B3AF62: fdopen@@GLIBC_2.2.5 (...) ==1578139== by 0x118B09: subprocess::Popen::execute_process() (...) ``` I noticed that a similar fix had been proposed by another contributor previously. I did not mean to scoop their work, but merely hoping to fix it sooner so other people don't get confused by it just as I did today. Github-Pull: arun11299/cpp-subprocess#106 Rebased-From: 3afe581c1f22f106d59cf54b9b65251e6c554671 --- src/util/subprocess.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 3449fa3b1b..26651d3ae5 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -1181,11 +1181,13 @@ inline void Popen::execute_process() noexcept(false) try { char err_buf[SP_MAX_ERR_BUF_SIZ] = {0,}; - int read_bytes = util::read_atmost_n( - fdopen(err_rd_pipe, "r"), - err_buf, - SP_MAX_ERR_BUF_SIZ); - close(err_rd_pipe); + FILE* err_fp = fdopen(err_rd_pipe, "r"); + if (!err_fp) { + close(err_rd_pipe); + throw OSError("fdopen failed", errno); + } + int read_bytes = util::read_atmost_n(err_fp, err_buf, SP_MAX_ERR_BUF_SIZ); + fclose(err_fp); if (read_bytes || strlen(err_buf)) { // Call waitpid to reap the child process From d3f511b4583b3771bf941ebc7884477430115039 Mon Sep 17 00:00:00 2001 From: Haowen Liu <35328328+lunacd@users.noreply.github.com> Date: Thu, 1 May 2025 22:06:28 +0100 Subject: [PATCH 2/9] 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 26651d3ae5..086a455361 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 647630462f10cacc7a75da7c82ca7c6d33bbde4b Mon Sep 17 00:00:00 2001 From: Haowen Liu <35328328+lunacd@users.noreply.github.com> Date: Thu, 1 May 2025 22:07:21 +0100 Subject: [PATCH 3/9] 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 086a455361..ba274dfb47 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 7997b7656f99c5415cfa02cb1206226d0a82efd6 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 1 May 2025 22:07:39 +0100 Subject: [PATCH 4/9] 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 ba274dfb47..d013d47432 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 174bd43f2e46a0ccc6f5ad486bb587c72c1241c3 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 1 May 2025 22:10:35 +0100 Subject: [PATCH 5/9] 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 d013d47432..6771632840 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(); @@ -1188,7 +1200,7 @@ inline void Popen::execute_process() noexcept(false) FILE* err_fp = fdopen(err_rd_pipe, "r"); if (!err_fp) { - close(err_rd_pipe); + subprocess_close(err_rd_pipe); throw OSError("fdopen failed", errno); } int read_bytes = util::read_atmost_n(err_fp, err_buf, SP_MAX_ERR_BUF_SIZ); @@ -1278,13 +1290,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()); @@ -1311,15 +1323,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 bb9ffea53fb021580f069c431aee02f547039831 Mon Sep 17 00:00:00 2001 From: Shunsuke Shimizu Date: Thu, 1 May 2025 22:14:29 +0100 Subject: [PATCH 6/9] subprocess: Explicitly define move constructor of Streams class This suppresses the following warning caused by clang-20. ``` error: definition of implicit copy constructor for 'Streams' is deprecated because it has a user-declared copy assignment operator [-Werror,-Wdeprecated-copy] ``` Copy constructor or move constructor is called when std::vector re-allocates memory. In this case, move constructor should be called, because copying Streams instances breaks file-descriptor management. Communication class is modified as well, since it's instance is a member of Streams class. Github-Pull: arun11299/cpp-subprocess#107 Rebased-From: 38d98d9d20be50c7187b98ac9bc9a6e66920f6ef --- src/util/subprocess.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/subprocess.h b/src/util/subprocess.h index 6771632840..91f84a290b 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -770,7 +770,10 @@ class Communication public: Communication(Streams* stream): stream_(stream) {} - void operator=(const Communication&) = delete; + Communication(const Communication&) = delete; + Communication& operator=(const Communication&) = delete; + Communication(Communication&&) = default; + Communication& operator=(Communication&&) = default; public: int send(const char* msg, size_t length); int send(const std::vector& msg); @@ -807,7 +810,10 @@ class Streams { public: Streams():comm_(this) {} - void operator=(const Streams&) = delete; + Streams(const Streams&) = delete; + Streams& operator=(const Streams&) = delete; + Streams(Streams&&) = default; + Streams& operator=(Streams&&) = default; public: void setup_comm_channels(); From 7423214d8deebbbcdab66090e185129decfaa799 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Thu, 1 May 2025 22:15:19 +0100 Subject: [PATCH 7/9] 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 91f84a290b..6fa84af2af 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 b7288decdf534391b3f917cfb11ec62580407c3f Mon Sep 17 00:00:00 2001 From: Haowen Liu <35328328+lunacd@users.noreply.github.com> Date: Thu, 1 May 2025 22:15:57 +0100 Subject: [PATCH 8/9] 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 6fa84af2af..e8b7989ce8 100644 --- a/src/util/subprocess.h +++ b/src/util/subprocess.h @@ -1061,11 +1061,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 cd95c9d6a7ec08cca0f9c98328c759be586720f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Andr=C3=B3il?= Date: Thu, 1 May 2025 22:16:11 +0100 Subject: [PATCH 9/9] 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 e8b7989ce8..be4b3e5e39 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); + } }