Bug 220587 - /bin/sh Incorrect options handling
Summary: /bin/sh Incorrect options handling
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 11.0-RELEASE
Hardware: Any Any
: --- Affects Many People
Assignee: Jilles Tjoelker
URL:
Keywords:
Depends on: 222872 222870
Blocks:
  Show dependency treegraph
 
Reported: 2017-07-10 00:08 UTC by Ryan Moeller
Modified: 2017-10-11 12:24 UTC (History)
4 users (show)

See Also:


Attachments
Patch sh to end option processing after -c (301 bytes, patch)
2017-07-10 00:56 UTC, Ryan Moeller
no flags Details | Diff
Fix option processing for sh -c and add tests (3.50 KB, patch)
2017-07-27 04:27 UTC, Ryan Moeller
no flags Details | Diff
Pre-fix tests for sh -c (2.53 KB, patch)
2017-08-07 03:00 UTC, Ryan Moeller
no flags Details | Diff
Fix option processing for sh -c and add tests (3.52 KB, patch)
2017-08-07 07:20 UTC, Ryan Moeller
no flags Details | Diff
Additional pre-fix invocation tests (1.04 KB, patch)
2017-08-14 06:55 UTC, Ryan Moeller
no flags Details | Diff
Fix option processing for sh -c and add tests (5.18 KB, patch)
2017-08-14 07:00 UTC, Ryan Moeller
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Moeller 2017-07-10 00:08:39 UTC
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
```
Comment 1 Ryan Moeller 2017-07-10 00:30:21 UTC
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.
Comment 2 Ryan Moeller 2017-07-10 00:56:19 UTC
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.
Comment 3 Jilles Tjoelker freebsd_committer 2017-07-17 22:09:55 UTC
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.
Comment 4 Ryan Moeller 2017-07-27 04:27:08 UTC
Created attachment 184754 [details]
Fix option processing for sh -c and add tests
Comment 5 Jilles Tjoelker freebsd_committer 2017-07-30 22:08:56 UTC
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.)
Comment 6 Ryan Moeller 2017-08-07 03:00:55 UTC
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.
Comment 7 Ryan Moeller 2017-08-07 07:20:26 UTC
Created attachment 185110 [details]
Fix option processing for sh -c and add tests

If I missed anything, let me know.
Comment 8 commit-hook freebsd_committer 2017-08-12 19:18:23 UTC
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
Comment 9 Jilles Tjoelker freebsd_committer 2017-08-13 14:49:40 UTC
(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'".
Comment 10 Ryan Moeller 2017-08-14 06:55:20 UTC
Created attachment 185396 [details]
Additional pre-fix invocation tests
Comment 11 Ryan Moeller 2017-08-14 07:00:57 UTC
Created attachment 185397 [details]
Fix option processing for sh -c and add tests
Comment 12 Jilles Tjoelker freebsd_committer 2017-09-19 21:56:00 UTC
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.
Comment 13 Jilles Tjoelker freebsd_committer 2017-10-08 15:43:59 UTC
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.
Comment 14 Mathieu Arnold freebsd_committer 2017-10-11 12:24:02 UTC
Changing this behavior is going to break so many scripts, this is a very big POLA.