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

[Petr Mladek]

On Fri 2021-02-05 14:50:56, Andy Shevchenko wrote:
> 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:
> > >> 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.

Yeah, if I remember correctly I wanted to have INT_MAX everywhere but
I did not want to nitpick about it in the later versions. It looked
like an arbitrary number anyway.

> 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.

OK, you made me to do some archaeology. The INT_MAX limit has
been added into vsnprintf() in 2.6.2 by the commit:

Author: Linus Torvalds <torvalds@xxxxxxxxxxxxx>
Date:   Mon Feb 2 21:17:29 2004 -0800

    Warn loudly if somebody passes a negative value as
    the size to "vsnprintf()".

    That's a pretty clear case of overflow.


It might catch problems. And the limit seems to have worked all the time.

IMHO, it would make sense to have INT_MAX limit also in
_parse_integer_limit() and WARN() when a larger value is passed.

By other words, it would mean to add this check and use INT_MAX
everywhere in this patch.

Best Regards,
Petr