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

[enh]

On Fri, Mar 17, 2017 at 9:28 AM, Rob Landley <rob at landley.net> wrote:
> On 03/16/2017 12:08 PM, enh wrote:
>> commands with options are fine:
>>
>> $ ./toybox df -z
>> ...
>> df: Unknown option z
>>
>> but any command with no options silently ignores any options:
>>
>> $ ./toybox uptime -z
>>  10:07:34 up 34 days, 30 min,  6 users,  load average: 4.44, 2.28, 1.46
>> $ ./toybox uptime -z g
>>  10:08:26 up 34 days, 31 min,  6 users,  load average: 3.65, 2.46, 1.57
>> $ ./toybox uptime g
>>  10:08:28 up 34 days, 31 min,  6 users,  load average: 3.65, 2.46, 1.57
>
> 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_, 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.

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.

> When you NULL out the optstr of the NEWTOY() macro the option parsing
> infrastructure doesn't get called (and can actually drop out at compile
> time if nothing uses it, see the NEED_OPTIONS in main.c). Instead
> main(argv) gets put straight into toys.argv[] and if the command wants
> to parse stuff itself it can.
>
> You can get a similar effect with the "?" prefix to optstr (as find.c
> and kill.c do so "find -depth" and "kill -stop" aren't intercepted as
> errors).
>
> 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.

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

>   $ ./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 :-/

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

> Rob



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