Skip site navigation (1)Skip section navigation (2)
Date:      Wed, 21 Aug 2013 23:37:55 +0200
From:      Jilles Tjoelker <jilles@stack.nl>
To:        Sergey Kandaurov <pluknet@freebsd.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org, Andrey Chernov <ache@freebsd.org>
Subject:   Re: svn commit: r254600 - head/lib/libutil
Message-ID:  <20130821213755.GA8052@stack.nl>
In-Reply-To: <20130821212413.GC52908@omg>
References:  <201308211646.r7LGk6eV051215@svn.freebsd.org> <5214F72B.7070006@freebsd.org> <20130821190309.GB52908@omg> <20130821202725.GA4991@stack.nl> <20130821212413.GC52908@omg>

next in thread | previous in thread | raw e-mail | index | archive | help
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.

> > > Thanks for pointing out.
> > > Does the patch look good?

> > > Index: expand_number.c
> > > ===================================================================
> > > --- expand_number.c     (revision 254600)
> > > +++ expand_number.c     (working copy)
> > > @@ -53,6 +53,8 @@
> > >         unsigned shift;
> > >         char *endptr;
> > >  
> > > +       errno = 0;
> > > +
> > >         number = strtoumax(buf, &endptr, 0);
> > >  
> > >         if (number == UINTMAX_MAX && errno == ERANGE) {

> > This may cause the function to set errno=0 if it is successful, which is
> > not allowed for standard library functions from C and POSIX. There may
> > be a problem not only if expand_number() is standardized but also if it
> > is used in the implementation of a standard library function. The best
> > solution is to save and restore errno around this (if [ERANGE] is
> > detected, that is a valid errno value to keep).

> > In an application it is acceptable to set errno=0 without further ado.

> What about this change?
> It changes errno only if it was modified from zero in strtoumax().
> Unconditionally restoring errno after strtoumax() unless ERANGE is
> probably not good as strtoumax() might set different errno (e.g. EINVAL)
> and we might want to keep it as well.  Please correct me, if I'm wrong.

> Index: lib/libutil/expand_number.c
> ===================================================================
> --- lib/libutil/expand_number.c (revision 254600)
> +++ lib/libutil/expand_number.c (working copy)
> @@ -50,15 +50,22 @@
>  expand_number(const char *buf, uint64_t *num)
>  {
>         uint64_t number;
> +       int saved_errno;
>         unsigned shift;
>         char *endptr;
>  
> +       saved_errno = errno;
> +       errno = 0;
> +
>         number = strtoumax(buf, &endptr, 0);
>  
>         if (number == UINTMAX_MAX && errno == ERANGE) {
>                 return (-1);
>         }
>  
> +       if (errno == 0)
> +               errno = saved_errno;
> +
>         if (endptr == buf) {
>                 /* No valid digits. */
>                 errno = EINVAL;
> 

This looks good to me.
-- 
Jilles Tjoelker



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130821213755.GA8052>