Merge bitcoin/bitcoin#29865: util: remove unused cpp-subprocess options

13adbf733f09c73c3cf0025d94c52f9cec5dba3b remove unneeded environment option from cpp-subprocess (Sebastian Falbesoner)
2088777ba0f9ad3f6d4ab8b0b6ff8aad71117307 remove unneeded cwd option from cpp-subprocess (Sebastian Falbesoner)
03ffb09c31aa04cc296c0ce10d07109e22a8dd75 remove unneeded bufsize option from cpp-subprocess (Sebastian Falbesoner)
79c30363733503a1fb7d4c98aa0d56ced0be6e32 remove unneeded close_fds option from cpp-subprocess (Sebastian Falbesoner)
62db8f8e5a6cfe19d905afc91731d6bc8a665f61 remove unneeded session_leader option from cpp-subprocess (Sebastian Falbesoner)
80d008c66d00d3496cd8549daee6775cf2c6b782 remove unneeded defer_spawn option from cpp-subprocess (Sebastian Falbesoner)
cececad7b29e2ca3de1216db1c541dba6dc81bfa remove unneeded preexec function option from cpp-subprocess (Sebastian Falbesoner)
633e45b2e2728efcb0637afa94fcbd5756dfbe76 remove unneeded shell option from cpp-subprocess (Sebastian Falbesoner)

Pull request description:

  The newly introduced cpp-subprocess library provides a good number of options for the `Popen` class:
  https://github.com/bitcoin/bitcoin/blob/0de63b8b46eff5cda85b4950062703324ba65a80/src/util/subprocess.hpp#L1009-L1020
  Some of them are either not fully implemented (`shell`, missing an implementation on Windows), implemented in an ugly way (e.g. using "Impoverished, meager, needy, truly needy version of type erasure" for `preexec_func` according to the author's own words) or simply unlikely to be ever needed for our external signer use-case (`defer_spawn`). Instead of maintaining incomplete and/or unneeded code, I'd suggest to get rid of it and only keep support for options if there is a strong reason for it.

ACKs for top commit:
  achow101:
    ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b
  hebasto:
    re-ACK 13adbf733f09c73c3cf0025d94c52f9cec5dba3b.

Tree-SHA512: 8270da27891cb659da2ef6062a23f4b86331859b15ac27b79ae7433b14f5bd7efaba621f2b3ba1953708d0f38377a8bd23ef1cc0f28b9c152ac8958dd9eec6b0

1 files changed, 9 insertions, 385 deletions

diff --git a/src/util/subprocess.hpp b/src/util/subprocess.hpp
index e660aa143d..701eec8c10 100644
--- a/src/util/subprocess.hpp
+++ b/src/util/subprocess.hpp
@@ -155,19 +155,6 @@ public:
 };
 
 //--------------------------------------------------------------------
-
-//Environment Variable types
-#ifndef _MSC_VER
-	using env_string_t = std::string;
-	using env_char_t = char;
-#else
-	using env_string_t = std::wstring;
-	using env_char_t = wchar_t;
-#endif
-using env_map_t = std::map<env_string_t, env_string_t>;
-using env_vector_t = std::vector<env_char_t>;
-
-//--------------------------------------------------------------------
 namespace util
 {
   template <typename R>
@@ -305,100 +292,6 @@ namespace util
     if (!SetHandleInformation(*child_handle, HANDLE_FLAG_INHERIT, 0))
       throw OSError("SetHandleInformation", 0);
   }
