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

hebasto commented at 8:07 pm on May 20, 2025: member

On OpenIndiana:

0./test/system_tests.cpp(54): error: in "system_tests/run_command": check what.find(tfm::format("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos has failed
1./test/system_tests.cpp(55): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed

Same on OmniOS:

0./test/system_tests.cpp(54): error: in "system_tests/run_command": check what.find(tfm::format("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos has failed
1./test/system_tests.cpp(55): error: in "system_tests/run_command": check what.find(expected) != std::string::npos has failed

hebasto added the label Tests on May 20, 2025

hebasto commented at 5:58 am on May 23, 2025: member

The underlying issue is not specific to illumos.

With the following patch:

 0--- a/src/test/system_tests.cpp
 1+++ b/src/test/system_tests.cpp
 2@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(run_command)
 3         const std::string expected{"err"};
 4         BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
 5             const std::string what(e.what());
 6-            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos);
 7+            BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned 1", command)) != std::string::npos);
 8             BOOST_CHECK(what.find(expected) != std::string::npos);
 9             return true;
10         });

it becomes evident that the command sh -c 'echo err 1>&2 && false' does not exit with status 1.

hebasto renamed this:
~~`system_tests/run_command` test fails on illumos OS~~
subprocess: `sh -c 'echo err 1>&2 && false` must return `1`
on May 23, 2025

hebasto renamed this:
~~subprocess: `sh -c 'echo err 1>&2 && false` must return `1`~~
subprocess: `sh -c 'echo err 1>&2 && false'` must return `1`
on May 23, 2025

ryanofsky commented at 7:23 pm on May 23, 2025: contributor

The underlying issue is not specific to illumos.

I couldn’t find an explanation of the underlying issue, but looking at this more, I think I maybe understand the problem. The test code that is failing looks like:

0    49          // Return non-zero exit code, with error message for stderr
1    50          const std::string command{"sh -c 'echo err 1>&2 && false'"};
2    51          const std::string expected{"err"};
3    52          BOOST_CHECK_EXCEPTION(RunCommandParseJSON(command), std::runtime_error, [&](const std::runtime_error& e) {
4    53              const std::string what(e.what());
5    54              BOOST_CHECK(what.find(strprintf("RunCommandParseJSON error: process(%s) returned", command)) != std::string::npos);
6    55              BOOST_CHECK(what.find(expected) != std::string::npos);
7    56              return true;
8    57          });

And the specific checks that are failing are on line 54 and 55 as reported initially.

What the test is trying to do here is execute the command sh -c 'echo err 1>&2 && false on the current system.

Then line 54 is checking trying to look for the RunCommandParseJSON error thrown when the command returns a nonzero exit code. Apparently this check is failing because on illumos the command returns a 0 exit code?

And line 55 is checking for the string “err” somewhere in the same error message. But that check also fails for the same reason.

The fact that these BOOST_CHECK statements are failing inside the BOOST_CHECK_EXCEPTION check for a std::runtime_error exception probably indicates that some std::runtime_error exception is being thrown, so it is probably just the one thrown when the command returns invalid json

The test seems pretty broken currently, so not surprising that it returns inconsistent results on different operating systems.

Because we are not enabling the subprocess module’s shell option, the subprocess module is attempting to parse the command line sh -c 'echo err 1>&2 && false' itself. And it looks like it does that pretty poorly splitting the command up into an argv array consisting of ["sh", "-c", "'echo", "err", "1>&2", "&&", "false'"] arguments. On linux, because 'echo is not a valid shell script, the bash shell returns exit code 2 and error message err: -c: line 1: unexpected EOF while looking for matching `''. The reason why the bash message begins with “err” is that bash interprets the trailing arguments “’echo”, “err”, “1>&2”, … as $0, $1, $2, …. arguments to the bash script, so it treating “err” as $0 as the name of the script it thinks it is executing and using that script name in its error message.

So that explains why the test is passing on linux with bash, even if its not checking for a particularly meaningful condition.

It is unclear what exactly is causing the behavior on illumos, but my guess would be that illumos shell might be just treating sh -c "'echo" "err" "1>&2" "&&" "false'" as an echo command and returning success and not bothering to complain that the 'echo quote is unterminated.

In any case, I couldn’t think of any way to fix this test to use RunCommandParseJSON to run a command printing to stderr on linux, because the split function splits on spaces and tabs and it’s hard to write a shell script that doesn’t include spaces or tabs.

We could change RunCommandParseJSON to actually execute full shell commands as was done in 4837fdc1569d58541fb310251b969ae160907e6e from #32577. But ultimately the best approach for our use-case might be to avoid even more joining and splitting and unnecessary use of shell and just make RunCommandParseJSON take an vector<string> array that it can pass to the subprocess library, instead of passing any string that needs to be parsed

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

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