util: Remove brittle and confusing sp::Popen(std::string)

src/common/run_command.cpp

@@ -8,12 +8,13 @@
 
 #include <tinyformat.h>
 #include <univalue.h>
+#include <util/string.h>
 
 #ifdef ENABLE_EXTERNAL_SIGNER
 #include <util/subprocess.h>
 #endif // ENABLE_EXTERNAL_SIGNER
 
-UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in)
+UniValue RunCommandParseJSON(const std::vector<std::string>& cmd_args, const std::string& str_std_in)
 {
 #ifdef ENABLE_EXTERNAL_SIGNER
     namespace sp = subprocess;
@@ -22,9 +23,9 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
     std::istringstream stdout_stream;
     std::istringstream stderr_stream;
 
-    if (str_command.empty()) return UniValue::VNULL;
+    if (cmd_args.empty()) return UniValue::VNULL;
 
-    auto c = sp::Popen(str_command, sp::input{sp::PIPE}, sp::output{sp::PIPE}, sp::error{sp::PIPE});
+    auto c = sp::Popen(cmd_args, sp::input{sp::PIPE}, sp::output{sp::PIPE}, sp::error{sp::PIPE});
     if (!str_std_in.empty()) {
         c.send(str_std_in);
     }
@@ -38,7 +39,7 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
     std::getline(stderr_stream, error);
 
     const int n_error = c.retcode();
-    if (n_error) throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", str_command, n_error, error));
+    if (n_error) throw std::runtime_error(strprintf("RunCommandParseJSON error: process(%s) returned %d: %s\n", util::Join(cmd_args, " "), n_error, error));
     if (!result_json.read(result)) throw std::runtime_error("Unable to parse JSON: " + result);
 
     return result_json;

src/common/run_command.h

@@ -6,16 +6,17 @@
 #define BITCOIN_COMMON_RUN_COMMAND_H
 
 #include <string>
+#include <vector>
 
 class UniValue;
 
 /**
  * Execute a command which returns JSON, and parse the result.
  *
- * @param str_command The command to execute, including any arguments
+ * @param cmd_args The command and arguments
  * @param str_std_in string to pass to stdin
  * @return parsed JSON
  */
-UniValue RunCommandParseJSON(const std::string& str_command, const std::string& str_std_in="");
+UniValue RunCommandParseJSON(const std::vector<std::string>& cmd_args, const std::string& str_std_in = "");
 
 #endif // BITCOIN_COMMON_RUN_COMMAND_H

src/external_signer.cpp

@@ -9,24 +9,27 @@
 #include <core_io.h>
 #include <psbt.h>
 #include <util/strencodings.h>
+#include <util/subprocess.h>
 
 #include <algorithm>
 #include <stdexcept>
 #include <string>
 #include <vector>
 
-ExternalSigner::ExternalSigner(std::string command, std::string chain, std::string fingerprint, std::string name)
+ExternalSigner::ExternalSigner(std::vector<std::string> command, std::string chain, std::string fingerprint, std::string name)
     : m_command{std::move(command)}, m_chain{std::move(chain)}, m_fingerprint{std::move(fingerprint)}, m_name{std::move(name)} {}
 
-std::string ExternalSigner::NetworkArg() const
+std::vector<std::string> ExternalSigner::NetworkArg() const
 {
-    return " --chain " + m_chain;
+    return {"--chain", m_chain};
 }
 
 bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalSigner>& signers, const std::string& chain)
 {
     // Call <command> enumerate
-    const UniValue result = RunCommandParseJSON(command + " enumerate");
+    std::vector<std::string> cmd_args = Cat(subprocess::util::split(command), {"enumerate"});
+
+    const UniValue result = RunCommandParseJSON(cmd_args, "");
     if (!result.isArray()) {
         throw std::runtime_error(strprintf("'%s' received invalid response, expected array of signers", command));
     }
@@ -56,19 +59,19 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS
         if (model_field.isStr() && model_field.getValStr() != "") {
             name += model_field.getValStr();
         }
-        signers.emplace_back(command, chain, fingerprintStr, name);
+        signers.emplace_back(subprocess::util::split(command), chain, fingerprintStr, name);
     }
     return true;
 }
 
 UniValue ExternalSigner::DisplayAddress(const std::string& descriptor) const
 {
-    return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " displayaddress --desc " + descriptor);
+    return RunCommandParseJSON(Cat(m_command, Cat(Cat({"--fingerprint", m_fingerprint}, NetworkArg()), {"displayaddress", "--desc", descriptor})), "");
 }
 
 UniValue ExternalSigner::GetDescriptors(const int account)
 {
-    return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " getdescriptors --account " + strprintf("%d", account));
+    return RunCommandParseJSON(Cat(m_command, Cat(Cat({"--fingerprint", m_fingerprint}, NetworkArg()), {"getdescriptors", "--account", strprintf("%d", account)})), "");
 }
 
 bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::string& error)
