On Mon 2021-02-08 17:38:29, Richard Fitzgerald wrote:
> On 08/02/2021 15:18, Andy Shevchenko wrote:
> > On Mon, Feb 08, 2021 at 02:01:52PM +0000, Richard Fitzgerald wrote:
> > > The existing code attempted to handle numbers by doing a strto[u]l(),
> > > ignoring the field width, and then repeatedly dividing to extract the
> > > field out of the full converted value. If the string contains a run of
> > > valid digits longer than will fit in a long or long long, this would
> > > overflow and no amount of dividing can recover the correct value.
> > >
> > > -unsigned long long simple_strtoull(const char *cp, char **endp, unsigned int base)
> > > +static unsigned long long simple_strntoull(const char *startp, size_t max_chars,
> > > + char **endp, unsigned int base)
> > > {
> > > - unsigned long long result;
> > > + const char *cp;
> > > + unsigned long long result = 0ULL;
> > > unsigned int rv;
> > > - cp = _parse_integer_fixup_radix(cp, &base);
> > > - rv = _parse_integer(cp, base, &result);
> > > + cp = _parse_integer_fixup_radix(startp, &base);
> > > + if ((cp - startp) >= max_chars) {
> > > + cp = startp + max_chars;
> > > + goto out;
> > > + }
> > > +
> > > + max_chars -= (cp - startp);
> > > + rv = _parse_integer_limit(cp, base, &result, max_chars);
> > > /* FIXME */
> > > cp += (rv & ~KSTRTOX_OVERFLOW);
> > > +out:
> > > if (endp)
> > > *endp = (char *)cp;
> > > return result;
> > > }
> >
> > A nit-pick: What if we rewrite above as
> >
> > static unsigned long long simple_strntoull(const char *cp, size_t max_chars,
> > char **endp, unsigned int base)
> > {
> > unsigned long long result = 0ULL;
> > const char *startp = cp;
> > unsigned int rv;
> > size_t chars;
> >
> > cp = _parse_integer_fixup_radix(cp, &base);
> > chars = cp - startp;
> > if (chars >= max_chars) {
> > /* We hit the limit */
> > cp = startp + max_chars;
> > } else {
> > rv = _parse_integer_limit(cp, base, &result, max_chars - chars);
> > /* FIXME */
> > cp += (rv & ~KSTRTOX_OVERFLOW);
> > }
> >
> > if (endp)
> > *endp = (char *)cp;
> >
> > return result;
> > }
> >
> > ...
>
>
> I don't mind rewriting that code if you prefer that way.
> I am used to working on other kernel subsytems where the preference is
> to bail out on the error case so that the "normal" case flows without
> nesting.
Yeah. But in this case Andy's variant looks slightly better redable to me.
...
> >
> > > + val.s = simple_strntoll(str,
> > > + field_width > 0 ? field_width : SIZE_MAX,
> > > + &next, base);
> >
> > A nit-pick: Wouldn't be negative field_width "big enough" to just being used as
> field_width is s16 so really should be sign-extended
I guess that Andy just missed that it was a signed type. And it has to be
because -1 means SIZE_MAX.
> to make it "very
> big". I think this would be less readable what the intention is and what
> assumptions it is based on. There's a risk someone would look at
>
> (size_t)(long)field_width
>
> and think the (long) is redundant.
> Perhaps change field_width to int? There I ask myself "if it can be an
> int, why is it declared s16?" and worry there is something subtle in the
> code.
>
> My personal preference is to avoid using tricks in code that isn't time
> critical.
I agree. Let's keep the check with signed type.
> > is? Also, is field_width == 0 should be treated as "parse to the MAX"?
filed_width == 0 actually means that no characters are read. I should
return zero value.
> > ...
>
> Earlier code terminates scanning if the width parsed from the format
> string is <= 0.
To make it clear what earlier code means. vsscanf() bail out earlier
when field_width == 0. It is handled by this code:
/* get field width */
field_width = -1;
if (isdigit(*fmt)) {
field_width = skip_atoi(&fmt);
if (field_width <= 0)
break;
}
> So field_width can only be -1 or > 0 here. But now you
> point it out, that test would be better as field_width >= 0 ... so
> it deals with 0 if it ever happened to sneak through to here
> somehow.
It might make sense to be proactive and change it to >= 0.
But I would do it in a separate patch. The "< 0" condition
matches the original code.
Best Regards,
Petr
