The system shell /bin/sh is consuming options after the command string when -c is used. According to the synopsis, nothing after the -c command_string should be interpreted as an option. Furthermore, passing -s as the program_name argument causes the shell to read from standard input, which is at odds with the description of the -c option in the manual and in the POSIX standard. The behavior on FreeBSD is inconsistent with the behavior on other operating systems. A NetBSD and an Ubuntu Linux system were checked, and both have the expected behavior. Observed behavior: ``` $ sh -c 'echo $0 $@' -m foo foo $ sh -c 'echo $0 $@' -s foo $ <child shell, reading commands from standard input> ``` Expected behavior: ``` $ sh -c 'echo $0 $@' -m foo -m foo $ sh -c 'echo $0 $@' -s foo -s foo ```
I am trying to absorb the source code for this right now. In bin/sh/options.c there is a comment on line 116 noting the POSIX standard and the behavior I was expecting. The problem seems to be in the options() function around the NOHACK macros. I will submit a patch here as an attachment when I have worked out a fix.
Created attachment 184215 [details] Patch sh to end option processing after -c This seems to work, but I'm not sure how to run the test suite. I would like to learn how to add a regression test for this too.
You are right that FreeBSD sh deviates here, but I do not agree with your patch. POSIX specifies that -c's command_string is an operand and not an option-argument (but -o does take an option-argument). Therefore in sh -ca 'echo $-:$0' and sh -c -a 'echo $-:$0' the allexport option should be set, while in sh -c 'echo $-:$0' -a the allexport option should not be set and $0 should be set to "-a". Your patch fixes the latter case at the cost of breaking the former case, while the middle case remains broken. Various shells such as bash, ksh93 and mksh handle all three cases correctly. Note that this means that passing a command string starting with "-" to popen() or system() shall not execute that string as a command, since sh will interpret it as containing options and there will not be a command_string for -c. A command string starting with "-" can be passed like sh -c -- '-a; echo continued' which bash, ksh93 and mksh also support and we do not. The specification for popen() and system() is unambiguous about the command that should be used, though, and it does not include "--". Please forget about the NOHACK code. Apart from the fact that this kind of thing makes a poor compile-time option, the NOHACK case has been obviously broken since it was added, because it uses q uninitialized if (*p != '\0'). I will take it out soon. The easiest way to run the test suite is to install everything and then run as root cd /usr/tests/bin/sh && kyua test If these directories do not exist, place WITH_TESTS=YES in /etc/src.conf, rebuild and reinstall. An uninstalled sh can be tested against installed tests by running cd /usr/tests/bin/sh && kyua -v test_suites.FreeBSD.bin.sh.test_shell=/path/to/test/sh test This latter command might break in the future since this feature should be provided generically, not specifically to sh.
Created attachment 184754 [details] Fix option processing for sh -c and add tests
I think this is the right direction but I do have some things that need to be changed. So that installworld does not fail, the new directory should be added to the appropriate file in etc/mtree/ (BSD.tests.dist in this case). Perhaps it should be named "invocation" instead of "options", since tests for a few set options are under "execution" already. Also, I would like this split up in two patches, one that adds some testcases that already work (and will not be broken by the change) and one that fixes the bug and adds testcases that fail with the previous version. The variable saw_minus_c would be more consistently named cflag. I suppose it is OK to start using <stdbool.h> at this point. A -c without a command_string operand (as in "sh -c" or "sh -c -s") should not be ignored, since this causes commands to be read from unexpected sources. (This is one situation for a testcase that already passes.) The current code also rejects -cc, even if a command_string operand is given. Accepting this, as your patch does, seems consistent with other shells. The -f option is better suited than the -m option as a "dummy" option since -m may cause the shell to stop itself in certain situations. The -a option is also suitable. I like to be able to run testcases with various other shells, where possible. To help with this, avoid echoing things starting with '-' or containing '\' and do not assume that $- contains only the expected characters in the order sh currently happens to put them. For example, ${SH} -f -c -a 'echo $-:$0:$@' -foo -bar | grep -qx "fa:-foo:-bar" could be fixed as case `${SH} -f -c -a 'echo $-:$-:$0:$@' -foo -bar` in *f*:*a*:-foo:-bar) : ;; *) echo bad ;; esac (I have not used $() here since shells/heirloom-sh may pass a few related tests, even though it will not pass this one.)
Created attachment 185108 [details] Pre-fix tests for sh -c I will submit a patch for the fix and additional tests as a separate attachment.
Created attachment 185110 [details] Fix option processing for sh -c and add tests If I missed anything, let me know.
A commit references this bug: Author: jilles Date: Sat Aug 12 19:17:49 UTC 2017 New revision: 322438 URL: https://svnweb.freebsd.org/changeset/base/322438 Log: sh: Add tests for sh -c that already pass. PR: 220587 Submitted by: Ryan Moeller Changes: head/bin/sh/tests/Makefile head/bin/sh/tests/invocation/ head/bin/sh/tests/invocation/Makefile head/bin/sh/tests/invocation/sh-ac1.0 head/bin/sh/tests/invocation/sh-c1.0 head/bin/sh/tests/invocation/sh-ca1.0 head/bin/sh/tests/invocation/sh-fca1.0 head/etc/mtree/BSD.tests.dist
(In reply to Ryan Moeller from comment #7) I have committed your pre-fix tests. However, the options fix has various cases where it ignores -c, which may lead to unexpected results (e.g. system("--") will read commands from standard input). Whenever -c is given, commands must be read from the command_string operand; if there is none, the invocation is invalid and must fail. I have added a test "invocation/sh-c-missing1.0" for the simplest "sh -c" case; however, there are also "sh -c --", "sh -c -", "sh -c -- 'echo hi'" and "sh -c - 'echo hi'".
Created attachment 185396 [details] Additional pre-fix invocation tests
Created attachment 185397 [details] Fix option processing for sh -c and add tests
Sorry for the delay. I think your patch is good, but I would like to know about existing wrong uses of sh -c in src and ports that this patch will cause to break.
This change breaks various ports. Some problems are in Mk/bsd.port.mk (COPYTREE_BIN and COPYTREE_SHARE) affecting many ports and in lang/ruby*. These are all of the form ${SH} -c 'command' -- arg0 arg1 ... where current sh will expand $0 to arg0 and $1 to arg1, while POSIX says to and this patch makes sh expand $0 to --, $1 to arg0 and $2 to arg1. The fix is to remove the -- and to, if arg0 may start with '-', add a dummy arg0 and adjust the command string accordingly. These fixes need to be committed to ports before the sh patch can be committed.
Changing this behavior is going to break so many scripts, this is a very big POLA.
batch change: For bugs that match the following - Status Is In progress AND - Untouched since 2018-01-01. AND - Affects Base System OR Documentation DO: Reset to open status. Note: I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
This bug is still valid and I would like to fix it, but getting ports fixed takes very long. Due to the breaking change, this fix will not be MFC'ed.
FYI, in the next version, the POSIX standard will require system(3) to invoke sh in the form "sh -c -- COMMAND" for safety. Please see: https://austingroupbugs.net/view.php?id=1440
*** Bug 264319 has been marked as a duplicate of this bug. ***
(In reply to Martijn Dekker from comment #17) POSIX requires system(command); to behave *as if* sh -c -- command was called, for safety, because POSIX requires sh to handle options after -c. I think the better design would be to only allow -c as last argument to sh, as the SYNOPSIS in our man page says too, that would eliminate all bugs where command could potentially start with -, without requiring everyone to write -c -- or have a bug in waiting.
(as an outsider comment,) For the MUA i maintain INSTALL now says - All systems: * "$SHELL -c -- 'echo du'" is required to print "du". POSIX issue #1440, FreeBSD /bin/sh fails (#264319, #220587). The test suite works around, but consider VAL_SHELL=/better/one. and the test of my MUA now has to redirect the entire machinery ($MAILX is ourselves) via (works fine in practice). The port (mail/s-nail) does not set VAL_SHELL when building. Well, it does not seem to hurt too many people, or they set the $SHELL variable as such... Here the test workaround: # "sh -c -- 'echo yes'" must echo "yes"; FreeBSD #264319, #220587: work around if [ $("${VAL_SHELL}" -c -- 'echo yes' 2>/dev/null) = yes ]; then T_MAILX= T_SH= else echo '! '"${VAL_SHELL}"' cannot deal with "-c -- ARG", using workaround' T_MAILX=./t.mailx.sh T_SH=./t.sh.sh ${rm} -f ${T_MAILX} ${T_SH} ${cat} > ${T_MAILX} <<_EOT #!${VAL_SHELL} SHELL=${TMPDIR}/${T_SH} export SHELL exec ${MAILX} "\$@" _EOT ${cat} > ${T_SH} <<_EOT #!${VAL_SHELL} shift 2 exec ${VAL_SHELL} -c "\${@}" _EOT ${chmod} 0755 ${T_MAILX} ${T_SH} MAILX=${TMPDIR}/${T_MAILX} fi
(Ie the source code now does, and somehow that has to be done, without too much configuration hassle, right? I wonder how many such pitfalls really exist in the tens of thousands of packages of the FreeBSD port system.) void mx_child_ctx_set_args_for_sh(struct mx_child_ctx *ccp, char const *sh_or_nil, char const *cmd_string){ NYD2_IN; if(sh_or_nil == NIL) sh_or_nil = ok_vlook(SHELL); ccp->cc_cmd = sh_or_nil; ccp->cc_args[0] = "-c"; ccp->cc_args[1] = "--"; ccp->cc_args[2] = cmd_string; NYD2_OU; }
Quite apart from ports, this has the potential to break any application software that uses system(3) or popen(3) so it has huge POLA potential.