svn commit: r216967 - head/usr.sbin/rtprio

[Bruce Evans]

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