highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-01 00:34:01 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#34349: util: Remove brittle and confusing sp::Popen(std::string)

Browse Source 
fa48d42163 test: Stricter unit test (MarcoFalke)
fa626bd143 util: Remove brittle and confusing sp::Popen(std::string) (MarcoFalke)

Pull request description:

  The subprocess Popen call that accepts a full `std::string` has many issues:

  * It promotes brittle and broken code, where spaces are not properly quoted. Example: https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065
  * The internally used `util::split` function does incorrectly split on spaces, instead of using `shlex.split`.
  * It is redundant and not needed, because a vector interface already exists.

  Fix all issues by removing it and just using the vector interface.

  This pull request should not change any behavior: Note that the command taken from `gArgs.GetArg("-signer", "")` is still passed through the `sp::util::split` helper, just like before. Fixing that is left for a follow-up, so that this change here is basically just a refactor.

  This also fixes a unit test bug as a side-effect: Fixes https://github.com/bitcoin/bitcoin/issues/32574.

ACKs for top commit:
  janb84:
    cr ACK fa48d42163
  fjahr:
    Code review ACK fa48d42163
  hebasto:
    re-ACK fa48d42163.

Tree-SHA512: 3d29226977c9392502f9361e2bd42b471ad03761bbf6a94ef6e545cbe4492ad5858da1ac9cc64b2791aacb9b6e6f3c3f63dbcc3a2bf45f6a13b5bc33eddf8c2b
This commit is contained in:
merge-script
2026-02-18 10:18:25 +00:00
parent 59e10a5463 fa48d42163
commit 2706758dc3
6 changed files with 38 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
src/test/system_tests.cpp
View File

@@ -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 %s: %s", util::Join(command, " "), 1, "err")) != 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());