subprocess: Let shell parse command on non-Windows systems #32577

hebasto · 2025-05-21T11:45:24Z

The subprocess::Popen constructor has two overloads:

Lines 938 to 941 in 7763e86

    
            * Popen({"cmd"}, output{..}, error{..}, ....) 
        
            *    Command provided as a sequence. 
        
            * Popen("cmd arg1", output{..}, error{..}, ....) 
        
            *    Command provided in a single string.

During the migration from Boost.Process in #28981, the second was chosen for two reasons: (1) it minimized changes at the call sites, and (2) it addressed quoting issues on Windows. This approach internally uses the subprocess::util::split() function, which currently fails to handle quoted tokens, such as in subshell invocations like sh -c 'ls; echo'.

This issue was not caught by our tests as they are broken. The last commit fixes the system_tests/run_command test. This commit can be applied on top of the master branch to reproduce the existing failure in subprocess.

The second commit is intended to be upstreamed. However, I'm seeking code review feedback first (please note that the src/util/subprocess.h code is C++11 compatible).

DrahtBot · 2025-05-21T11:45:27Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32577.

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33063 (util: Revert "common: Close non-std fds before exec in RunCommandJSON" by maflcko)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

Vector of strings which needs to be joined -> Vector of strings which need to be joined [Subject-verb agreement error: "strings" is plural, so "need" is the correct verb]

^{drahtbot_id_0520}

laanwj · 2025-05-21T12:00:06Z

It's ambiguous how to split arguments. The UNIX shell does it in one way (which is pretty nice and supports quoting), Windows shell does it another way. In #32566 we want the OS shell's behavior, because the notifications have historically invoked the shell.

However, for RunCommandParseJSON (as changed here) we don't use a shell, so have to implement our own splitting if we want that. And i guess emulating a UNIX shell makes more sense, as it's more versatile. But there is no single "proper" way, just whatever we decide to implement (and should document).

Alternatively, can't we just pass a vector of arguments to RunCommandParseJSON and avoid splitting/merging? Hmm maybe not as it comes in through one argument string -externalsigner anyhow, there's just some additional arguments that we add. OK this is annoying.

src/util/subprocess.h

hebasto · 2025-05-21T12:12:14Z

Alternatively, can't we just pass a vector of arguments to RunCommandParseJSON and avoid splitting/merging?

This approach caused quoting-related issues on Windows, though I haven’t tested it with the current master branch.

laanwj · 2025-05-21T12:17:11Z

This approach #28981 (comment) quoting-related issues on Windows, though I haven’t tested it with the current master branch.

The thing on Windows is that CreateProcessW takes a single command line, instead of a list of arguments. So a "join" is unavoidable. It does do quoting on windows while joining this command line though-i don't know enough to know if it's correct: https://github.com/arun11299/cpp-subprocess/blob/master/cpp-subprocess/subprocess.hpp#L186. It does this independently of whether the command line was passed as a single string of a vector, the former just goes through split too, so i don't see how it causes that issue.

hebasto · 2025-05-21T14:20:13Z

Alternatively, can't we just pass a vector of arguments to RunCommandParseJSON and avoid splitting/merging? Hmm maybe not as it comes in through one argument string -externalsigner anyhow, there's just some additional arguments that we add. OK this is annoying.

Exactly. We have to parse and tokenize the -signer argument in cases like bitcoind -signer="/usr/bin/python3 /some_path/signer.py".

ryanofsky · 2025-05-21T15:01:18Z

I'd think for both windows and unix, a good approach could be not parse -signer strings, just leave it up to the shell on unix, and shell+appilcation itself on windows, because on windows the application runtime is responsible for splitting arguments, usually using CommandLineToArgv. This was also discussed pretty extensively in #13339.

If we can avoid to have a split function at all that would seem great.

hebasto · 2025-05-21T15:14:28Z

If we can avoid to have a split function at all that would seem great.

We can't avoid splitting when calling execvp:

bitcoin/src/util/subprocess.h

Line 1382 in 87ec923

sys_ret = execvp(parent_->exe_name_.c_str(), parent_->cargv_.data());

ryanofsky · 2025-05-21T16:27:13Z

We can't avoid splitting when calling execvp:

The suggestion is to let the shell do the splitting so we do not need to do it. We would call execvp with sh as the first command line argument -c as the second one, and the -signer value as the third one. This is also what the python subprocess module does when shell=True is passed.

hebasto · 2025-05-21T21:29:50Z

@laanwj @ryanofsky

Thank you for your feedback!

I've restored the shell option in the subprocess API, and employed it on non-Windows systems.

ryanofsky · 2025-05-21T22:56:47Z

I've restored the shell option in the subprocess API, and employed it on non-Windows systems.

Hmm the shell support implemented in eb16060 actually seems somewhat broken as it is still splitting and joining instead of preserving the original string passed to Popen() instead of just passing it unaltered to execvp as I was suggesting.

