Date: Sun, 10 Dec 2017 16:03:34 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Niclas Zeising <zeising@freebsd.org> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326733 - head/usr.sbin/acpi/acpiconf Message-ID: <20171210132430.M1124@besplex.bde.org> In-Reply-To: <201712091559.vB9FxAv9001255@repo.freebsd.org> References: <201712091559.vB9FxAv9001255@repo.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 9 Dec 2017, Niclas Zeising wrote:
> Log:
> Improve options and error handling.
>
> Improve options handling and error out if multiple mutually exclusive
> options are passed to acpiconf. Switch from using atoi() to strtol() for
> argument parsing, and add error checking and handling, instead of blindly
> trusting that the integer conversion is OK.
> Cange err() to errx() in once case, the errno value was garbage there.
This has the usual bugs in strtol() handling, making it little better than
atoi(). It adds manu new bugs.
> Modified: head/usr.sbin/acpi/acpiconf/acpiconf.c
> ==============================================================================
> --- head/usr.sbin/acpi/acpiconf/acpiconf.c Sat Dec 9 15:47:26 2017 (r326732)
> +++ head/usr.sbin/acpi/acpiconf/acpiconf.c Sat Dec 9 15:59:10 2017 (r326733)
> @@ -205,8 +205,9 @@ usage(const char* prog)
> int
> main(int argc, char *argv[])
> {
> - char *prog;
> - int c, sleep_type;
> + char *prog, *end;
> + int c, sleep_type, battery, ack;
> + int iflag = 0, kflag = 0, sflag = 0;
>
> prog = argv[0];
> if (argc < 2)
> @@ -218,16 +219,24 @@ main(int argc, char *argv[])
> while ((c = getopt(argc, argv, "hi:k:s:")) != -1) {
> switch (c) {
> case 'i':
> - acpi_battinfo(atoi(optarg));
> + iflag = 1;
> + battery = strtol(optarg, &end, 10);
First, errno is not set before starting, making it impossible to detect
overflow properly.
Second, the value is blindly assigned to a variable of type int, just like
for atoi(). This gives implementation-defined behaviour. For atoi(), the
behaviour is is undefined on any overflow, but for (int)strtol() the
behaviour on other overflows is defined for strtol() and the behaviour on
this overflow is only implementation-defined, so the combined behaviour is
not undefined. Good luck finding an implementation that documents its
behaviour.
Third, forcing base 10 preserves the bug that only decimal values are
supported. This is good enough for acpiconf, but still a gratuitous
restriction. POSIX might have ths restriction for all integer args on
the command line. It is also unclear if POSIX accepts args not representable
by the int type. Any such restrictions are bugs in POSIX. Portability
requires not using anything except small decimal integers for args.
Good luck finding a utility that documents the form of integer args
that it accepts. For acpiconf -s, it was clear that the arg must be
one of the characters [1-4], but this commit breaks that (see below).
For acpi -i battery, no form is documented, so it is unclear if the
arg should be a number, name, or either. -i foobar used to work to
give battery number 0 by ignoring errors in atoi(). Almost any
error handling for strtol() tends to break this.
> + if ((size_t)(end - optarg) != strlen(optarg))
> + errx(EX_USAGE, "invalid battery");
> break;
Here is the "almost any" error handling for strtol(). It is very
incomplete, but much larger than needed or usual. All it does is check
that there is no garbage after the end of the parsed part of the string.
This is normally written as:
if (*end != '\0')
but is written as:
if ((size_t)(end - optarg) != strlen(optarg))
Both see "foobar" as garbage after the end, so as an error.
The following error checks are still missing:
- null args. Best written as another test of 'end' in the same expression:
if (end == optarg || *end != '\0')
errx(... /* better error message than above */)
POSIX requires errno to be set to EINVAL if end == optarg and for some other
errors. This is unportable and should not be used. But sloppy code uses
it to combine some tests into a single tests of errno and then print a
non-specific error message. acpiconf already has the non-specific error
message.
- overflow in strtol(). Best written as:
long bnum; /* must be long to hold result */
...
errno = 0;
bnum = strtol(optarg, &end, 0);
if (errno == ERANGE)
errx(... /* better error message than above */)
Another usual error is checking if the result is LONG_MIN or LONG_MAX.
These values are returned on overflow errors but also for no errors.
errno must be used as above to distinguish, but then checking these values
just breaks support for this values. However, if these values are out of
the range of subsequent range checks and a specific error message for
overflow is not done, then the errno check and checks for the these values
are redundant. Sloppy code gets minor simplifications from this with the
minor sloppiness of non-speficic error messages.
- overflow in assignment. Best avoided by assigning to a variable of the
correct type as above.
- range checking not related to overflow. This is actually not missing, but
done in acpi_battinfo(). The range is 0 to 63 inclusive, modulo overflow
of the value before it reaches this function. E.g., 2**32 + 1 is truncated
to 1 on 64-bit arches. This range is of course not documented in the man
page.
The above also has some style bugs. Quoting it again:
> + if ((size_t)(end - optarg) != strlen(optarg))
> + errx(EX_USAGE, "invalid battery");
Some style bugs are:
- verbose spelling of (*end != '\0) as discussed above
- obfuscated name 'end'. 'endp' or 'endptr' is better. Both C99 and the
FreeBSD man page use 'endptr'
- 4-char indentation of errx()
- use of sysexits(3). Bug for bug compatible with the rest of the file
and with the error exit in acpi_battinfo()
- non-specific error message. The rest of the file is mostly better. It
mostly prints the invalid value in error messages. A similar message
in acpi_battinfo() prints the arg. Most error messages don't print
the range of valid values. The similar message has this bug (it
doesn't print [0-63]), and it also has the bug of containing a bogus
system message from using err() instead of errx(). errno is garbage
from before atoi() or strtol() at that point, but sometimes strtol()
sets errno to ERANGE or EINVAL and then the system message is
accidentally correct.
- other poor wording in error message. It is not the battery that is
invalid, but the battery number given by the battery arg. The similar
message in acpi_battinfo() has the same bug. Even the man page is
better. It names the are 'batt'. That is a bit cryptic for the man
page, and better for the source code, but the source code expands the
name of the battery number arg from 'batt' to 'battery'.
> case 'k':
> - acpi_sleep_ack(atoi(optarg));
> + kflag = 1;
> + ack = strtol(optarg, &end, 10);
> + if ((size_t)(end - optarg) != strlen(optarg))
> + errx(EX_USAGE, "invalid ack argument");
> break;
Similarly, except it prints "argument", and the valid range is even more
obscure. The arg is bindly blindly truncated and then blindly passed to
the kernel which presumably checks. The later error message tells us that
the arg is a sleep type. If it is a sleep type, then it can be checked
here to get more specific messages as for -s. The later error message
for this doesn't print the arg. The later error message for -s does
print the arg.
> case 's':
> + sflag = 1;
> if (optarg[0] == 'S')
> - sleep_type = optarg[1] - '0';
> - else
> - sleep_type = optarg[0] - '0';
> + optarg++;
> + sleep_type = strtol(optarg, &end, 10);
This is incompatible. Leading whitespace and signs are now accepted. If
breaking compatibility, then also allow any base (actually only octal,
hex and decimal since C literals are too feeble to even support binary.
The main incompatiblity is the octal prefix of 0 not being different
enough).
> + if ((size_t)(end - optarg) != strlen(optarg))
> + errx(EX_USAGE, "invalid sleep type");
Similarly.
> if (sleep_type < 1 || sleep_type > 4)
> errx(EX_USAGE, "invalid sleep type (%d)",
> sleep_type);
The old error message is missing most of the style bugs in the new one.
There are rather too many error messages to print specific errors, without
even specific ones for range errors. 2 here, with the first one only
being distinguishable from the second one due to bugs in it. Both basically
just say that the arg is invalid. Later the arg is passed to the kernel.
Since the range is checked here, it must be valid, but might be unsupported.
The later message just says "... failed" (then the kernel errno).
> @@ -241,7 +250,25 @@ main(int argc, char *argv[])
> argc -= optind;
> argv += optind;
The last 2 lines still seem to be garbage.
>
> - if (sleep_type != -1)
> + if (iflag != 0 && kflag != 0 && sflag != 0)
> + errx(EX_USAGE, "-i, -k and -s are mutually exclusive");
Stranger indentation here.
> +
Style bug (extra blank line). KNF doesn't use blank lines even to separate
unrelated code, but the code here is related.
> + if (iflag != 0) {
Style bug (extra space).
> + if (kflag != 0)
> + errx(EX_USAGE, "-i and -k are mutually exclusive");
> + if (sflag != 0)
> + errx(EX_USAGE, "-i and -s are mutually exclusive");
> + acpi_battinfo(battery);
> + }
> +
> + if (kflag != 0) {
> + if (sflag != 0)
> + errx(EX_USAGE, "-k and -s are mutually exclusive");
> + acpi_sleep_ack(ack);
> + }
> +
> +
Now 2 extra blank lines.
> + if (sflag != 0)
> acpi_sleep(sleep_type);
>
> close(acpifd);
Everything near the end seems to be wrong. All options except -s, -h and
unknown options were processed in order. Some repeated options were
repeated. Some combinations and some repetitions might not work, but the
verbose checking doesn't do much except prevent users tryning them.
It is certainly valid and useful to do any number of -i's for different
batteries. That is broken now.
Several i's for the same battery is valid but not so useful. Similarly
several -i's followed by an option that prevents doing more.
The large code to disallow combinations is not large enough to disallow
valid and invalid combinations with -h, since -h is still processed in
order (similarly for unknown options treated like -h). E.g., -i 0 -h
used to print battery info then help. Now it prints help without
detecting that this breaks -b. -s 0 -h and -h -s 0 are not so valid.
They are not considered to be invalid, but are silently converted to
-h the same as before. -s 666 -h is detected as an error by seeing
and checking the -s arg before -h; -h -s 666 is not detected as an
error because -h is processed in order and the processing terminates
the program.
The error handling is to exit on any error, so parsing and checking
most of the options before handling 1 is especially non-useful for
this program. E.g, -i 0 -i 1 ... is no good for probing for batteries
since it exited on the first unconfigured battery. Now it exits after
handling the first battery and doesn't even do full checking for the
other batteries, since the checking is not all in the getopt() loop
now that acpi_battinfo() is not in the loop.
Documentation for the unsupported combinations is missing in both the
usage method and the man page. This can be hard to write for even
a few combinations when the unsupported combinations are non-orthogonal
or make no sense. This is usualy handled with multiple command lines
for the random logic and complicated notation in single command lines
for non-random logic.
To disallow combinations without undocumented complications, just process
options in order and exit after handling 1. acpiconf doesn't have any
coomands that need multiple options. I don't really like this. It is
just easy to document using 1 command line with multiple exclusive options.
Hmm, this can be checked nicely: after finding 1 option, exit with an
error without doing anything if there are any more options. Don't bother
parsing the garbage options after the first one. Don't print 2**N error
messages for 2**N combinations of N invalid options afger the first one.
The current documentation is quite broken even for the the old behaviour --
it doesn't mention multiple options.
Even ls(1) is not clear about what happens for multiple options.
Positive-logic flag options can be repeated with no effect, but the
syntax in the synopsis doesn't say this. Like many utilities, ls
takes a --libxo option whose syntax is wrong in the SYNOPOSIS,
missing in the usage message, and only documented by pointing to
libxo(3) in the DESCRIPTION. ls takes only 1 option with an arg
(-D). Repetition of this is undocumented. It silently overrides
the previous -D.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171210132430.M1124>