Date: Fri, 26 Sep 2014 15:26:55 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: d@delphij.net Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, Mateusz Guzik <mjguzik@gmail.com>, Xin LI <delphij@freebsd.org>, src-committers@freebsd.org Subject: Re: svn commit: r272144 - head/sbin/sysctl Message-ID: <20140926132358.T2195@besplex.bde.org> In-Reply-To: <5424B11E.4030909@delphij.net> References: <201409252237.s8PMbScl041679@svn.freebsd.org> <20140925224035.GA6065@dft-labs.eu> <5424B11E.4030909@delphij.net>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 25 Sep 2014, Xin Li wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA512 > > On 09/25/14 15:40, Mateusz Guzik wrote: >> On Thu, Sep 25, 2014 at 10:37:28PM +0000, Xin LI wrote: >>> Author: delphij Date: Thu Sep 25 22:37:27 2014 New Revision: >>> 272144 URL: http://svnweb.freebsd.org/changeset/base/272144 >>> >>> Log: The strtol(3) family of functions would set errno when it >>> hits one. Check errno and handle it as invalid input. >> >> But this requires explicitely setting errno to 0 before strto* >> call, otherwise you cannot know whether errno is meaningful when >> you test it. Yes, it has error #3 of 1-10 for programming the strto*() family. (I made up the #3, and 10 is as high as I can count.) At least the following errors were already present: - clobbering the return value by assigning it to a variable of a wrong type. This causes implementation-defined behaviour. It prevents some types of range checking and some types of error reporting. - using a cast to possibly break possible compiler warnings about truncation from the previous bug - for CTLTYPE_UINT, using the wrong bogus cast (to int instead of u_int) to possibly not even break the compiler warnings, but to break the value on exotic arches - no range checking at all. I think you just added some, but with a bad error message. Input that is out of bounds is a little different from general invalid input. - for CTLTYPE_UINT, also using misformatting (space after cast) in the wrong bogus cast - verboseness. Despite the missing error handling, the code is large. Before the switch statement, there is a silly special check for all the numeric types. This checks for empty strings using a slow method of strlen() instead of checking if the first character is NUL (perhaps the compiler can optimize this). Leading whitespace is stripped before this, so this finds a few more errors than empty strings. But all this does is print "empty numeric value" of the less specific "invalid integer ''%s" when the string is empty. (Here %s prints the variable 'line'. It looks like an output formatting error to append it unquoted to the quoted string newval, but I couldn't find an example where 'line' is nonempty. The special cases is removed in my version. I didn't fix the other errors. The following error was not present: - saying in error messages that invalid input is "illegal". sysctl.c has worse style bugs than the verboseness. The case statement containing most of the strto*() statements is misindented. This gives verboseness and style bugs like splitting error messages strings colaterally, although not long lines. >> Also it looks like the code would use some deduplications with >> macros or something. > > I think the code can use some refactor. Does the attached patch look > good to you? No, it looks bad. % Index: sysctl.c % =================================================================== % --- sysctl.c (revision 272145) % +++ sysctl.c (working copy) % @@ -57,6 +57,7 @@ static const char rcsid[] = % #include <machine/pc/bios.h> % #endif % % +#include <assert.h> The necessary error handling is ugly enough. % #include <ctype.h> % #include <err.h> % #include <errno.h> % @@ -80,8 +81,32 @@ static int show_var(int *, int); % static int sysctl_all(int *oid, int len); % static int name2oid(const char *, int *); % % -static int set_IK(const char *, int *); % +static int strIKtoi(const char *, char **); % % +static int ctl_sign[CTLTYPE+1] = { % + [CTLTYPE_INT] = 1, % + [CTLTYPE_LONG] = 1, % + [CTLTYPE_S64] = 1, % +}; % + % +static int ctl_size[CTLTYPE+1] = { % + [CTLTYPE_INT] = sizeof(int), % + [CTLTYPE_UINT] = sizeof(u_int), % + [CTLTYPE_LONG] = sizeof(long), % + [CTLTYPE_ULONG] = sizeof(u_long), % + [CTLTYPE_S64] = sizeof(int64_t), % + [CTLTYPE_U64] = sizeof(uint64_t), % +}; % + % +static const char *ctl_typename[CTLTYPE+1] = { % + [CTLTYPE_INT] = "integer", % + [CTLTYPE_UINT] = "unsigned integer", % + [CTLTYPE_LONG] = "long integer", % + [CTLTYPE_ULONG] = "unsigned long", % + [CTLTYPE_S64] = "int64_t", % + [CTLTYPE_U64] = "uint64_t", % +}; % + % static void % usage(void) % { Takes about the same amount of code as before, and is harder to read. Loses some context-dependent error messages. Add tables to restore those and it might be larger than before. The old code could use goto to common error handling code if to reduce duplication in a different ugly way. % @@ -288,10 +314,22 @@ parse(const char *string, int lineno) % (kind & CTLTYPE) == CTLTYPE_ULONG || % (kind & CTLTYPE) == CTLTYPE_S64 || % (kind & CTLTYPE) == CTLTYPE_U64) { % - if (strlen(newval) == 0) { % + if (strlen(newvalstr) == 0) { % warnx("empty numeric value"); % return (1); % } Just remove this. % + } else if ((kind & CTLTYPE) != CTLTYPE_STRING) { % + /* % + * The only acceptable types are: % + * CTLTYPE_INT, CTLTYPE_UINT, % + * CTLTYPE_LONG, CTLTYPE_ULONG, % + * CTLTYPE_S64, CTLTYPE_U64 and % + * CTLTYPE_STRING. % + */ % + warnx("oid '%s' is type %d," % + " cannot set that%s", bufp, % + kind & CTLTYPE, line); % + " cannot set that%s", bufp, % + return (1); % } Don't increase it. This adds (actually moves) the following style bugs: - obfuscating a string by splitting it - splitting a string that doesn't even require splitting to fit in 80 columns - non-KNF indentation in the splitting. % % errno = 0; Extra blank line before and after this. % @@ -298,91 +336,53 @@ parse(const char *string, int lineno) % % switch (kind & CTLTYPE) { % case CTLTYPE_INT: The case statement is still misindented. This gives lots of ugly line splitting (most actually needed) below. % - if (strcmp(fmt, "IK") == 0) { % - if (!set_IK(newval, &intval)) { % - warnx("invalid value '%s'%s", % - (char *)newval, line); % - return (1); % - } % - } else { % - intval = (int)strtol(newval, &endptr, % + if (strcmp(fmt, "IK") == 0) % + intval = strIKtoi(newvalstr, &endptr); % + else % + intval = (int)strtol(newvalstr, &endptr, % 0); The style here is not too bad. % - if (errno != 0 || endptr == newval || % - *endptr != '\0') { % - warnx("invalid integer '%s'%s", % - (char *)newval, line); % - return (1); % - } % - } All integer values should be parsed using strtoumax(). The range checking that needs to be added to this is only slightly more complicated that the range checking that is needed using special strto*(), except for CTLTYPE_LONG and CTLTYPE_ULONG where strtol() and strtoul() match the type exactly. Conversion from uintmax_t to a signed type is a little more complicated. % newval = &intval; % newsize = sizeof(intval); % break; % case CTLTYPE_UINT: % - uintval = (int) strtoul(newval, &endptr, 0); % - if (errno != 0 || endptr == newval || % - *endptr != '\0') { % - warnx("invalid unsigned integer '%s'%s", % - (char *)newval, line); % - return (1); % - } % + uintval = (int) strtoul(newvalstr, &endptr, 0); The bogus casts and style bugs in them remain. Correct code in all cases begins with something like: uintmxval = strtoumax(newvalstr, &endptr, 0); I don't see how to do range checking on this cleanly except by using special code for every case. In this case, it is something like: if (uintmxval > UINT_MAX) errno = ERANGE; uintval = (u_int)uintmaxval; /* unused if ERANGE */ Then continue as before. Not too bad. Printing a specific error message with the range for this cased would take more work. The signed cases also need to detect and put back the sign. % newval = &uintval; % newsize = sizeof(uintval); % break; This needs some specific code, but there was already a table of sizes, so the newsize initialization in it can be moved out of the switch. But I prefer a simple switch. The size table (ctl_size[]) just allows avoiding 1 line in each statement of the corresponding switch for output, at the cost of a more complicated and unusally structured switch. % ... % - case CTLTYPE_OPAQUE: % - /* FALLTHROUGH */ % default: % - warnx("oid '%s' is type %d," % - " cannot set that%s", bufp, % - kind & CTLTYPE, line); % + /* NOTREACHED */ % return (1); This was better placed in where it was. It needed a verbose comment even less then. % } % Style bug (extra blank line to detach the error handling). % + if (errno != 0 || endptr == newvalstr || *endptr != '\0') { % + warnx("invalid %s '%s'%s", ctl_typename[kind & CTLTYPE], % + newvalstr, line); % + return (1); % + } Printing the typename is not very useful except in the case of a range error. Then the user can use it to decrypt the rest of the message from "invalid" to "range error" after looking up or know the range of the variable type. That type is an implementation detail but it is not obvious what it is. % + % i = show_var(mib, len); % if (sysctl(mib, len, 0, 0, newval, newsize) == -1) { % if (!i && !bflag) % @@ -665,33 +665,35 @@ S_bios_smap_xattr(size_t l2, void *p) % #endif % % static int % -set_IK(const char *str, int *val) % +strIKtoi(const char *str, char **endptrp) % { % + int kelv; % float temp; % - int len, kelv; % + size_t len; % const char *p; % - char *endptr; % % - if ((len = strlen(str)) == 0) % - return (0); % + assert(errno == 0); Silly. It's easier to set it again than check it. % + Style bug (extra blank line). % + len = strlen(str); % + /* caller already checked this */ Style bugs (comment capitalization and punctuation). % + assert(len > 0); The caller shouldn't have checked this. % + % p = &str[len - 1]; Was sloppy. I would parse the number and check for a suffix later. Perhaps there is a problem with the F suffix being part of a float-precision number. % - errno = 0; % if (*p == 'C' || *p == 'F') { % - temp = strtof(str, &endptr); % - if (errno != 0 || endptr == str || % - endptr != p) % - return (0); % - if (*p == 'F') % - temp = (temp - 32) * 5 / 9; % - kelv = temp * 10 + 2732; % + temp = strtof(str, endptrp); strtof() and the calculations using its value require different and larger range checking than the integer functions. This was mostly missing. % + if (*endptrp != str && *endptrp == p && errno != 0) { strtof() can return NaN or HUGE_VALF on errors. It then sets errno. The previous version added the missing errno check. % + if (*p == 'F') % + temp = (temp - 32) * 5 / 9; Overflow still occurs for bad input. So does underflow and total loss of precision. Garbage values below 0 degrees Kelvin are accepted and turned into worse garbage. It seems reasonable to disallow values that are below 0 in Kelvin or above the boiling point of Silicon. This gives a simple sanity check. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140926132358.T2195>