It actually might not work too badly in that commit as long as the original string doesn't contain any tab characters, because otherwise the split and join functions are inverses of each other. But the later commit c82420b seems like a mistake and I don't see how adding double quotes there could be good.

It could help to know more about the bug this is supposed to fix as I don't think I understand it.

fanquake · 2025-05-22T08:36:25Z

(please note that the src/util/subprocess.h code is C++11 compatible).

It seems a bit odd that we've imported a dependency that is pinned to an older C++ version than what we want to use, especially when we are trying to upstream changes to it. This being a nuisance has come up multiple times already (also here: #32343 (comment)). What is the longer term plan here?

hebasto · 2025-05-22T10:35:47Z

What is the longer term plan here?

The initial plan was to maintain our own code independently of the upstream repository. However, there have been a few requests to upstream our changes. At this point, I still believe we shouldn't concern ourselves with upstream and should instead take full advantage of C++20.

hebasto · 2025-05-22T14:14:39Z

I've restored the shell option in the subprocess API, and employed it on non-Windows systems.

Hmm the shell support implemented in eb16060 actually seems somewhat broken as it is still splitting and joining instead of preserving the original string passed to Popen() instead of just passing it unaltered to execvp as I was suggesting.

Thanks! Implemented as you suggested.

(no further cleanups in subprocess.h for now)

fanquake · 2025-05-22T15:13:06Z

src/test/system_tests.cpp

 #endif
        BOOST_CHECK(result.isObject());
        const UniValue& success = result.find_value("success");
        BOOST_CHECK(!success.isNull());
        BOOST_CHECK_EQUAL(success.get_bool(), true);
    }
+#ifdef WIN32


Why is this test being scoped to only Windows?

On other platforms, execl() might fail only if there is problem with /bin/sh. I can't see any trivial way to adjust the test to trigger a failure. Any suggestions?

ryanofsky · 2025-05-22T19:47:06Z

Looking at 4837fdc:

The change to subprocess.h seems like an improvement. Seems to make more sense to construct command line strings and pass them to the shell to parse, than construct command line strings and parse the strings we constructed ourselves.
It would probably be good if the new behavior were controlled by a shell boolean option like python & the upstream library
Windows seems to be doing joining, splitting, and the joining again, but that is unchanged in this PR.
It's not clear to me what the problem in #32574 on illumos or actually is or how this change fixes it.
I don't think the added quotes in external_signer.cpp are doing anything, or it isn't clear what they are trying to do. They at least do not seem to be related to the illumos bug so maybe could be dropped here?

On the c++11 issue, I think maybe just compiling the subprocess unit test in c++11 would be a good way to ensure compatibility. It seems cmake has a CXX_STANDARD per-target property. I would be surprised if there there were benefits to the subprocess library actually using newer c++ features.

hebasto · 2025-05-23T06:13:57Z

It's not clear to me what the problem in #32574 on illumos or actually is or how this change fixes it.

I've updated #32574 with more details.

On the c++11 issue, I think maybe just compiling the subprocess unit test in c++11 would be a good way to ensure compatibility. It seems cmake has a CXX_STANDARD per-target property. I would be surprised if there there were benefits to the subprocess library actually using newer c++ features.

Please see #32343 (comment).

hebasto · 2025-05-23T12:51:52Z

Closing in favour of #32601.

hebasto · 2025-05-23T12:55:44Z

src/external_signer.cpp