-
-  // env_map_t MapFromWindowsEnvironment()
-  // * Imports current Environment in a C-string table
-  // * Parses the strings by splitting on the first "=" per line
-  // * Creates a map of the variables
-  // * Returns the map
-  inline env_map_t MapFromWindowsEnvironment(){
-      wchar_t *variable_strings_ptr;
-      wchar_t *environment_strings_ptr;
-      std::wstring delimiter(L"=");
-      int del_len = delimiter.length();
-      env_map_t mapped_environment;
-
-      // Get a pointer to the environment block.
-      environment_strings_ptr = GetEnvironmentStringsW();
-      // If the returned pointer is NULL, exit.
-      if (environment_strings_ptr == NULL)
-      {
-          throw OSError("GetEnvironmentStringsW", 0);
-      }
-
-      // Variable strings are separated by NULL byte, and the block is
-      // terminated by a NULL byte.
-
-      variable_strings_ptr = (wchar_t *) environment_strings_ptr;
-
-      //Since the environment map ends with a null, we can loop until we find it.
-      while (*variable_strings_ptr)
-      {
-          // Create a string from Variable String
-          env_string_t current_line(variable_strings_ptr);
-          // Find the first "equals" sign.
-          auto pos = current_line.find(delimiter);
-          // Assuming it's not missing ...
-          if(pos!=std::wstring::npos){
-              // ... parse the key and value.
-              env_string_t key = current_line.substr(0, pos);
-              env_string_t value = current_line.substr(pos + del_len);
-              // Map the entry.
-              mapped_environment[key] = value;
-          }
-          // Jump to next line in the environment map.
-          variable_strings_ptr += std::wcslen(variable_strings_ptr) + 1;
-      }
-      // We're done with the old environment map buffer.
-      FreeEnvironmentStringsW(environment_strings_ptr);
-
-      // Return the map.
-      return mapped_environment;
-  }
-
-  // env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map)
-  // * Creates a vector buffer for the new environment string table
-  // * Copies in the mapped variables
-  // * Returns the vector
-  inline env_vector_t WindowsEnvironmentVectorFromMap(const env_map_t &source_map)
-  {
-	// Make a new environment map buffer.
-	env_vector_t environment_map_buffer;
-	// Give it some space.
-	environment_map_buffer.reserve(4096);
-
-	// And fill'er up.
-	for(auto kv: source_map){
-	  // Create the line
-	  env_string_t current_line(kv.first); current_line += L"="; current_line += kv.second;
-	  // Add the line to the buffer.
-	  std::copy(current_line.begin(), current_line.end(), std::back_inserter(environment_map_buffer));
-	  // Append a null
-	  environment_map_buffer.push_back(0);
-	}
-	// Append one last null because of how Windows does it's environment maps.
-	environment_map_buffer.push_back(0);
-
-	return environment_map_buffer;
-  }
-
-  // env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map)
-  // * Merges host environment with new mapped variables
-  // * Creates and returns string vector based on map
-  inline env_vector_t CreateUpdatedWindowsEnvironmentVector(const env_map_t &changes_map){
-	// Import the environment map
-	env_map_t environment_map = MapFromWindowsEnvironment();
-    // Merge in the changes with overwrite
-	for(auto& it: changes_map)
-	{
-		environment_map[it.first] = it.second;
-	}
-    // Create a Windows-usable Environment Map Buffer
-    env_vector_t environment_map_strings_vector = WindowsEnvironmentVectorFromMap(environment_map);
-
-	return environment_map_strings_vector;
-  }
-
 #endif
 
   /*!
@@ -431,26 +324,6 @@ namespace util
   }
 
 
-  /*!
-   * Function: join
-   * Parameters:
-   * [in] vec : Vector of strings which needs to be joined to form
-   *            a single string with words separated by a separator char.
-   *  [in] sep : String used to separate 2 words in the joined string.
-   *             Default constructed to ' ' (space).
-   *  [out] string: Joined string.
-   */
-  static inline
-  std::string join(const std::vector<std::string>& vec,
-                   const std::string& sep = " ")
-  {
-    std::string res;
-    for (auto& elem : vec) res.append(elem + sep);
-    res.erase(--res.end());
-    return res;
-  }
-
-
 #ifndef __USING_WINDOWS__
   /*!
    * Function: set_clo_on_exec
@@ -652,56 +525,6 @@ namespace util
  */
 
 /*!
- * The buffer size of the stdin/stdout/stderr
- * streams of the child process.
- * Default value is 0.
- */
-struct bufsize {
-  explicit bufsize(int sz): bufsiz(sz) {}
-  int  bufsiz = 0;
-};
-
-/*!
- * Option to defer spawning of the child process
- * till `Popen::start_process` API is called.
- * Default value is false.
- */
-struct defer_spawn {
-  explicit defer_spawn(bool d): defer(d) {}
-  bool defer  = false;
-};
-
-/*!
- * Option to close all file descriptors
- * when the child process is spawned.
- * The close fd list does not include
- * input/output/error if they are explicitly
- * set as part of the Popen arguments.
- *
- * Default value is false.
- */
-struct close_fds {
-  explicit close_fds(bool c): close_all(c) {}
-  bool close_all = false;
-};
-
-/*!
- * Option to make the child process as the
- * session leader and thus the process
- * group leader.
- * Default value is false.
- */
-struct session_leader {
-  explicit session_leader(bool sl): leader_(sl) {}
-  bool leader_ = false;
-};
-
-struct shell {
-  explicit shell(bool s): shell_(s) {}
-  bool shell_ = false;
-};
-
-/*!
  * Base class for all arguments involving string value.
  */
 struct string_arg
