[PATCH] Fix string to integer conversion

[swen]

Hi Ralph
On Tue, 2019-02-26 at 11:15 +0100, Ralph Böhme wrote:
> On Tue, Feb 26, 2019 at 09:22:07AM +0100, swen wrote:
> > Hi Ralph
> > 
> > I just wish you would have come up with your thoughts/ideas
> > when the discussion was on 3-4 weeks ago :-)
> 
> yeah, sorry, been really busy so I wasn't able to keep up with some
> stuff.
> 
> > On Mon, 2019-02-25 at 18:56 +0100, Ralph Böhme via samba-technical
> > wrote:
> > > Hi Swen,
> > > 
> > > On Mon, Feb 25, 2019 at 01:00:34PM +0000, Swen Schillig wrote:
> > > 
> > > The problem is that the new function and the use of the new
> > > function
> > > in a bunch
> > > of places suggests that you can *rely* on the error out variable
> > > being for
> > > strings like "" or "foo".
> > > 
> > > Eg in one place your patch adds this:
> > > 
> > > --- a/ctdb/tools/ctdb.c
> > > +++ b/ctdb/tools/ctdb.c
> > > @@ -544,7 +545,10 @@ static bool db_exists(TALLOC_CTX *mem_ctx,
> > > struct ctdb_context *ctdb,
> > >         }
> > > 
> > > -               id = strtoul(db_arg, NULL, 0);
> > > +               id = strtoul_err(db_arg, NULL, 0, &ret);
> > > +               if (ret != 0) {
> > > +                       return false;
> > > +               }
> > > 
> > What about if something like this is added ?
> > 
> > --- a/lib/util/util.c
> > +++ b/lib/util/util.c
> > @@ -64,6 +64,13 @@ strtoul_err(const char *nptr, char **endptr, int
> > base, int *err)
> >                *endptr = tmp_endptr;
> >        }
> > 
> > +       if (nptr == tmp_endptr) {
> > +               /* we got an invalid number-string resulting in no
> > conversion */
> > +               *err = EINVAL;
> > +               errno = save_errno;
> > +               return val;
> > +       }
> > +
> >        if (errno != 0) {
> >                *err = errno;
> >                errno = saved_errno;
> > 
> > This will catch the non-conversion case and leaves everything else
> > as it is ?
> 
> Then I guess we also need
> 
> +       if (nptr[0] == '\0') {
> +               /* we got an empty string */
> +               *err = EINVAL;
> +               errno = save_errno;
> +               return val;
> +       }
> +
> 
> before the call to strtoul().

No why ?
strtoul(l) can deal with leading blanks and even the "new" check
for fully wrong strings is still working.
..something like " foo" would be detected as invalid while
something like " 1234" would be valid.
Isn't that what we want ?

Not sure if you know, but there are places in the code 
(I remember seeing at least one) where strings like "1234 foo" should
be considered valid, with a conversion of 1234 only of course.

> 
> Then lets remove the out-parameter uglyness and we're going
> somewhere:
> 
> bool tstrtoull(const char *nptr, char **endptr, int base, unsigned
> long long int *val);
> 
> :)
> 
> Let's see what Jeremy has to say on it, I'll give him a chance to
> convince me 
> that strtoull_err() in the current form is really a good idea.
> 
well, I prefer the old-fashioned idea as well, not just Jeremy :-)
Instead, could you come up with an explanation why your approach would 
be so beneficial ?

> I imagine one previous argument has been, that a strict
> implementation of a new 
> conversion function, be it strtoull_err() or tstrtoull(), may not be
> suitable to 
> replace the existing call to strtoull() in all places due to the
> behaviour 
> change?
To be honest, I prefer to not open Pandora's box here and go 
with what I'd call a fix instead of a design change.
> 
> Btw, there'a bug in strtoull_err(), fixup attached.

I know, I fixed it already (running at gitlab currently) but was
waiting for more "concerns" to be fixed with a new version.

But thanks :-)

Cheers Swen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20190226/100b3407/signature.sig>