[Toybox] bug: commands with no options silently allow all options

[Rob Landley]

On 03/17/2017 12:44 PM, enh wrote:
> On Fri, Mar 17, 2017 at 9:28 AM, Rob Landley <rob at landley.net> wrote:
>> Um, yeah. That was intentional. (Postel's law is a bug now?)
> 
> i think the side-effect of this choice is that you get bugs like...
> 
>   what i did: uptime -s
>   what i expected: 2016-02-19 18:43:57
>   what i saw: 10:33:40 up 391 days, 14:49,  5 users,  load average:
> 0.47, 0.46, 0.57
> 
> whereas if you just said
> 
>   uptime: Unknown option s
> 
> it would have been clearer what just happened.
> 
> you could argue that you'll still get a _feature request_,

Indeed. It'll not do what you want in a slightly different way. Both
ways seem obvious to me, but I may be too close to the problem.

> but silently ignoring unknown options seems like a bug to me. this one
> isn't particularly dangerous, and the output makes it fairly clear
> that something went wrong, but not all options are like that. us
> silently ignoring a --dry-run or --interactive kind of option seems
> like a scary prospect.

True, but patch already parses options, so it wouldn't. :)

Right now "lsusb" only knows how to produce one kind of output, and
nobody's asked for -s so I haven't implemented it because I'm not the
gnu/dammit project writing code in search of a user. Ignoring -s if
supplied or complaining that it doesn't know what -s means are both
failures to implement -s.

*shrug* I added a new config option, TOYBOX_PEDANTIC_ARGS, and made
uptime use it. If you want to add more users, I have not done...

$ sort -k2,2 generated/newtoys.h | egrep '0,|NULL,'
USE_ASCII(NEWTOY(ascii, 0, TOYFLAG_USR|TOYFLAG_BIN))
USE_BLKID(NEWTOY(blkid, 0, TOYFLAG_BIN))
USE_BOOTCHARTD(NEWTOY(bootchartd, 0,
TOYFLAG_STAYROOT|TOYFLAG_USR|TOYFLAG_BIN))
USE_DOS2UNIX(NEWTOY(dos2unix, 0, TOYFLAG_BIN))
USE_FACTOR(NEWTOY(factor, 0, TOYFLAG_USR|TOYFLAG_BIN))
USE_GETCONF(NEWTOY(getconf, 0, TOYFLAG_USR|TOYFLAG_BIN))
USE_HELLO(NEWTOY(hello, 0, TOYFLAG_USR|TOYFLAG_BIN))
USE_MORE(NEWTOY(more, 0, TOYFLAG_USR|TOYFLAG_BIN))
USE_RESET(NEWTOY(reset, 0, TOYFLAG_USR|TOYFLAG_BIN))
USE_TEST_SCANKEY(NEWTOY(test_scankey, 0, TOYFLAG_BIN))
USE_UNIX2DOS(NEWTOY(unix2dos, 0, TOYFLAG_BIN))
USE_ZCAT(NEWTOY(zcat, 0, TOYFLAG_USR|TOYFLAG_BIN))
USE_BZCAT(NEWTOY(bzcat, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_CHATTR(NEWTOY(chattr, NULL, TOYFLAG_BIN))
USE_CLEAR(NEWTOY(clear, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_COUNT(NEWTOY(count, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_DD(NEWTOY(dd, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_EXPR(NEWTOY(expr, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_FALSE(NEWTOY(false, NULL, TOYFLAG_BIN|TOYFLAG_NOHELP))
USE_GROUPS(NEWTOY(groups, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_IP(NEWTOY(ip, NULL, TOYFLAG_SBIN))
USE_LSMOD(NEWTOY(lsmod, NULL, TOYFLAG_SBIN))
USE_LSUSB(NEWTOY(lsusb, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_READAHEAD(NEWTOY(readahead, NULL, TOYFLAG_BIN))
USE_REV(NEWTOY(rev, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_SH(NEWTOY(cd, NULL, TOYFLAG_NOFORK))
USE_SH(NEWTOY(exit, NULL, TOYFLAG_NOFORK))
USE_SYNC(NEWTOY(sync, NULL, TOYFLAG_BIN))
USE_TAC(NEWTOY(tac, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_TEST(NEWTOY(test, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_TOYBOX(NEWTOY(toybox, NULL, TOYFLAG_STAYROOT))
USE_TRUE(NEWTOY(true, NULL, TOYFLAG_BIN|TOYFLAG_NOHELP))
USE_W(NEWTOY(w, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_XZCAT(NEWTOY(xzcat, NULL, TOYFLAG_USR|TOYFLAG_BIN))
USE_YES(NEWTOY(yes, NULL, TOYFLAG_USR|TOYFLAG_BIN))

Except several of those _do_ use arguments, they just parse their own
arguments and skip the dash handling step because all it would do is
screw stuff up. ("bzcat" iterates through its arguments, and will
happily decompress "-9" as a filename. "yes" will happily repeat "yes
--fruitbasket". Would washing those through gratuitous generic option
parsing just so they can _fail_ on things starting with a dash actually
improve matters?)

Anyway, new infrastructure to do what you asked for.

> postel's law assumes it's safe to just skip the bits you don't
> understand. i'd say the unix command line is a very risky place to do
> that.

For "uptime"? "sync"? "clear"/"reset"? (_Not_ skipping it for true/false
--help was already report as a bug and fixed. :)

And skipping option parsing doesn't mean we're ignoring options. In
toysh "exit" looks at its first argument. I have a todo item to improve
the error handling there, but what exactly does error handling _mean_ in
this context?

  $ /bin/bash
  $ exit fruit
  exit
  bash: exit: fruit: numeric argument required
  $ echo $?
  2

Note: it _did_ still exit. And it decided that "2" was the code to exit
with. Possibly this is in posix somewhere, I need to go read that.

However, the failure-to-police here (other than atoi() returning 0 for
non-numeric and it then exiting 0, which I already have on my todo list
to fix) is:

  $ exit 1 fruitbasket
  exit
  bash: exit: too many arguments
  $ echo $?
  1

So it writes out an error string (only in interacive mode maybe? It
doesn't say "exit" for non-interactive uses of the exit command...) but
then exits with the argument value you told it, so...

*shrug*

>> I also added a flag to skip the --help and --version parsing for "false"
>> and "true" because paying _any_ attention to their arguments was
>> considered a bug by somebody (I forget who, there was a complaint).
> 
> yeah, some commands are just fucked by "design". but that seems like
> something to opt in to, rather than a good default.

There isn't a "default". Feeding NULL in to the option string is a
choice just like any other value you can feed in is a choice.

>> It's easy to change the behavior, but... could you clarify what you want?
>>
>> If you change the NULL to ">0", it goes:
>>
>>   $ ./uptime -z
>>   usage: uptime
>>
>>   Tell how long the system has been running and the system load
>>   averages for the past 1, 5 and 15 minutes.
>>
>>   uptime: Unknown option z
> 
> makes sense to me.
> 
> personally i wouldn't output the help without --help, even though it
> matters more for stuff like ps. i recently cured adb of this annoying
> habit because it tends to obscure the actual error. whether or not
> it's helpful depends on whether you have no idea what you're doing or
> simply mistyped. personally i'd rather train people that "--help
> always works"), but with or without the full help text, this still
> feels like an improvement to me.

--help always working is a much newer thing than unknown options
producing help output. :)

But I can change it if you feel strongly about it.  Pretty much just a
global search and replace of help_exit with error_exit in lib/args.c.

>>   $ ./uptime
>>    10:53:24 up 122 days, 22:01,  269 users,  load average: 1.67, 1.71,
>>
>> I see that the gnu/dammit stuff has grown checking for this over the
>> past decade (along with systemd). Circa Red Hat 9 a lot of stuff with no
>> options ignored them. Now everything has lots and lots of options even
>> when it does nothing. (Good grief, "help cd" produces 33 lines of
>> output, and that's despite popd/pushd being separate commands.)
> 
> i _reduced_ the verbosity of adb's --help output... to 143 lines :-/

Woo!

Writing concise but informative help text is #*(%&# hard. For things
like "sed --help" I want the help to be standalone: you shouldn't need
to look at the posix spec or somebody else's man page to learn how to
use the command. But it's _tough_. I need to do another pass over "ps",
I don't think it includes all the -o options yet...

>> If you're going to police this you may also want to do stuff like "nproc
>> one two three four five" which happily ignores the extraneous garbage
>> and produces its output. I'm guessing it should _not_ work?
> 
> to my mind, no, that should be rejected. because i don't want to
> assume i know that the user meant nothing by it. to me, it's more
> likely they fucked up and we just did something unintended for them.
> (again, uptime and nproc aren't the scariest commands, but if i say
> "nproc --ignore=24" it's not helpful for toybox to ignore --ignore.)

I) It doesn't, it's already option parsing for --all so will reject
--walrus, it's just not doing ">0" in its option string.

2) I'm sort of amazed a command line option to do "X=$(((nproc)-24); [
$X -lt 1 ] && X=1" exists, but sure. (Does df have a similar --ignore
option to make filesystems look smaller?)

C) This isn't the same fix as the new infrastructure (or in the list of
commands above). Auditing all the commands to make them care when the
user appends nonsense to the command line isn't on my todo list,  but
it's a trivial patch if somebody cares enough to work out what goes where.

四) I could continue counting, yes.

Rob