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 <k...@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
_______________________________________________
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