@@ -62,12 +62,12 @@ bool ExternalSigner::Enumerate(const std::string& command, std::vector<ExternalS

 UniValue ExternalSigner::DisplayAddress(const std::string& descriptor) const
 {
-    return RunCommandParseJSON(m_command + " --fingerprint " + m_fingerprint + NetworkArg() + " displayaddress --desc " + descriptor);
+    return RunCommandParseJSON(m_command + " --fingerprint \"" + m_fingerprint + "\"" + NetworkArg() + " displayaddress --desc \"" + descriptor + "\"");


#32577 (comment):

I don't think the added quotes in external_signer.cpp are doing anything, or it isn't clear what they are trying to do.

Descriptors contain the ( and ) characters, which may cause an issue with the shell:

$ echo pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798) bash: syntax error near unexpected token `('

#32577 (comment):

I don't think the added quotes in external_signer.cpp are doing anything, or it isn't clear what they are trying to do.

Descriptors contain the ( and ) characters, which may cause an issue with the shell:

$ echo pk(0279be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798) bash: syntax error near unexpected token `('

I see. I think this will probably work pretty well on unix systems and will even work on windows systems as long as descriptor strings do not contain any whitespace because subprocess quote_argument function will currently leave arguments not containing whitespace alone.

So this is probably fine if we are looking for a good-enough solution. But there still may be problems though if the descriptor strings contained any $ characters on unix (because these will still be interpreted by the shell) or whitespace characters on windows (because these would cause quote_argument to double the quotes), or if the strings contained backslashes on either platform.

Also it looks like the subprocess quote_argument function is buggy, because any argument that begins and ends with quotes and doesn't have spaces will incorrectly have its quotes stripped by the windows program that is executed. If that bug is ever fixed, we would have a bug here on windows.

This is all very messy. So despite my earlier suggestion to use the shell to avoid needing to split ourselves #32577 (comment), I think the most robust solution would actually be to avoid needing to split these command line strings at all, and it would be best to change signature of RunCommandParseJSON to take a vector<string> argument instead of a std::string to avoid the need for any splitting.

it would be best to change signature of RunCommandParseJSON to take a vector argument instead of a std::string to avoid the need for any splitting

Another advantage of this approach is that we should be able to avoid needing to change subprocess library at all, all the changes could be internal

hebasto · 2025-05-23T15:33:29Z

It would probably be good if the new behavior were controlled by a shell boolean option like python & the upstream library

Reverting 633e45b is still an option on the table. #32566 may also benefit from it.

The issues mentioned in #32577 (comment) might be addressed by switching to an alternative subprocess::Popen constructor, as done in #32566.

This reverts commit 633e45b.

hebasto · 2025-05-23T17:21:03Z

Reopened per request.

luke-jr · 2025-05-27T15:12:21Z

Probably should really go through the shell on Windows too...

DrahtBot · 2025-07-26T10:45:11Z

🐙 This pull request conflicts with the target branch and needs rebase.

laanwj reviewed May 21, 2025

View reviewed changes

src/util/subprocess.h Outdated Show resolved Hide resolved

laanwj added the Utils/log/libs label May 21, 2025

hebasto force-pushed the 250521-subprocess-split branch from 7a871fe to 621367d Compare May 21, 2025 21:27

hebasto force-pushed the 250521-subprocess-split branch from 621367d to 4837fdc Compare May 22, 2025 14:12

fanquake reviewed May 22, 2025

View reviewed changes

hebasto changed the title ~~subprocess: Handle quoted tokens properly~~ subprocess: Let shell parse command on non-Windows systems May 23, 2025

This was referenced May 23, 2025

Use subprocess library for notifications #32566

Open

test: Fix system_tests/run_command test #32601

Closed

hebasto closed this May 23, 2025

hebasto commented May 23, 2025

View reviewed changes

hebasto added 2 commits May 23, 2025 16:39

Revert "remove unneeded shell option from cpp-subprocess"

1df30b9

This reverts commit 633e45b.

Run subprocess in shell on non-Windows systems

48a0556

hebasto reopened this May 23, 2025

hebasto force-pushed the 250521-subprocess-split branch from 783db05 to 48a0556 Compare May 23, 2025 17:20

ryanofsky mentioned this pull request May 23, 2025

subprocess: sh -c 'echo err 1>&2 && false' must return 1 #32574

Open

This was referenced Jul 21, 2025

NOMERGE DEBUG WIP ignore #33028

Closed

util: Revert "common: Close non-std fds before exec in RunCommandJSON" #33063

Merged

DrahtBot added the Needs rebase label Jul 26, 2025

	* Popen({"cmd"}, output{..}, error{..}, ....)
	* Command provided as a sequence.
	* Popen("cmd arg1", output{..}, error{..}, ....)
	* Command provided in a single string.

subprocess: Let shell parse command on non-Windows systems #32577

Are you sure you want to change the base?

subprocess: Let shell parse command on non-Windows systems #32577

Uh oh!

Conversation

hebasto commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)

Uh oh!

laanwj commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hebasto commented May 21, 2025

Uh oh!

laanwj commented May 21, 2025

Uh oh!

hebasto commented May 21, 2025

Uh oh!

ryanofsky commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented May 21, 2025

Uh oh!

ryanofsky commented May 21, 2025

Uh oh!

hebasto commented May 21, 2025

Uh oh!

ryanofsky commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented May 22, 2025

Uh oh!

hebasto commented May 22, 2025

Uh oh!

hebasto commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake May 22, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto May 22, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented May 22, 2025

Uh oh!

hebasto commented May 23, 2025

Uh oh!

hebasto commented May 23, 2025

Uh oh!

hebasto May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 23, 2025

Choose a reason for hiding this comment

Uh oh!

ryanofsky May 23, 2025

Choose a reason for hiding this comment

Uh oh!

hebasto commented May 23, 2025

Uh oh!

hebasto commented May 23, 2025

Uh oh!

luke-jr commented May 27, 2025

Uh oh!

DrahtBot commented Jul 26, 2025

Uh oh!

Uh oh!

hebasto commented May 21, 2025 •

edited

Loading

DrahtBot commented May 21, 2025 •

edited

Loading

laanwj commented May 21, 2025 •

edited

Loading

ryanofsky commented May 21, 2025 •

edited

Loading

ryanofsky commented May 21, 2025 •

edited

Loading

hebasto commented May 22, 2025 •

edited

Loading

hebasto May 23, 2025 •

edited

Loading