Re: [PATCH v4 2/4] lib: vsprintf: Fix handling of number field widths in vsscanf

[Andy Shevchenko]

On Fri, Feb 5, 2021 at 1:35 PM Richard Fitzgerald
<rf@xxxxxxxxxxxxxxxxxxxxx> wrote:
> On 04/02/2021 16:35, Petr Mladek wrote:
> > On Wed 2021-02-03 21:45:55, Andy Shevchenko wrote:
> >> On Wed, Feb 03, 2021 at 04:50:07PM +0000, Richard Fitzgerald wrote:

...

> >>> +   for (; max_chars > 0; max_chars--) {
> >>
> >> Less fragile is to write
> >>
> >>      while (max_chars--)
> >
> > Except that the original was more obvious at least for me.
> > I always prefer more readable code when the compiler might do
> > the optimization easily. But this is my personal taste.
> > I am fine with both variants.

I *slightly* prefer while-loop *in this case* due to less characters
to parse to understand the logic.

> >> This allows max_char to be an unsigned type.
> >>
> >> Moreover...
> >>
> >>> +   return _parse_integer_limit(s, base, p, INT_MAX);
> >>
> >> You have inconsistency with INT_MAX vs, size_t above.
> >
> > Ah, this was on my request. INT_MAX is already used on many other
> > locations in vsnprintf() for this purpose.
>
> I originally had UINT_MAX and changed on Petr's request to be
> consistent with other code. (Sorry Andy - my mistake not including
> you on the earlier review versions).
>
> But 0 < INT_MAX < UINT_MAX, so ok to pass to an unsigned. And as Petr
> said on his original review, INT_MAX is "big enough".

Some code has INT_MAX, some has UINT_MAX, while the parameter is size_t.
I think all of these inconsistencies should have a comment either in
the code, or in the commit message, or in the cover letter (depending
on the importance).
Or being fixed to be more consistent with existing code. Whichever you
consider better.

> I don't mind either way.
>
> > An alternative is to fix all the other locations. We would need to
> > check if it is really safe. Well, I do not want to force Richard
> > to fix this historical mess. He already put a lot lot of effort
> > into fixing this long term issue.

...

> >>> -   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);
> >>
> >>> +   if (max_chars == 0) {
> >>> +           cp = startp;
> >>> +           goto out;
> >>> +   }
> >>
> >> It's redundant if I'm not mistaken.
> >
> > Also this is more obvious and less error prone from my POV.
> > But I agree that it is redundant. I am not sure if this
> > function is used in some fast paths.
> >
> > Again I am fine with both variants.

I think we don't need a (redundant) code, but rather a comment.

> >>> +   cp = _parse_integer_fixup_radix(startp, &base);
> >>> +   if ((cp - startp) >= max_chars) {
> >>> +           cp = startp + max_chars;
> >>> +           goto out;
> >>> +   }
> >>
> >> This will be exactly the same, no?

-- 
With Best Regards,
Andy Shevchenko