Merge bitcoin#32358: subprocess: Backport upstream changes

cd95c9d subprocess: check and handle fcntl(F_GETFD) failure (Tomás Andróil)
b7288de subprocess: Proper implementation of wait() on Windows (Haowen Liu)
7423214 subprocess: Do not escape double quotes for command line arguments on Windows (Hennadii Stepanov)
bb9ffea subprocess: Explicitly define move constructor of Streams class (Shunsuke Shimizu)
174bd43 subprocess: Avoid leaking POSIX name aliases beyond `subprocess.h` (Hennadii Stepanov)
7997b76 subprocess: Fix cross-compiling with mingw toolchain (Hennadii Stepanov)
6476304 subprocess: Get Windows return code in wait() (Haowen Liu)
d3f511b subprocess: Fix string_arg when used with rref (Haowen Liu)
2fd3f2f subprocess: Fix memory leaks (Haoran Peng)

Pull request description:

  Most of these changes were developed during work on bitcoin#29868 and bitcoin#32342 and have since been upstreamed.

  As they are now merged, this PR backports them to our `src/util/subprocess.h` header.

  Required for bitcoin#29868.

  A list of the backported PRs:
   - arun11299/cpp-subprocess#106
  - arun11299/cpp-subprocess#110
  - arun11299/cpp-subprocess#109
  - arun11299/cpp-subprocess#99
  - arun11299/cpp-subprocess#112
  - arun11299/cpp-subprocess#107
  - arun11299/cpp-subprocess#113
  - arun11299/cpp-subprocess#116
  - arun11299/cpp-subprocess#117

  The following PRs were skipped for backporting:
  - arun11299/cpp-subprocess#108 because we are not planning to support this feature.
  - arun11299/cpp-subprocess#101 because that change has been already landed in bitcoin#29849.

ACKs for top commit:
  theStack:
    Light ACK cd95c9d
  laanwj:
    Code review re-ACK cd95c9d

Tree-SHA512: f9b60b932957d2e1cad1d87f2ad8bb68c97136e9735eb78547018a42cc50c4652750367f29462eadb0512c27db1dd8a7d4b17a2f0aeab62b3dbf86db5f51a61c

Author: hebasto

src/util/subprocess.h

@@ -65,13 +65,9 @@ Documentation for C++ subprocessing library.
 
 extern "C" {
 #ifdef __USING_WINDOWS__
-  #include <Windows.h>
+  #include <windows.h>
   #include <io.h>
   #include <cwchar>
-
-  #define close _close
-  #define open _open
-  #define fileno _fileno
 #else
   #include <sys/wait.h>
   #include <unistd.h>
@@ -81,6 +77,22 @@ extern "C" {
   #include <sys/types.h>
 }
 
+// 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:
@@ -159,6 +171,7 @@ class OSError: public std::runtime_error
 //--------------------------------------------------------------------
 namespace util
 {
+#ifdef __USING_WINDOWS__
   inline void quote_argument(const std::wstring &argument, std::wstring &command_line,
                       bool force)
   {
@@ -169,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 {
@@ -219,7 +232,6 @@ namespace util
     }
   }
 
-#ifdef __USING_WINDOWS__
   inline std::string get_last_error(DWORD errorMessageID)
   {
     if (errorMessageID == 0)
@@ -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);
     }
 
@@ -334,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);
+    }
   }
 
 
@@ -383,7 +399,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 +427,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;
 
@@ -527,7 +543,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;
 };
 
@@ -573,10 +589,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 +622,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 +653,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;
   }
@@ -758,7 +774,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<char>& msg);
@@ -795,36 +814,39 @@ 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();
 
   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(); }
@@ -1043,7 +1065,19 @@ inline int Popen::wait() noexcept(false)
 #ifdef __USING_WINDOWS__
   int ret = WaitForSingleObject(process_handle_, INFINITE);
 
-  return 0;
+  // 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;
   std::tie(ret, status) = util::wait_for_child_exit(child_pid_);
@@ -1156,8 +1190,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);
   }
 
@@ -1167,25 +1201,27 @@ 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();
 
     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) {
+          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);
+      fclose(err_fp);
 
       if (read_bytes || strlen(err_buf)) {
         // Call waitpid to reap the child process
@@ -1271,13 +1307,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());
@@ -1304,15 +1340,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"));