Date: Wed, 5 Jan 2011 07:56:19 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Garrett Cooper <yanegomi@gmail.com> Cc: src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, svn-src-all@freebsd.org, Garrett Cooper <gcooper@freebsd.org>, Konstantin Belousov <kib@freebsd.org>, svn-src-head@freebsd.org Subject: Re: svn commit: r216967 - head/usr.sbin/rtprio Message-ID: <20110105064831.B1008@besplex.bde.org> In-Reply-To: <004687DF-47EF-4468-97DB-8443A249BE00@gmail.com> References: <201101041727.p04HRHZC048894@svn.freebsd.org> <AANLkTikqR3dzNV23Ti=cDmhig93Ko3LTgLh7HCaQFv5-@mail.gmail.com> <201101041322.33071.jhb@freebsd.org> <004687DF-47EF-4468-97DB-8443A249BE00@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 4 Jan 2011, Garrett Cooper wrote: > On Jan 4, 2011, at 10:22 AM, John Baldwin wrote: > >> On Tuesday, January 04, 2011 12:41:51 pm Garrett Cooper wrote: >>> On Tue, Jan 4, 2011 at 9:27 AM, Konstantin Belousov <kib@freebsd.org> wrote: >>>> Author: kib >>>> Date: Tue Jan 4 17:27:17 2011 >>>> New Revision: 216967 >>>> URL: http://svn.freebsd.org/changeset/base/216967 >>>> >>>> Log: >>>> Use errx() instead of err() in parseint. There is usually no interesting >>>> information in errno. >>>> >>>> Noted by: Garrett Cooper <yanegomi gmail com> >>>> MFC after: 1 week >>> >>> Someday it would be nice if we could have one standard / libraritized >>> set of functions that properly handle overflow and all that good stuff >>> (as bde has suggested to me elsewhere), but this is good enough for >>> today. >> >> strtonum(3)? > > bde said (back in 11/15): > > "It is good to fix the blind truncation, but overflow is never graceful. > > I will review the patch for errors like the ones in corresponding user > code (dd get_num(), strtonum(), expand_number()... get_num() is almost > OK, but the newer more public APIs are broken as designed)." > > So there's some concern that he has with those APIs still.. the > fact that I was trying to use a similar API to parse numbers in strings > in the kernel (strtoq in tunable work) didn't make things any better > unfortunately. Part of the brokenness is that strtonum() uses the long long abomination nstead of [u]intmax_t. humanize_number() and expand_number() also haven't caught up with C99 -- they use uint64_t. % Modified: head/usr.sbin/rtprio/rtprio.c % ============================================================================== % --- head/usr.sbin/rtprio/rtprio.c Tue Jan 4 17:18:53 2011 (r216966) % +++ head/usr.sbin/rtprio/rtprio.c Tue Jan 4 17:27:17 2011 (r216967) % @@ -133,9 +133,9 @@ parseint(const char *str, const char *er % errno = 0; % res = strtol(str, &endp, 10); % if (errno != 0 || endp == str || *endp != '\0') % - err(1, "%s must be a number", errname); % + errx(1, "%s must be a number", errname); % if (res >= INT_MAX) % - err(1, "Integer overflow parsing %s", errname); % + errx(1, "Integer overflow parsing %s", errname); % return (res); % } Unfortunately this has many of the common bugs for using the strto*() family: - although it carefully sets errno, it uses errno in a bad way. When strtol() sets ERANGE, this is misinterpreted as a generic error so the misleading message "%s must be a number" is printed for numbers that are out of range. The previous version was better since it also printed the strerror(ERANGE). OTOH, the other error (EINVAL) reported in errno is detected better by the checks on endp. errno being set to EINVAL is a POSIX mistake^Wextextension. Portable code still needs to use the endp checks. rtprio isn't portable but it shouldn't set a bad example. - the other part of the overflow checking is mostly wrong too: strtol() returns LONG_MAX for range errors, but also returns LONG_MIN for range errors; LONG_MAX may or may not exceed INT_MAX; on 32-bit arches it happens to equal INT_MAX. So: (a) overflow of negative values is not detected. Except, all cases of overflow are detected by the generic check on errno and misreported. (b) when LONG_MAX exceeds INT_MAX, there is no problem (yet) when res exceed INT_MAX since although there is no overflow yet, there would be when parseint() returns int. When res equals INT_MAX, there is an off-by-1 error -- a non-overflow case is misreported as overflow. This is almost harmless since a value of INT_MAX is out of range for a pid. The error message is just slightly misleading -- the value is too large, although not overflow. (c) when LONG_MAX equals INT_MAX, res cannot INT_MAX. No problem when the infinite-precision value exceeds INT_MAX (this is overflow). When res equals INT_MAX, there may or may not be overflow. Both cases are reported as overflow. This is mostly harmless, as in (b). (d) Overflow may occur immediately when parseint() returns, since its value is stored in a pid_t with no further checks. pid_t happens to have type int on all supported arches, so there is no problem in practice. (e) Negative values for the pid are not errors, but have a special meaning. The code for handling negative values is laborious and has at least the following bugs: (i) proc = abs(proc) overflows on all supported arches when proc = INT_MIN (ii) only leading '-' signs are detected by the ad hoc parsing. strtol() will find '-' signs after leading whitespace. Thus negative pids may reach the kernel. (iii) isdigit() is applied to a value that may be negative. As a result of these bugs, the rtprio passed to the kernel can be way out of the valid range [0..31]. Hopefully the kernel knows how to check ranges so the result is just late detection of the invalid option with an error message less specific than it could be. I get "rtprio: rtprio: No such process" for "rtprio 22 -33333333" with an old version of rtprio(1) using atoi(). Similarly with enough more 3's to overflow to LONG_MIN. Bit with 21 3's, the error changes to the weird "No such file or directory". Summary: all the strto*() interfaces are too hard to use. When fixing use of atoi(), you want to be able to use the same API (maybe s/atoi/xatoi/, where xatoi exits on error) and not have to write a buggy parse for each case or even change to a more complicated API like strtonum(). Here the change from atoi() seems to only gain detection of garbage input like "1garbage". Even using atoi(), most programs need range checking that has nothing to do with INT_MAX. Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110105064831.B1008>