@@ -727,34 +550,6 @@ struct executable: string_arg
 };
 
 /*!
- * Option to set the current working directory
- * of the spawned process.
- *
- * Eg: cwd{"/some/path/x"}
- */
-struct cwd: string_arg
-{
-  template <typename T>
-  cwd(T&& arg): string_arg(std::forward<T>(arg)) {}
-};
-
-/*!
- * Option to specify environment variables required by
- * the spawned process.
- *
- * Eg: environment{{ {"K1", "V1"}, {"K2", "V2"},... }}
- */
-struct environment
-{
-  environment(env_map_t&& env):
-    env_(std::move(env)) {}
-  explicit environment(const env_map_t& env):
-    env_(env) {}
-  env_map_t env_;
-};
-
-
-/*!
  * Used for redirecting input/output/error
  */
 enum IOTYPE {
@@ -870,44 +665,6 @@ struct error
   int wr_ch_ = -1;
 };
 
-// Impoverished, meager, needy, truly needy
-// version of type erasure to store function pointers
-// needed to provide the functionality of preexec_func
-// ATTN: Can be used only to execute functions with no
-// arguments and returning void.
-// Could have used more efficient methods, ofcourse, but
-// that won't yield me the consistent syntax which I am
-// aiming for. If you know, then please do let me know.
-
-class preexec_func
-{
-public:
-  preexec_func() {}
-
-  template <typename Func>
-  explicit preexec_func(Func f): holder_(new FuncHolder<Func>(std::move(f)))
-  {}
-
-  void operator()() {
-    (*holder_)();
-  }
-
-private:
-  struct HolderBase {
-    virtual void operator()() const = 0;
-    virtual ~HolderBase(){};
-  };
-  template <typename T>
-  struct FuncHolder: HolderBase {
-    FuncHolder(T func): func_(std::move(func)) {}
-    void operator()() const override { func_(); }
-    // The function pointer/reference
-    T func_;
-  };
-
-  std::unique_ptr<HolderBase> holder_ = nullptr;
-};
-
 // ~~~~ End Popen Args ~~~~
 
 
@@ -1007,17 +764,9 @@ struct ArgumentDeducer
   ArgumentDeducer(Popen* p): popen_(p) {}
 
   void set_option(executable&& exe);
-  void set_option(cwd&& cwdir);
-  void set_option(bufsize&& bsiz);
-  void set_option(environment&& env);
-  void set_option(defer_spawn&& defer);
-  void set_option(shell&& sh);
   void set_option(input&& inp);
   void set_option(output&& out);
   void set_option(error&& err);
-  void set_option(close_fds&& cfds);
-  void set_option(preexec_func&& prefunc);
-  void set_option(session_leader&& sleader);
 
 private:
   Popen* popen_ = nullptr;
@@ -1168,9 +917,6 @@ public:// Yes they are public
   HANDLE g_hChildStd_ERR_Wr = nullptr;
 #endif
 
-  // Buffer size for the IO streams
-  int bufsiz_ = 0;
-
   // Pipes for communicating with child
 
   // Emulates stdin
@@ -1200,9 +946,9 @@ private:
  * interface to the client.
  *
  * API's provided by the class:
- * 1. Popen({"cmd"}, output{..}, error{..}, cwd{..}, ....)
+ * 1. Popen({"cmd"}, output{..}, error{..}, ....)
  *    Command provided as a sequence.
- * 2. Popen("cmd arg1"m output{..}, error{..}, cwd{..}, ....)
+ * 2. Popen("cmd arg1"m output{..}, error{..}, ....)
  *    Command provided in a single string.
  * 3. wait()             - Wait for the child to exit.
  * 4. retcode()          - The return code of the exited child.
