From owner-svn-src-all@freebsd.org Sun Dec 10 05:03:43 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 04F7CE823DA; Sun, 10 Dec 2017 05:03:43 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id 7EF9363A1D; Sun, 10 Dec 2017 05:03:42 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id D39721084EF; Sun, 10 Dec 2017 16:03:34 +1100 (AEDT) Date: Sun, 10 Dec 2017 16:03:34 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Niclas Zeising 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 In-Reply-To: <201712091559.vB9FxAv9001255@repo.freebsd.org> Message-ID: <20171210132430.M1124@besplex.bde.org> References: <201712091559.vB9FxAv9001255@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=27B39O857qpdsyqPqwEA:9 a=FxGlBrPq6Vi2QFDL:21 a=QMWDO7Dw0PKc16hh:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Dec 2017 05:03:43 -0000 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