@@ -94,7 +97,7 @@ bool ExternalSigner::SignTransaction(PartiallySignedTransaction& psbtx, std::str
         return false;
     }
 
-    const std::string command = m_command + " --stdin --fingerprint " + m_fingerprint + NetworkArg();
+    const std::vector<std::string> command = Cat(m_command, Cat({"--stdin", "--fingerprint", m_fingerprint}, NetworkArg()));
     const std::string stdinStr = "signtx " + EncodeBase64(ssTx.str());
 
     const UniValue signer_result = RunCommandParseJSON(command, stdinStr);

src/external_signer.h

@@ -19,19 +19,19 @@ class ExternalSigner
 {
 private:
     //! The command which handles interaction with the external signer.
-    std::string m_command;
+    std::vector<std::string> m_command;
 
     //! Bitcoin mainnet, testnet, etc
     std::string m_chain;
 
-    std::string NetworkArg() const;
+    std::vector<std::string> NetworkArg() const;
 
 public:
     //! @param[in] command      the command which handles interaction with the external signer
     //! @param[in] fingerprint  master key fingerprint of the signer
     //! @param[in] chain        "main", "test", "regtest" or "signet"
     //! @param[in] name         device name
-    ExternalSigner(std::string command, std::string chain, std::string fingerprint, std::string name);
+    ExternalSigner(std::vector<std::string> command, std::string chain, std::string fingerprint, std::string name);
 
     //! Master key fingerprint of the signer
     std::string m_fingerprint;

src/test/system_tests.cpp

@@ -4,9 +4,11 @@
 //
 
 #include <bitcoin-build-config.h> // IWYU pragma: keep
-#include <test/util/setup_common.h>
+
 #include <common/run_command.h>
+#include <test/util/setup_common.h>
 #include <univalue.h>
+#include <util/string.h>
 
 #ifdef ENABLE_EXTERNAL_SIGNER
 #include <util/subprocess.h>
@@ -21,14 +23,14 @@ BOOST_FIXTURE_TEST_SUITE(system_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(run_command)
 {
     {
-        const UniValue result = RunCommandParseJSON("");
+        const UniValue result = RunCommandParseJSON({});
         BOOST_CHECK(result.isNull());
     }
     {
 #ifdef WIN32
-        const UniValue result = RunCommandParseJSON("cmd.exe /c echo {\"success\": true}");
+        const UniValue result = RunCommandParseJSON({"cmd.exe", "/c", "echo", "{\"success\":", "true}"}); // The command is intentionally split "incorrectly", to exactly preserve previous behavior. This is due to the cmd.exe internal echo quoting strings with spaces in it, unlike the normal 'echo' below.
 #else
-        const UniValue result = RunCommandParseJSON("echo {\"success\": true}");
+        const UniValue result = RunCommandParseJSON({"echo", "{\"success\": true}"});
 #endif
         BOOST_CHECK(result.isObject());
         const UniValue& success = result.find_value("success");
@@ -42,32 +44,32 @@ BOOST_AUTO_TEST_CASE(run_command)
 #else
         const std::string expected{"execve failed: "};
 #endif
-        BOOST_CHECK_EXCEPTION(RunCommandParseJSON("invalid_command"), subprocess::CalledProcessError, HasReason(expected));
+        BOOST_CHECK_EXCEPTION(RunCommandParseJSON({"invalid_command"}), subprocess::CalledProcessError, HasReason(expected));
     }
     {
         // Return non-zero exit code, no output to stderr
 #ifdef WIN32
-        const std::string command{"cmd.exe /c exit 1"};
+        const std::vector<std::string> command = {"cmd.exe", "/c", "exit 1"};
 #else
-        const std::string command{"false"};
+        const std::vector<std::string> command = {"false"};
 #endif
         BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
             const std::string what{e.what()};
-            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned 1: \n", command)) != std::string::npos);
+            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned 1: \n", util::Join(command, " "))) != std::string::npos);
             return true;
         });
     }
     {
         // Return non-zero exit code, with error message for stderr
 #ifdef WIN32
-        const std::string command{"cmd.exe /c \"echo err 1>&2 && exit 1\""};
+        const std::vector<std::string> command = {"cmd.exe", "/c", "echo err 1>&2 && exit 1"};
 #else
-        const std::string command{"sh -c 'echo err 1>&2 && false'"};
+        const std::vector<std::string> command = {"sh", "-c", "echo err 1>&2 && false"};
 #endif
         const std::string expected{"err"};
         BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
             const std::string what(e.what());
-            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos);
+            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned", util::Join(command, " "))) != std::string::npos);
             BOOST_CHECK(what.find(expected) != std::string::npos);
             return true;
         });
