Date: Thu, 29 Aug 2013 15:14:56 +0400 From: Sergey Kandaurov <pluknet@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Andrey Chernov <ache@freebsd.org>, Jilles Tjoelker <jilles@stack.nl> Subject: Re: svn commit: r254600 - head/lib/libutil Message-ID: <CAE-mSO%2BuzOG2QbhiPbcSwPCUupaMtHvhPc61JEjyb4E-XuHLiw@mail.gmail.com> In-Reply-To: <20130822185756.Y1297@besplex.bde.org> References: <201308211646.r7LGk6eV051215@svn.freebsd.org> <5214F72B.7070006@freebsd.org> <20130821190309.GB52908@omg> <20130821202725.GA4991@stack.nl> <20130821212413.GC52908@omg> <20130821213755.GA8052@stack.nl> <20130822185756.Y1297@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 22 August 2013 14:51, Bruce Evans <brde@optusnet.com.au> wrote:
> On Wed, 21 Aug 2013, Jilles Tjoelker wrote:
>
>> On Thu, Aug 22, 2013 at 01:24:13AM +0400, Sergey Kandaurov wrote:
>>>
>>> On Wed, Aug 21, 2013 at 10:27:25PM +0200, Jilles Tjoelker wrote:
>>>>
>>>> On Wed, Aug 21, 2013 at 11:03:10PM +0400, Sergey Kandaurov wrote:
>>>>>
>>>>> On Wed, Aug 21, 2013 at 09:21:47PM +0400, Andrey Chernov wrote:
>>>>>>
>>>>>> On 21.08.2013 20:46, Sergey Kandaurov wrote:
>>>>>>>
>>>>>>> number = strtoumax(buf, &endptr, 0);
>>>>>>>
>>>>>>> + if (number == UINTMAX_MAX && errno == ERANGE) {
>>>>>>> + return (-1);
>>>>>>> + }
>>
>>
>>>>>> You need to reset errno before strtoumax() call (errno = 0), because
>>>>>> any
>>>>>> of previous functions may left it as ERANGE.
>
>
> This also has a bogus test for UINTMAX_MAX wheich prevents input of
> the number UINTMAX_MAX on systems where the limit for the function
> (UINT64_T_MAX) is the same as the limit for strtoumax(). This test is
> a wrong way of doing the errno thing.
>
> This also has a high density of style bugs:
> - excessive braces
> - extra blank line
>
> expand_number() remains a very badly designed and implemented function.
> Its design errors start with its name. It doesn't expand numbers. It
> converts strings to numbers. The strtoumax() family is missing this
> bug and others. Its design errors continue with it taking a uint64_t
> arg and not a uint64_t arg.
>
> Its implementation errors begin with broken range checking. The above
> is an incomplete fix for this. Unless uintmax_t is the same as uint64_t,
> there are still the following bugs:
> - the return value of strotumax() is assigned to a uint64_t. This may
> overflow. After overflow, the bogus (number == UINTMAX_MAX) check
> has no effect, since when overflow is possible 'number' cannot equal
> UINTMAX_MAX.
> - values that exceed the maximum supported (UINT64_MAX) are not detected.
> They are silently truncated to uint64_t.
Ack.
[...]
>
> Some of the other bugs in the old version:
>
> % /*
> % * Convert an expression of the following forms to a uint64_t.
> % * 1) A positive decimal number.
> % * 2) A positive decimal number followed by a 'b' or 'B' (mult by 1).
> % * 3) A positive decimal number followed by a 'k' or 'K' (mult by 1 <<
> 10).
> % * 4) A positive decimal number followed by a 'm' or 'M' (mult by 1 <<
> 20).
> % * 5) A positive decimal number followed by a 'g' or 'G' (mult by 1 <<
> 30).
> % * 6) A positive decimal number followed by a 't' or 'T' (mult by 1 <<
> 40).
> % * 7) A positive decimal number followed by a 'p' or 'P' (mult by 1 <<
> 50).
> % * 8) A positive decimal number followed by a 'e' or 'E' (mult by 1 <<
> 60).
>
> This garbage was copied from a similar comment in dd/args.c. The number is
> actually neither positive nor decimal, especially in dd where there is a
> variant of this function that returns negative numbers. Another design
> error
> in this function is that it can't return negative numbers.
Well, I see no useful contents in this comment. It is obvious and verbose.
Probably it should be trimmed.
>
> % */
> %
>
> This bogus blank line detaches the comment from the code that it describes,
> resulting in the comment describing null code.
Sorry, I didn't see such blank line in head.
>
> % int
> % expand_number(const char *buf, uint64_t *num)
> % {
> % uint64_t number;
> % unsigned shift;
> % char *endptr;
> % % number = strtoumax(buf, &endptr, 0);
>
> This supports non-positive non-decimal numbers. The support for non-decimal
> numbers is more intentional (it is given by the base 0 in the call).
> Negative
> numbers are only supported by strtoumax() allowing a minus sign and
> returning a nonnegative number equal to 1 larger than UINTMAX_MAX less the
> number without the minus sign.
>
Fixing the verbose comment above would result in adding more verboseness
citing from man strtoumax(3) wrt. signedness and base details.
Yet another point to trim it.
[ dd dd details here ]
>
> To start fixing expand_number():
>
> /* Fix comment here. */
> int
>
> expand_number(const char *buf, uint64_t *num)
> {
> char *endptr;
> uintmax_t num;
> uint64_t number;
> unsigned shift;
> int serrno;
>
> serrno = errno;
> errno = 0;
> num = strtoumax(buf, &endptr, 0);
> if (num > UINT64_MAX)
> errno = ERANGE;
> if (errno != 0)
> return (-1);
> errno = serrno;
> number = num;
> ...
> }
>
> This depends on the POSIX bug for detecting the no-digits case.
> Fortunately, the early error handling of this function is simple and
> compatible with that of strtoumax(), so we don't need to translate
> strtoumax()'s errors except for adjusting its range error.
>
Thanks for your valuable input. What about this change (on top of head)?
Index: expand_number.c
===================================================================
--- expand_number.c (revision 255017)
+++ expand_number.c (working copy)
@@ -35,42 +35,29 @@
#include <libutil.h>
#include <stdint.h>
-/*
- * Convert an expression of the following forms to a uint64_t.
- * 1) A positive decimal number.
- * 2) A positive decimal number followed by a 'b' or 'B' (mult by 1).
- * 3) A positive decimal number followed by a 'k' or 'K' (mult by 1 << 10).
- * 4) A positive decimal number followed by a 'm' or 'M' (mult by 1 << 20).
- * 5) A positive decimal number followed by a 'g' or 'G' (mult by 1 << 30).
- * 6) A positive decimal number followed by a 't' or 'T' (mult by 1 << 40).
- * 7) A positive decimal number followed by a 'p' or 'P' (mult by 1 << 50).
- * 8) A positive decimal number followed by a 'e' or 'E' (mult by 1 << 60).
- */
int
expand_number(const char *buf, uint64_t *num)
{
+ char *endptr;
+ uintmax_t umaxval;
uint64_t number;
- int saved_errno;
unsigned shift;
- char *endptr;
+ int serrno;
- saved_errno = errno;
+ serrno = errno;
errno = 0;
-
- number = strtoumax(buf, &endptr, 0);
-
- if (number == UINTMAX_MAX && errno == ERANGE) {
- return (-1);
- }
-
- if (errno == 0)
- errno = saved_errno;
-
+ umaxval = strtoumax(buf, &endptr, 0);
if (endptr == buf) {
/* No valid digits. */
errno = EINVAL;
return (-1);
}
+ if (umaxval > UINT64_MAX)
+ errno = ERANGE;
+ if (errno != 0)
+ return (-1);
+ errno = serrno;
+ number = umaxval;
switch (tolower((unsigned char)*endptr)) {
case 'e':
--
wbr,
pluknet
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAE-mSO%2BuzOG2QbhiPbcSwPCUupaMtHvhPc61JEjyb4E-XuHLiw>