@@ -1218,8 +964,6 @@ private:
                            in case of redirection. See piping examples.
  *12. error()            - Get the error channel/File pointer. Usually used
                            in case of redirection.
- *13. start_process()    - Start the child process. Only to be used when
- *                         `defer_spawn` option was provided in Popen constructor.
  */
 class Popen
 {
@@ -1237,7 +981,7 @@ public:
     // Setup the communication channels of the Popen class
     stream_.setup_comm_channels();
 
-    if (!defer_process_start_) execute_process();
+    execute_process();
   }
 
   template <typename... Args>
@@ -1249,7 +993,7 @@ public:
     // Setup the communication channels of the Popen class
     stream_.setup_comm_channels();
 
-    if (!defer_process_start_) execute_process();
+    execute_process();
   }
 
   template <typename... Args>
@@ -1260,7 +1004,7 @@ public:
     // Setup the communication channels of the Popen class
     stream_.setup_comm_channels();
 
-    if (!defer_process_start_) execute_process();
+    execute_process();
   }
 
 /*
@@ -1272,8 +1016,6 @@ public:
   }
 */
 
-  void start_process() noexcept(false);
-
   int pid() const noexcept { return child_pid_; }
 
   int retcode() const noexcept { return retcode_; }
@@ -1347,16 +1089,7 @@ private:
   std::future<void> cleanup_future_;
 #endif
 
-  bool defer_process_start_ = false;
-  bool close_fds_ = false;
-  bool has_preexec_fn_ = false;
-  bool shell_ = false;
-  bool session_leader_ = false;
-
   std::string exe_name_;
-  std::string cwd_;
-  env_map_t env_;
-  preexec_func preexec_fn_;
 
   // Command in string format
   std::string args_;
@@ -1391,20 +1124,6 @@ inline void Popen::populate_c_argv()
   cargv_.push_back(nullptr);
 }
 
-inline void Popen::start_process() noexcept(false)
-{
-  // The process was started/tried to be started
-  // in the constructor itself.
-  // For explicitly calling this API to start the
-  // process, 'defer_spawn' argument must be set to
-  // true in the constructor.
-  if (!defer_process_start_) {
-    assert (0);
-    return;
-  }
-  execute_process();
-}
-
 inline int Popen::wait() noexcept(false)
 {
 #ifdef __USING_WINDOWS__
@@ -1483,8 +1202,7 @@ inline void Popen::kill(int sig_num)
     throw OSError("TerminateProcess", 0);
   }
 #else
-  if (session_leader_) killpg(child_pid_, sig_num);
-  else ::kill(child_pid_, sig_num);
+  ::kill(child_pid_, sig_num);
 #endif
 }
 
