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