re: #15382 (review)
I think the current PR is ok, and any followup can happen later. This is kind of a mess, but just read details below if interested in what I think is wrong here.
I think it is bad if an external signer option isn’t going to execute commands as straight shell commands the way -alertnotify -blocknotify and -walletnotify do and instead is going to use a strange, undocumented boost-shell splitting syntax, that doesn’t even try to try be posix compatible like python’s shlex.split().
I was going to suggest the following change to stop using boost’s argument splitter and instead pass commands straight to the system shell the same way -alertnotify -blocknotify and -walletnotify do:
0diff --git a/src/util/system.cpp b/src/util/system.cpp
1index 066cfe88925..24f6869a871 100644
2--- a/src/util/system.cpp
3+++ b/src/util/system.cpp
4@@ -1178,6 +1178,7 @@ UniValue RunCommandParseJSON(const std::string& str_command, const std::string&
5 if (str_command.empty()) return UniValue::VNULL;
6
7 bp::child c(
8+ bp::shell,
9 str_command,
10 bp::std_out > stdout_stream,
11 bp::std_err > stderr_stream,
12diff --git a/src/test/system_tests.cpp b/src/test/system_tests.cpp
13index a55145c7383..529fd74e472 100644
14--- a/src/test/system_tests.cpp
15+++ b/src/test/system_tests.cpp
16@@ -57,7 +57,7 @@ BOOST_AUTO_TEST_CASE(run_command)
17 const UniValue result = RunCommandParseJSON("echo '{\"success\": true}'");
18 #else
19 // ... but Linux and macOS echo a single quote if it's used
20- const UniValue result = RunCommandParseJSON("echo \"{\"success\": true}\"");
21+ const UniValue result = RunCommandParseJSON("echo '{\"success\": true}'");
22 #endif
23 BOOST_CHECK(result.isObject());
24 const UniValue& success = find_value(result, "success");
But it turns out there is a bug in boost so this doesn’t work: https://github.com/boostorg/process/issues/95. The bug is caused by commit https://github.com/boostorg/process/commit/c31f69725c3160a7a0adce744ed9d4c0a1a4afbe which seems kind of insane and more alarming than the shell splitting code above. The joining code added in this commit is wrong and vulnerable to shell code injection. And the extra quotes added in the cmd_shell() function there just flat-out break boost’s shell option on posix.
Separately, why the test above works on windows (thank you for the appveyor link) is still kind of a mystery, but I bet appveyor has some nonstandard echo.exe on the windows PATH, maybe part of git or mingw or msys or wsl, since current code is just going to invoke CreateProcess without a shell.
In practice, I’m sure none of this matters at all, and not sure what I would suggest here. I think the ideal thing would be to use the bp::shell like the diff above and have bug https://github.com/boostorg/process/issues/95 fixed upstream. But the workaround used in https://github.com/boostorg/process/issues/95 is also pretty reasonable, and something we could adopt.
I’m a little disappointed boost::process has these shell issues, since they seem like bad misunderstandings, and boost::process otherwise looks like a flexible and nicely-designed library.