@@ -1492,17 +1210,6 @@ inline void Popen::kill(int sig_num)
 inline void Popen::execute_process() noexcept(false)
 {
 #ifdef __USING_WINDOWS__
-  if (this->shell_) {
-    throw OSError("shell not currently supported on windows", 0);
-  }
-
-  void* environment_string_table_ptr = nullptr;
-  env_vector_t environment_string_vector;
-  if(this->env_.size()){
-	  environment_string_vector = util::CreateUpdatedWindowsEnvironmentVector(env_);
-	  environment_string_table_ptr = (void*)environment_string_vector.data();
-  }
-
   if (exe_name_.length()) {
     this->vargs_.insert(this->vargs_.begin(), this->exe_name_);
     this->populate_c_argv();
@@ -1549,7 +1256,7 @@ inline void Popen::execute_process() noexcept(false)
                             NULL,         // primary thread security attributes
                             TRUE,         // handles are inherited
                             creation_flags,	// creation flags
-                            environment_string_table_ptr,  // use provided environment
+                            NULL,         // use parent's environment
                             NULL,         // use parent's current directory
                             &siStartInfo, // STARTUPINFOW pointer
                             &piProcInfo); // receives PROCESS_INFORMATION
@@ -1588,14 +1295,6 @@ inline void Popen::execute_process() noexcept(false)
   int err_rd_pipe, err_wr_pipe;
   std::tie(err_rd_pipe, err_wr_pipe) = util::pipe_cloexec();
 
-  if (shell_) {
-    auto new_cmd = util::join(vargs_);
-    vargs_.clear();
-    vargs_.insert(vargs_.begin(), {"/bin/sh", "-c"});
-    vargs_.push_back(new_cmd);
-    populate_c_argv();
-  }
-
   if (exe_name_.length()) {
     vargs_.insert(vargs_.begin(), exe_name_);
     populate_c_argv();
@@ -1662,30 +1361,6 @@ namespace detail {
     popen_->exe_name_ = std::move(exe.arg_value);
   }
 
-  inline void ArgumentDeducer::set_option(cwd&& cwdir) {
-    popen_->cwd_ = std::move(cwdir.arg_value);
-  }
-
-  inline void ArgumentDeducer::set_option(bufsize&& bsiz) {
-    popen_->stream_.bufsiz_ = bsiz.bufsiz;
-  }
-
-  inline void ArgumentDeducer::set_option(environment&& env) {
-    popen_->env_ = std::move(env.env_);
-  }
-
-  inline void ArgumentDeducer::set_option(defer_spawn&& defer) {
-    popen_->defer_process_start_ = defer.defer;
-  }
-
-  inline void ArgumentDeducer::set_option(shell&& sh) {
-    popen_->shell_ = sh.shell_;
-  }
-
-  inline void ArgumentDeducer::set_option(session_leader&& sleader) {
-    popen_->session_leader_ = sleader.leader_;
-  }
-
   inline void ArgumentDeducer::set_option(input&& inp) {
     if (inp.rd_ch_ != -1) popen_->stream_.read_from_parent_ = inp.rd_ch_;
     if (inp.wr_ch_ != -1) popen_->stream_.write_to_child_ = inp.wr_ch_;
@@ -1708,15 +1383,6 @@ namespace detail {
     if (err.rd_ch_ != -1) popen_->stream_.err_read_ = err.rd_ch_;
   }
 
-  inline void ArgumentDeducer::set_option(close_fds&& cfds) {
-    popen_->close_fds_ = cfds.close_all;
-  }
-
-  inline void ArgumentDeducer::set_option(preexec_func&& prefunc) {
-    popen_->preexec_fn_ = std::move(prefunc);
-    popen_->has_preexec_fn_ = true;
-  }
-
 
   inline void Child::execute_child() {
 #ifndef __USING_WINDOWS__
@@ -1763,41 +1429,8 @@ namespace detail {
       if (stream.err_write_ != -1 && stream.err_write_ > 2)
         close(stream.err_write_);
 
-      // Close all the inherited fd's except the error write pipe
-      if (parent_->close_fds_) {
-        int max_fd = sysconf(_SC_OPEN_MAX);
-        if (max_fd == -1) throw OSError("sysconf failed", errno);
-
-        for (int i = 3; i < max_fd; i++) {
-          if (i == err_wr_pipe_) continue;
-          close(i);
-        }
-      }
-
-      // Change the working directory if provided
-      if (parent_->cwd_.length()) {
-        sys_ret = chdir(parent_->cwd_.c_str());
-        if (sys_ret == -1) throw OSError("chdir failed", errno);
-      }
-
-      if (parent_->has_preexec_fn_) {
-        parent_->preexec_fn_();
-      }
-
-      if (parent_->session_leader_) {
-        sys_ret = setsid();
-        if (sys_ret == -1) throw OSError("setsid failed", errno);
-      }
-
       // Replace the current image with the executable
-      if (parent_->env_.size()) {
-        for (auto& kv : parent_->env_) {
-          setenv(kv.first.c_str(), kv.second.c_str(), 1);
-        }
-        sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());
-      } else {
-        sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());
-      }
+      sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());
 
       if (sys_ret == -1) throw OSError("execve failed", errno);
 
@@ -1840,16 +1473,7 @@ namespace detail {
 
     for (auto& h : handles) {
       if (h == nullptr) continue;
-      switch (bufsiz_) {
-      case 0:
-        setvbuf(h, nullptr, _IONBF, BUFSIZ);
-        break;
-      case 1:
-        setvbuf(h, nullptr, _IONBF, BUFSIZ);
-        break;
-      default:
-        setvbuf(h, nullptr, _IOFBF, bufsiz_);
-      };
+      setvbuf(h, nullptr, _IONBF, BUFSIZ);
     }
   #endif
   }