@@ -75,16 +77,16 @@ BOOST_AUTO_TEST_CASE(run_command)
     {
         // Unable to parse JSON
 #ifdef WIN32
-        const std::string command{"cmd.exe /c echo {"};
+        const std::vector<std::string> command = {"cmd.exe", "/c", "echo {"};
 #else
-        const std::string command{"echo {"};
+        const std::vector<std::string> command = {"echo", "{"};
 #endif
         BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, HasReason("Unable to parse JSON: {"));
     }
 #ifndef WIN32
     {
         // Test stdin
-        const UniValue result = RunCommandParseJSON("cat", "{\"success\": true}");
+        const UniValue result = RunCommandParseJSON({"cat"}, "{\"success\": true}");
         BOOST_CHECK(result.isObject());
         const UniValue& success = result.find_value("success");
         BOOST_CHECK(!success.isNull());

src/util/subprocess.h

@@ -920,8 +920,6 @@ private:
  * API's provided by the class:
  * Popen({"cmd"}, output{..}, error{..}, ....)
  *    Command provided as a sequence.
- * Popen("cmd arg1", output{..}, error{..}, ....)
- *    Command provided in a single string.
  * wait()             - Wait for the child to exit.
  * retcode()          - The return code of the exited child.
  * send(...)          - Send input to the input channel of the child.
@@ -934,19 +932,6 @@ public:
   friend struct detail::ArgumentDeducer;
   friend class detail::Child;
 
-  template <typename... Args>
-  Popen(const std::string& cmd_args, Args&& ...args):
-    args_(cmd_args)
-  {
-    vargs_ = util::split(cmd_args);
-    init_args(std::forward<Args>(args)...);
-
-    // Setup the communication channels of the Popen class
-    stream_.setup_comm_channels();
-
-    execute_process();
-  }
-
   template <typename... Args>
   Popen(std::initializer_list<const char*> cmd_args, Args&& ...args)
   {
@@ -1028,8 +1013,6 @@ private:
 
   std::string exe_name_;
 
-  // Command in string format
-  std::string args_;
   // Command provided as sequence
   std::vector<std::string> vargs_;
   std::vector<char*> cargv_;