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

[enh]

On Tue, Mar 21, 2017 at 2:35 PM, Rob Landley <rob at landley.net> wrote:
> 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.

i guess the useful example i've seen was that we eased the transition
between toolbox and toybox ps because folks can try "ps -A || ps", but
as the TL;DR version of your response would probably go "there aren't
many useful commands that don't actually have any arguments". there
are certainly a lot more that have weird ad-hoc argument parsing!

>> 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.

looking at the list, i think you're probably right that uptime was in
a very small set of special cases.

>> 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...

very occasionally i consider writing an apropos. but it feels weird
somehow. even though i do sometimes find myself doing "grep -r" in the
source. it seems like busybox never had one, so it's probably not
useful enough?

>>> 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?)

i think roughly 0 people know how to do arithmetic using just the
shell... and most of them have been involved in the development of at
least one shell.

> 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
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.